| Message ID | 20190212141200.99370-1-nbd@nbd.name |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wpa_supplicant: fix race condition in mesh mpm new peer handling | expand |
On Tue, 2019-02-12 at 15:12 +0100, Felix Fietkau wrote: > When wpa_supplicant receives another new peer event before the first one > has been processed, it tries to add a station to the driver a second time > (which fails) and then tears down the station entry until another event > comes in. > Fix this by only adding a station to the driver if it didn't exist already. > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > wpa_supplicant/mesh_mpm.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/wpa_supplicant/mesh_mpm.c b/wpa_supplicant/mesh_mpm.c > index 44859396c..4bd130fb8 100644 > --- a/wpa_supplicant/mesh_mpm.c > +++ b/wpa_supplicant/mesh_mpm.c > @@ -685,11 +685,12 @@ static struct sta_info * mesh_mpm_add_peer(struct wpa_supplicant *wpa_s, > } > > sta = ap_get_sta(data, addr); > - if (!sta) { > - sta = ap_sta_add(data, addr); > - if (!sta) > - return NULL; > - } That's kinda odd, how could you process another event between those calls? Or am I misunderstanding the situation? johannes
On 2019-02-12 20:15, Johannes Berg wrote: > On Tue, 2019-02-12 at 15:12 +0100, Felix Fietkau wrote: >> When wpa_supplicant receives another new peer event before the first one >> has been processed, it tries to add a station to the driver a second time >> (which fails) and then tears down the station entry until another event >> comes in. >> Fix this by only adding a station to the driver if it didn't exist already. >> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> >> --- >> wpa_supplicant/mesh_mpm.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/wpa_supplicant/mesh_mpm.c b/wpa_supplicant/mesh_mpm.c >> index 44859396c..4bd130fb8 100644 >> --- a/wpa_supplicant/mesh_mpm.c >> +++ b/wpa_supplicant/mesh_mpm.c >> @@ -685,11 +685,12 @@ static struct sta_info * mesh_mpm_add_peer(struct wpa_supplicant *wpa_s, >> } >> >> sta = ap_get_sta(data, addr); >> - if (!sta) { >> - sta = ap_sta_add(data, addr); >> - if (!sta) >> - return NULL; >> - } > > That's kinda odd, how could you process another event between > those calls? From what I can see, it happens when there are multiple calls to mesh_sta_info_get in mac80211 (resulting in calls to cfg80211_notify_new_peer_candidate), before user space has a chance to create the station entry. In my tests this happened quite frequently and resulted in log output like this: Tue Feb 12 12:53:37 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 Tue Feb 12 12:53:37 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 Tue Feb 12 12:53:37 2019 daemon.err wpa_supplicant[14613]: wlan1: Driver failed to insert 78:a3:51:1d:b1:69: -80 Tue Feb 12 12:53:37 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 Tue Feb 12 12:53:38 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 Tue Feb 12 12:53:38 2019 daemon.err wpa_supplicant[14613]: wlan1: Driver failed to insert 78:a3:51:1d:b1:69: -80 - Felix
On 2019-02-12 23:13, Felix Fietkau wrote: > On 2019-02-12 20:15, Johannes Berg wrote: >> On Tue, 2019-02-12 at 15:12 +0100, Felix Fietkau wrote: >>> When wpa_supplicant receives another new peer event before the first one >>> has been processed, it tries to add a station to the driver a second time >>> (which fails) and then tears down the station entry until another event >>> comes in. >>> Fix this by only adding a station to the driver if it didn't exist already. >>> >>> Signed-off-by: Felix Fietkau <nbd@nbd.name> >>> --- >>> wpa_supplicant/mesh_mpm.c | 11 ++++++----- >>> 1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/wpa_supplicant/mesh_mpm.c b/wpa_supplicant/mesh_mpm.c >>> index 44859396c..4bd130fb8 100644 >>> --- a/wpa_supplicant/mesh_mpm.c >>> +++ b/wpa_supplicant/mesh_mpm.c >>> @@ -685,11 +685,12 @@ static struct sta_info * mesh_mpm_add_peer(struct wpa_supplicant *wpa_s, >>> } >>> >>> sta = ap_get_sta(data, addr); >>> - if (!sta) { >>> - sta = ap_sta_add(data, addr); >>> - if (!sta) >>> - return NULL; >>> - } >> >> That's kinda odd, how could you process another event between >> those calls? > From what I can see, it happens when there are multiple calls to > mesh_sta_info_get in mac80211 (resulting in calls to > cfg80211_notify_new_peer_candidate), before user space has a chance to > create the station entry. > In my tests this happened quite frequently and resulted in log output > like this: > > Tue Feb 12 12:53:37 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 > Tue Feb 12 12:53:37 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 > Tue Feb 12 12:53:37 2019 daemon.err wpa_supplicant[14613]: wlan1: Driver failed to insert 78:a3:51:1d:b1:69: -80 > Tue Feb 12 12:53:37 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 > Tue Feb 12 12:53:38 2019 daemon.notice wpa_supplicant[14613]: wlan1: new peer notification for 78:a3:51:1d:b1:69 > Tue Feb 12 12:53:38 2019 daemon.err wpa_supplicant[14613]: wlan1: Driver failed to insert 78:a3:51:1d:b1:69: -80 Seems that my fix was incomplete, it needs to return NULL when the sta entry already exists, otherwise the state machine gets reset. I will send a fixed v2. - Felix
diff --git a/wpa_supplicant/mesh_mpm.c b/wpa_supplicant/mesh_mpm.c index 44859396c..4bd130fb8 100644 --- a/wpa_supplicant/mesh_mpm.c +++ b/wpa_supplicant/mesh_mpm.c @@ -685,11 +685,12 @@ static struct sta_info * mesh_mpm_add_peer(struct wpa_supplicant *wpa_s, } sta = ap_get_sta(data, addr); - if (!sta) { - sta = ap_sta_add(data, addr); - if (!sta) - return NULL; - } + if (sta) + return sta; + + sta = ap_sta_add(data, addr); + if (!sta) + return NULL; /* Set WMM by default since Mesh STAs are QoS STAs */ sta->flags |= WLAN_STA_WMM;
When wpa_supplicant receives another new peer event before the first one has been processed, it tries to add a station to the driver a second time (which fails) and then tears down the station entry until another event comes in. Fix this by only adding a station to the driver if it didn't exist already. Signed-off-by: Felix Fietkau <nbd@nbd.name> --- wpa_supplicant/mesh_mpm.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)