[PATCH] wpa_supplicant: multi_ap: only enable 4addr mode if not already enabled

[Janusz Dziedzic]

śr., 10 lut 2021 o 10:38 Jouni Malinen <j at 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 at gmail.com> napisał(a):
> > > pon., 8 lut 2021 o 18:30 Raphaël Mélotte <raphael.melotte at 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 at t2:~$ sudo ifconfig wlp1s0 up
> > > janusz at t2:~$ sudo iw wlp1s0 set 4addr on
> > > janusz at t2:~$ sudo iw wlp1s0 set 4addr on
> > > janusz at t2:~$ sudo brctl addbr br0
> > > janusz at t2:~$ sudo brctl addif br0 wlp1s0
> > > janusz at 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 at t2:~# brctl addif br0 wlp1s0
root at t2:~# iw wlp1s0 set 4addr on
root at t2:~# brctl show
bridge name bridge id STP enabled interfaces
br0 8000.68942328a725 no wlp1s0

root at t2:~# iw wlp1s0 set 4addr off
command failed: Device or resource busy (-16)
root at t2:~# iw wlp1s0 set 4addr on
root at 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