Patchwork P2P:Fixing the supplicant crash if a group interface has been removed in the event handler itself

login
register
mail settings
Submitter Neeraj Garg
Date May 29, 2012, 10:01 a.m.
Message ID <F764D07D6734DF489C733939E1A6F85A065A3C@SJEXCHMB12.corp.ad.broadcom.com>
Download mbox | patch
Permalink /patch/161724/
State Superseded
Headers show

Comments

Neeraj Garg - May 29, 2012, 10:01 a.m.
We hit a scenario where a PBC overlap was detected in the context of EVENT_SCAN_RESULTS. When in the event handler of do_process_drv_event, an overlap is detected, it will cause GROUP-FORMATION-FAILURE and that will remove the group interface and then corresponding drv pointer from the list global->interfaces will also get removed. (code path wpas_p2p_group_delete->wpa_supplicant_remove_iface -> wpa_supplicant_deinit_iface -> wpa_drv_deinit ->wpa_driver_nl80211_deinit)

In my opinion it is safe to do break, as the event had an ifidx no. and once that has been found, event is completed. There is no need to go for checking other interfaces. Plz let me know if my understanding is wrong.

From 01b4bf60d99f3b3fa41b057e7bae184db08f5707 Mon Sep 17 00:00:00 2001
From: Neeraj Garg <neerajkg@broadcom.com>
Date: Tue, 29 May 2012 15:23:51 +0530
Subject: [PATCH] P2P:Fixing the supplicant crash if a group interface has been removed in the event handler itself
 Signed-off-by: Neeraj Garg <neerajkg@broadcom.com>

---
 src/drivers/driver_nl80211.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Johannes Berg - May 30, 2012, 4:27 p.m.
On Tue, 2012-05-29 at 10:01 +0000, Neeraj Kumar Garg wrote:
> We hit a scenario where a PBC overlap was detected in the context of
> EVENT_SCAN_RESULTS. When in the event handler of do_process_drv_event,
> an overlap is detected, it will cause GROUP-FORMATION-FAILURE and that
> will remove the group interface and then corresponding drv pointer
> from the list global->interfaces will also get removed. (code path
> wpas_p2p_group_delete->wpa_supplicant_remove_iface ->
> wpa_supplicant_deinit_iface -> wpa_drv_deinit
> ->wpa_driver_nl80211_deinit)
> 
> In my opinion it is safe to do break, as the event had an ifidx no.
> and once that has been found, event is completed. There is no need to
> go for checking other interfaces. Plz let me know if my understanding
> is wrong.

No, the event isn't necessary complete. I suspect what's happening is
that we delete the interface we were looking at just now and then the
list gets messed up while we iterate it ...

http://p.sipsolutions.net/05e21c53908916a7.txt

should help with that.

johannes
Jouni Malinen - June 4, 2012, 5:28 p.m.
On Tue, May 29, 2012 at 10:01:19AM +0000, Neeraj Kumar Garg wrote:
> We hit a scenario where a PBC overlap was detected in the context of EVENT_SCAN_RESULTS. When in the event handler of do_process_drv_event, an overlap is detected, it will cause GROUP-FORMATION-FAILURE and that will remove the group interface and then corresponding drv pointer from the list global->interfaces will also get removed. (code path wpas_p2p_group_delete->wpa_supplicant_remove_iface -> wpa_supplicant_deinit_iface -> wpa_drv_deinit ->wpa_driver_nl80211_deinit)
> 
> In my opinion it is safe to do break, as the event had an ifidx no. and once that has been found, event is completed. There is no need to go for checking other interfaces. Plz let me know if my understanding is wrong.

Thanks! I used the patch from Johannes (dl_list_for_each_safe) since it
looked like a more generic way to avoid the crash.

Patch

diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index 693a885..a6cb0b8 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -2210,8 +2210,10 @@  static int process_global_event(struct nl_msg *msg, void *arg)
 	dl_list_for_each(drv, &global->interfaces,
 			 struct wpa_driver_nl80211_data, list) {
 		if (ifidx == -1 || ifidx == drv->ifindex ||
-		    have_ifidx(drv, ifidx))
+		    have_ifidx(drv, ifidx)) {
 			do_process_drv_event(drv, gnlh->cmd, tb);
+			break;
+		}
 	}
 
 	return NL_SKIP;