On Mon, Nov 19, 2018 at 01:36:39PM +0100, Arnout Vandecappelle wrote:
> > +#ifdef CONFIG_MULTI_AP
> > + /**
> > + * set_multi_ap - Enable/disable Multi-AP functionality
> > + * @priv: Private driver interface data
> > + * @bridge_ifname: Name of the bridge interface
> > + * @val: 0 for Multi-AP disable, 1 for Multi-AP enable
> > + */
> > + int (*set_multi_ap)(void *priv, const char *bridge_ifname, int val);
>
> Wouldn't it be more logical to have a driver callback for setting 4addr rather
> than setting Multi-AP?
If this ends up setting just the 4-address mode, yes, it would seem to
make sense to call this with name matching the use.
> Also, it's not really setting the BSS to multi-AP support, it's setting it to
> backhaul BSS.
>
> By the way, as far as I understand, the multi-AP specification allows a BSS to
> be fronthaul and backhaul BSS at the same time - which means it should use 3addr
> for the fronthaul STAs and 4addr for the backhaul STAs. I don't know if it's
> even possible to program a radio like that...
That's interesting.. If such combination is not supported, it sounds
like it would be useful to make sure it can be used at least with
mac80211-based drivers and IEEE 802.11 frame generation in mac80211.
That said, the current mac80211 design should already allow different
combinations of stations connected to an AP mode interface when using
separate AP VLANs. However, this patch seems to be using set_multi_ap()
only in station mode (backhaul STA) and that is a clearer case since
nl80211 requires the NL80211_ATTR_4ADDR attribute to be used for that
and there is only a single AP in that connection so no need to address
different modes within a netdev.
> > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> > +#ifdef CONFIG_MULTI_AP
> > +static int nl80211_set_multi_ap(void *priv, const char *bridge_ifname, int val)
> > +{
> > + struct i802_bss *bss = priv;
> > + struct wpa_driver_nl80211_data *drv = bss->drv;
> > + struct nl_msg *msg;
> > + int ret = -ENOBUFS;
> > +
> > + msg = nl80211_cmd_msg(drv->first_bss, 0, NL80211_CMD_SET_INTERFACE);
> > + if (!msg || nla_put_u8(msg, NL80211_ATTR_4ADDR, val))
> > + goto fail;
> > +
> > + if (bridge_ifname[0] && bss->added_if_into_bridge && !val) {
> > + if (linux_br_del_if(drv->global->ioctl_sock,
> > + bridge_ifname, bss->ifname)) {
> > + wpa_printf(MSG_ERROR, "nl80211: Failed to remove the "
> > + "interface %s to a bridge %s",
> > + bss->ifname, bridge_ifname);
>
> I don't understand why this is needed. Actually, I don't see why you'd want to
> support switching the backhaul option on or off while you're associated. In my
> opinion, this option is as much part of the BSS configuration as the SSID. So
> actually, it would make more sense to me to just set the wds argument to
> nl80211_create_iface() as appropriate. Or is that too early because we haven't
> checked the Multi-AP IE yet?
Agreed on the not switching this type of things during an association
part.. I'm not familiar enough with the specific use case on how the
netdev got added to comment on that nl80211_create_iface() part. In
other words, can this be used with the main netdev (say, wlan0) as the
backhaul STA or is there an expectation that a new netdev/vif is created
when backhaul STA functionality is enabled?
> > diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
> > +#ifdef CONFIG_MULTI_AP
> > + if (os_strcmp(name, "multi_ap_enabled") == 0) {
> > + wpa_supplicant_deauthenticate(wpa_s, WLAN_REASON_DEAUTH_LEAVING);
> > + if (wpa_drv_set_multi_ap(wpa_s, wpa_s->bridge_ifname,
> > + ssid->multi_ap_enabled))
>
> As mentioned before: it would make more sense to me to do nothing here, and let
> it be handled on the next association.
I'd probably agree on this, but would need to see an example sequence on
how this is used to fully understand what needs to happen and how this
ssid->multi_ap_enabled gets set.
> > diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h
> > +#ifdef CONFIG_MULTI_AP
> > +static inline int wpa_drv_set_multi_ap(struct wpa_supplicant *wpa_s,
> > + const char *bridge_ifname, int val)
> > +{
> > + if (wpa_s->driver->set_multi_ap)
> > + return wpa_s->driver->set_multi_ap(wpa_s->drv_priv,
> > + bridge_ifname, val);
>
> bridge_ifname *must* be wpa_s->bridge_ifname, so I'd remove it as a parameter
> of wpa_drv_set_multi_ap and only pass it to the driver callback. That is,
> assuming it is needed at all of course.
If all the callers use wpa_s->bridge_ifname here, sure, no point in
passing that as an argument to the wrapper function. As discussed above,
this should likely be renamed, though, to make it clearer what the
driver operation is doing (just enabled 4-address mode in backhaul STA
case? or something beyond that?). hostapd does have code for dynamically
adding/removing netdevs to/from a bridge, but I'm not sure that's the
best approach to copy here with wpa_supplicant. High layer description
of the exact expected sequence would be helpful in understanding this
and determining whether the bridge interface should be covered here or
not.
> > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> > +static int wpa_validate_multi_ap_ie(struct wpa_supplicant *wpa_s,
> > + const u8 *ies, size_t ies_len)
> > +{
> > + struct ieee802_11_elems elems;
> > +
> > + if (ies == NULL)
> > + return -1;
> > +
> > + if (ieee802_11_parse_elems(ies, ies_len, &elems, 1) == ParseFailed)
> > + return -1;
> > +
> > + if (wpa_s->current_ssid->multi_ap_enabled && !elems.multi_ap) {
> > + wpa_printf(MSG_INFO, " AP doesn't support Multi-AP");
> > + return -1;
>
> Ideally, the check should be something like:
>
> (wpa_s->current_ssid->multi_ap_enabled &&
> !(multi_ap->sub_elem_val & MULTI_AP_BACKHAUL_BSS)) ||
> (!wpa_s->current_ssid->multi_ap_enabled &&
> !(multi_ap->sub_elem_val & MULTI_AP_FRONTHAUL_BSS))
>
> i.e., if we are configured as backhaul STA, then the AP should announce
> backhaul BSS; if we are configured as ordinary STA, the AP should announce
> fronthaul BSS.
Yes and that parsing IE part need to become aware of the Multi-AP IE
subelements as discussed in another email.
> > @@ -2343,6 +2363,20 @@ static int wpa_supplicant_event_associnfo(struct wpa_supplicant *wpa_s,
> > get_ie(data->assoc_info.resp_ies,
> > data->assoc_info.resp_ies_len, WLAN_EID_VHT_CAP))
> > wpa_s->ieee80211ac = 1;
> > +#ifdef CONFIG_MULTI_AP
> > + if (wpa_validate_multi_ap_ie(wpa_s, data->assoc_info.resp_ies,
> > + data->assoc_info.resp_ies_len)) {
>
> Doesn't this break the non-multi-ap case? If the IE is missing (which it
> currently is on all access points...), wpa_validate_multi_ap_ie will return -1.
That seems to be the case.
> So probably it should just return 0 if resp_ies == NULL.
But that would not be sufficient. None of this should happen if
wpa_s->current_ssid->multi_ap_enabled == 0.
> > diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c
> > +static struct wpabuf *multi_ap_build_assoc_req_ie(void)
>
> Now we have three implemntations of the construction of this vendor-specific
> element. Isn't there a way to factor them into a single function?
I sure hope so. There may use for having a separate wrapper for
allocating a wpabug with the IE, but surely this function could just
call wpa_add_multi_ap_info_ie() to generate that actual contents.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
