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

[Jouni Malinen]

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?

> 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..

-- 
Jouni Malinen                                            PGP id EFC895FA