[EXTERNAL] Re: [PATCH] nl80211: Fix bridge option for non first-bss

[Mateusz Bajorski]

On Mon, Nov 28, 2022 at 10:15 AM Jouni Malinen <j at w1.fi> wrote:
>
> On Thu, Jul 21, 2022 at 03:01:19PM +0200, Mateusz Bajorski wrote:
> > Bridge interface was not added to ifidx list on non first-bss configuration.
> > This commit adapts solution from first bss where
> > bridge field is handled in i802_init function
> >
> > Issue occured when bridge interface already exist during adding bss.
> > i802_check_bridge covers only scenario when bridge interface does not exist.
>
> I was unable to reproduce any visible problem in that type of a
> scenario. wpa_driver_nl80211_if_add() did not add the bridge ifindex
> with add_ifidx(), but wpa_driver_nl80211_event_rtm_newlink() did this
> when processing the RTM_NEWLINK message indicating that the BSS netdev
> was added to a bridge.
>

Yes, a sample usecase is when the interface already exists and bridge
is based on Open vSwitch.
Open vSwitch bridge support is covered by ignoring failure to add
interface to bridge in i802_check_bridge.
Creating an interface device by system is useful when you want
to setup settings like ip or mtu once and keep it set across add and
remove from hostapd.

Preparing setup:
modprobe mac80211_hwsim
iw dev wlan0 interface add wlan0-1 type __ap
ovs-vsctl add-br testbr-ovs
ovs-vsctl add-port testbr-ovs wlan0

Adding bss:
hostapd_cli -i global raw ADD bss_config=phy0:/tmp/test-bss/bss1.conf
hostapd_cli -i global raw ADD bss_config=phy0:/tmp/test-bss/bss2.conf

logs from hostapd:
# ./hostapd/hostapd -dd -g /var/run/hostapd/global |grep -B1 "if_indices"
nl80211: Add own interface ifindex 13 (ifidx_reason 8)
nl80211: if_indices[16]: 13(8)
nl80211: Add own interface ifindex 8 (ifidx_reason -1)
nl80211: if_indices[16]: 13(8) 8(-1)
--
nl80211: Add own interface ifindex 16 (ifidx_reason -1)
nl80211: if_indices[16]: 13(8) 8(-1) 16(-1)

bss adding reversed
hostapd_cli -i global raw ADD bss_config=phy0:/tmp/test-bss/bss2.conf
hostapd_cli -i global raw ADD bss_config=phy0:/tmp/test-bss/bss1.conf

logs from hostapd:
# ./hostapd/hostapd -dd -g /var/run/hostapd/global |grep -B1 "if_indices"
nl80211: Add own interface ifindex 16 (ifidx_reason -1)
nl80211: if_indices[16]: 16(-1)
--
nl80211: Add own interface ifindex 8 (ifidx_reason -1)
nl80211: if_indices[16]: 16(-1) 8(-1)

interface indexes
wlan0:  8
wlan0-1: 16
testbr-ovs: 13

file bss1.conf:
driver=nl80211
hw_mode=g
channel=1
ieee80211n=1
interface=wlan0
bridge=testbr-ovs
ctrl_interface=/var/run/hostapd
ssid=bss-1

file bss2.conf:
driver=nl80211
hw_mode=g
channel=1
ieee80211n=1
interface=wlan0-1
ctrl_interface=/var/run/hostapd
ssid=bss-2

After applying a change the if_indices list contains all interfaces
regardless of order of inserting configs.

> Is there a sequence where the RTM_NEWLINK mechanism does not address
> this? That would seem to require the netdev of the second BSS to already
> exist and be in the bridge, but even when trying that kind of a
> sequence, I saw the RTM_NEWLINK event taking care of this when hostapd
> started operating the interface.
>
> > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> > @@ -7998,19 +7999,28 @@ static int wpa_driver_nl80211_if_add(void *priv, enum wpa_driver_if_type type,
>
> > +             if (bridge) {
> > +                     br_ifindex = if_nametoindex(bridge);
> > +                     if (br_ifindex)
> > +                             add_ifidx(drv, br_ifindex, ifidx);
> > +
> > +                     if (i802_check_bridge(drv, new_bss, bridge, ifname) < 0) {
> > +                             wpa_printf(MSG_ERROR, "nl80211: Failed to add the new "
> > +                                        "interface %s to a bridge %s",
> > +                                        ifname, bridge);
> > +                             if (br_ifindex)
> > +                                     del_ifidx(drv, br_ifindex, ifidx);
>
> Is that del_ifidx() correct thing to do? What if there had been some
> other BSSs that were already in that bridge? Shouldn't they remain
> functional even if something goes wrong with this new BSS addition?

if_indices list contains if index and ifidx_reason, in case of bridge this.
As br_ifindex is added on the beginning it should be cleaned on error
path just like added flag.
It should not create duplicate entries as the add_ifidx function is
checking for duplicates.

>
> >               if (linux_set_iface_flags(drv->global->ioctl_sock, ifname, 1))
> >               {
> > +                     if (br_ifindex)
> > +                             del_ifidx(drv, br_ifindex, ifidx);
>
> Same here..
>
> --
> Jouni Malinen                                            PGP id EFC895FA



-- 
Best regards,
Mateusz Bajorski