| Message ID | 20210208172823.11536-1-raphael.melotte@mind.be |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wpa_supplicant: multi_ap: only enable 4addr mode if not already enabled | expand |
pon., 8 lut 2021 o 18:30 Raphaël Mélotte <raphael.melotte@mind.be> napisał(a): > > If 4addr mode is already enabled, the call to enable it a second time > may fail. If this happens when roaming, it leads to deauthentication. > > Signed-off-by: Raphaël Mélotte <raphael.melotte@mind.be> > --- > wpa_supplicant/events.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > index 199829bcf..7682dae76 100644 > --- a/wpa_supplicant/events.c > +++ b/wpa_supplicant/events.c > @@ -2627,11 +2627,13 @@ static void multi_ap_set_4addr_mode(struct wpa_supplicant *wpa_s) > goto fail; > } > > - if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > - wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > - goto fail; > + if (wpa_s->enabled_4addr_mode == 0) { > + if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > + wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > + goto fail; > + } > + wpa_s->enabled_4addr_mode = 1; > } > - wpa_s->enabled_4addr_mode = 1; > return; > Thanks for patch. Today set_4addr_mode(1) fail with EBUSY if netdev is already in the bridge. So, maybe we should fix it in cfg80211? janusz@t2:~$ sudo ifconfig wlp1s0 up janusz@t2:~$ sudo iw wlp1s0 set 4addr on janusz@t2:~$ sudo iw wlp1s0 set 4addr on janusz@t2:~$ sudo iw wlp1s0 set 4addr on janusz@t2:~$ sudo brctl addbr br0 janusz@t2:~$ sudo brctl addif br0 wlp1s0 janusz@t2:~$ sudo iw wlp1s0 set 4addr on command failed: Device or resource busy (-16) janusz@t2:~$ sudo iw wlp1s0 info Interface wlp1s0 ifindex 3 wdev 0x1 addr 68:94:23:28:a7:25 type managed wiphy 0 txpower 16.00 dBm multicast TXQ: qsz-byt qsz-pkt flows drops marks overlmt hashcol tx-bytes tx-packets 0 0 0 0 0 0 0 0 0 4addr: on BR Janusz
wt., 9 lut 2021 o 10:26 Janusz Dziedzic <janusz.dziedzic@gmail.com> napisał(a): > > pon., 8 lut 2021 o 18:30 Raphaël Mélotte <raphael.melotte@mind.be> napisał(a): > > > > If 4addr mode is already enabled, the call to enable it a second time > > may fail. If this happens when roaming, it leads to deauthentication. > > > > Signed-off-by: Raphaël Mélotte <raphael.melotte@mind.be> > > --- > > wpa_supplicant/events.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > > index 199829bcf..7682dae76 100644 > > --- a/wpa_supplicant/events.c > > +++ b/wpa_supplicant/events.c > > @@ -2627,11 +2627,13 @@ static void multi_ap_set_4addr_mode(struct wpa_supplicant *wpa_s) > > goto fail; > > } > > > > - if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > > - wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > > - goto fail; > > + if (wpa_s->enabled_4addr_mode == 0) { > > + if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > > + wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > > + goto fail; > > + } > > + wpa_s->enabled_4addr_mode = 1; > > } > > - wpa_s->enabled_4addr_mode = 1; > > return; > > > Thanks for patch. > > Today set_4addr_mode(1) fail with EBUSY if netdev is already in the bridge. > So, maybe we should fix it in cfg80211? > > janusz@t2:~$ sudo ifconfig wlp1s0 up > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > janusz@t2:~$ sudo brctl addbr br0 > janusz@t2:~$ sudo brctl addif br0 wlp1s0 > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > command failed: Device or resource busy (-16) > > janusz@t2:~$ sudo iw wlp1s0 info > Interface wlp1s0 > ifindex 3 > wdev 0x1 > addr 68:94:23:28:a7:25 > type managed > wiphy 0 > txpower 16.00 dBm > multicast TXQ: > qsz-byt qsz-pkt flows drops marks overlmt hashcol tx-bytes tx-packets > 0 0 0 0 0 0 0 0 0 > 4addr: on > Did fast patch (didn't check code deeply yet - just check set 4addr when busy issuey, seems works correctly): janusz@t1:~/work/linux$ git diff diff --git a/net/wireless/util.c b/net/wireless/util.c index b4acc805114b..fec068f48113 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -1027,6 +1027,7 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev, /* if it's part of a bridge, reject changing type to station/ibss */ if (netif_is_bridge_port(dev) && + (ntype != otype) && (ntype == NL80211_IFTYPE_ADHOC || ntype == NL80211_IFTYPE_STATION || ntype == NL80211_IFTYPE_P2P_CLIENT)) @Johannes Berg - could you check if this will be correct? For old kernels/cfg80211 maybe we should introduce wpa_drv_get_4addr_mode() and set only if required? BR Janusz
On Wed, Feb 10, 2021 at 10:07:42AM +0100, Janusz Dziedzic wrote: > wt., 9 lut 2021 o 10:26 Janusz Dziedzic <janusz.dziedzic@gmail.com> napisał(a): > > pon., 8 lut 2021 o 18:30 Raphaël Mélotte <raphael.melotte@mind.be> napisał(a): > > > If 4addr mode is already enabled, the call to enable it a second time > > > may fail. If this happens when roaming, it leads to deauthentication. What is the case where 4ddr mode would have already been enabled? Does this happen every time when there is roaming since multi_ap_set_4addr_mode() did not have this check before? Or are you considering cases where something external to wpa_supplicant is changing this parameter? > > > - if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > > > - wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > > > - goto fail; > > > + if (wpa_s->enabled_4addr_mode == 0) { > > > + if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > > > + wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > > > + goto fail; > > > + } > > > + wpa_s->enabled_4addr_mode = 1; > > > } > > > - wpa_s->enabled_4addr_mode = 1; This looks safer to me if it is clear that no external entity is messing up with the kernel parameter. > > Today set_4addr_mode(1) fail with EBUSY if netdev is already in the bridge. > > So, maybe we should fix it in cfg80211? > > janusz@t2:~$ sudo ifconfig wlp1s0 up > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > janusz@t2:~$ sudo brctl addbr br0 > > janusz@t2:~$ sudo brctl addif br0 wlp1s0 > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > command failed: Device or resource busy (-16) That specific EBUSY seems unnecessary for a no-change operation, but disabling 4addr mode in station interface that is in a bridge should still be prevented. > Did fast patch (didn't check code deeply yet - just check set 4addr > when busy issuey, seems works correctly): > diff --git a/net/wireless/util.c b/net/wireless/util.c > @@ -1027,6 +1027,7 @@ int cfg80211_change_iface(struct > cfg80211_registered_device *rdev, > > /* if it's part of a bridge, reject changing type to station/ibss */ > if (netif_is_bridge_port(dev) && > + (ntype != otype) && > (ntype == NL80211_IFTYPE_ADHOC || > ntype == NL80211_IFTYPE_STATION || > ntype == NL80211_IFTYPE_P2P_CLIENT)) How would that prevent disabling of 4addr mode when the netdev is in a bridge? > For old kernels/cfg80211 maybe we should introduce > wpa_drv_get_4addr_mode() and set only if required? Do we really need that complexity? Isn't the internal tracking with wpa_s->enabled_4addr_mode sufficient? In other words, simply apply this patch as-is..
śr., 10 lut 2021 o 10:38 Jouni Malinen <j@w1.fi> napisał(a): > > On Wed, Feb 10, 2021 at 10:07:42AM +0100, Janusz Dziedzic wrote: > > wt., 9 lut 2021 o 10:26 Janusz Dziedzic <janusz.dziedzic@gmail.com> napisał(a): > > > pon., 8 lut 2021 o 18:30 Raphaël Mélotte <raphael.melotte@mind.be> napisał(a): > > > > If 4addr mode is already enabled, the call to enable it a second time > > > > may fail. If this happens when roaming, it leads to deauthentication. > > What is the case where 4ddr mode would have already been enabled? Does > this happen every time when there is roaming since > multi_ap_set_4addr_mode() did not have this check before? Or are you > considering cases where something external to wpa_supplicant is changing > this parameter? > > > > > - if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > > > > - wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > > > > - goto fail; > > > > + if (wpa_s->enabled_4addr_mode == 0) { > > > > + if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { > > > > + wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); > > > > + goto fail; > > > > + } > > > > + wpa_s->enabled_4addr_mode = 1; > > > > } > > > > - wpa_s->enabled_4addr_mode = 1; > > This looks safer to me if it is clear that no external entity is messing > up with the kernel parameter. > > > > Today set_4addr_mode(1) fail with EBUSY if netdev is already in the bridge. > > > So, maybe we should fix it in cfg80211? > > > > janusz@t2:~$ sudo ifconfig wlp1s0 up > > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > > janusz@t2:~$ sudo brctl addbr br0 > > > janusz@t2:~$ sudo brctl addif br0 wlp1s0 > > > janusz@t2:~$ sudo iw wlp1s0 set 4addr on > > > command failed: Device or resource busy (-16) > > That specific EBUSY seems unnecessary for a no-change operation, but > disabling 4addr mode in station interface that is in a bridge should > still be prevented. > > > Did fast patch (didn't check code deeply yet - just check set 4addr > > when busy issuey, seems works correctly): > > > diff --git a/net/wireless/util.c b/net/wireless/util.c > > @@ -1027,6 +1027,7 @@ int cfg80211_change_iface(struct > > cfg80211_registered_device *rdev, > > > > /* if it's part of a bridge, reject changing type to station/ibss */ > > if (netif_is_bridge_port(dev) && > > + (ntype != otype) && > > (ntype == NL80211_IFTYPE_ADHOC || > > ntype == NL80211_IFTYPE_STATION || > > ntype == NL80211_IFTYPE_P2P_CLIENT)) > > How would that prevent disabling of 4addr mode when the netdev is in a > bridge? > This what I have with cfg80211 patch included: root@t2:~# brctl addif br0 wlp1s0 root@t2:~# iw wlp1s0 set 4addr on root@t2:~# brctl show bridge name bridge id STP enabled interfaces br0 8000.68942328a725 no wlp1s0 root@t2:~# iw wlp1s0 set 4addr off command failed: Device or resource busy (-16) root@t2:~# iw wlp1s0 set 4addr on root@t2:~# iw wlp1s0 set 4addr on Seems there is another check in cfg80211 that already handle this. > > For old kernels/cfg80211 maybe we should introduce > > wpa_drv_get_4addr_mode() and set only if required? > > Do we really need that complexity? Isn't the internal tracking with > wpa_s->enabled_4addr_mode sufficient? In other words, simply apply this > patch as-is.. > Sounds good, not sure about case where: - iw wlX set 4addr on - brctl add br0 wlX - supplicant (wlX) - wpa_cli -i wlX wps_pbc multi_ap=1 This test case failed for me (or simply restart supplicant after wlX in bridge) - because of 4addr on failed. Will check this with original patch. BR Janusz
On 2/10/21 10:38 AM, Jouni Malinen wrote: > On Wed, Feb 10, 2021 at 10:07:42AM +0100, Janusz Dziedzic wrote: >> wt., 9 lut 2021 o 10:26 Janusz Dziedzic <janusz.dziedzic@gmail.com> napisał(a): >>> pon., 8 lut 2021 o 18:30 Raphaël Mélotte <raphael.melotte@mind.be> napisał(a): >>>> If 4addr mode is already enabled, the call to enable it a second time >>>> may fail. If this happens when roaming, it leads to deauthentication. > > What is the case where 4ddr mode would have already been enabled? Does > this happen every time when there is roaming since > multi_ap_set_4addr_mode() did not have this check before? Or are you > considering cases where something external to wpa_supplicant is changing > this parameter? In my specific case I think 4addr mode is always enabled (by the supplicant) before roaming, because I'm trying to switch from one backhaul AP to another one (which means the supplicant enables 4addr mode when connecting to the first bAP). As Janusz pointed out, it seems the actual problem I'm facing is not the fact that 4addr mode is already enabled, but the fact that the interface is already part of the bridge (so my patch description is not accurate). No external entity changes this parameter in my case. Thanks, Raphaël
On Wed, 2021-02-10 at 12:37 +0100, Janusz Dziedzic wrote: > > > Did fast patch (didn't check code deeply yet - just check set 4addr > > > when busy issuey, seems works correctly): > > > diff --git a/net/wireless/util.c b/net/wireless/util.c > > > @@ -1027,6 +1027,7 @@ int cfg80211_change_iface(struct > > > cfg80211_registered_device *rdev, > > > > > > /* if it's part of a bridge, reject changing type to station/ibss */ > > > if (netif_is_bridge_port(dev) && > > > + (ntype != otype) && > > > (ntype == NL80211_IFTYPE_ADHOC || > > > ntype == NL80211_IFTYPE_STATION || > > > ntype == NL80211_IFTYPE_P2P_CLIENT)) > > > > How would that prevent disabling of 4addr mode when the netdev is in a > > bridge? > > > This what I have with cfg80211 patch included: > root@t2:~# brctl addif br0 wlp1s0 > root@t2:~# iw wlp1s0 set 4addr on > root@t2:~# brctl show > bridge name bridge id STP enabled interfaces > br0 8000.68942328a725 no wlp1s0 > > root@t2:~# iw wlp1s0 set 4addr off > command failed: Device or resource busy (-16) > root@t2:~# iw wlp1s0 set 4addr on > root@t2:~# iw wlp1s0 set 4addr on > > Seems there is another check in cfg80211 that already handle this. It's a different code path to set 4addr on/off in this function, so this is not really a surprise? Anyway, from a code POV this seems OK, though I'd prefer moving the bridge port check into the existing "ntype != otype" if there. johannes
On Mon, Feb 08, 2021 at 06:28:23PM +0100, Raphaël Mélotte wrote: > If 4addr mode is already enabled, the call to enable it a second time > may fail. If this happens when roaming, it leads to deauthentication. Based on the discussion, I addressed this issue differently by ignoring the nl80211 command failure when trying to enable the 4addr mode in case it is already enabled.
On 2/13/21 11:56 PM, Jouni Malinen wrote: > On Mon, Feb 08, 2021 at 06:28:23PM +0100, Raphaël Mélotte wrote: >> If 4addr mode is already enabled, the call to enable it a second time >> may fail. If this happens when roaming, it leads to deauthentication. > > Based on the discussion, I addressed this issue differently by ignoring > the nl80211 command failure when trying to enable the 4addr mode in case > it is already enabled. > I confirm the issue which lead me to send this patch is fixed with '58bbbb5981440da508164cbd423ca5dd7c1b98d8'. I was wondering if it was possible to use a bridge in the tests to reproduce the original issue with hwsim, but now I see that the test is already there :-) Thanks!
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 199829bcf..7682dae76 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -2627,11 +2627,13 @@ static void multi_ap_set_4addr_mode(struct wpa_supplicant *wpa_s) goto fail; } - if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { - wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); - goto fail; + if (wpa_s->enabled_4addr_mode == 0) { + if (wpa_drv_set_4addr_mode(wpa_s, 1) < 0) { + wpa_printf(MSG_ERROR, "Failed to set 4addr mode"); + goto fail; + } + wpa_s->enabled_4addr_mode = 1; } - wpa_s->enabled_4addr_mode = 1; return; fail:
If 4addr mode is already enabled, the call to enable it a second time may fail. If this happens when roaming, it leads to deauthentication. Signed-off-by: Raphaël Mélotte <raphael.melotte@mind.be> --- wpa_supplicant/events.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)