[PATCH] nl80211: correct netlink attributes of set_supp_port() for MLD

[Zong-Zhe Yang]

On Mon, Dec 23, 2024 at 6:06 AM Jouni Malinen <j at w1.fi> wrote:
>
> On Fri, Nov 22, 2024 at 04:21:43PM +0800, Zong-Zhe Yang wrote:
> > When MLD, wpa_driver_nl80211_set_supp_port() gets
> > `nl80211: Failed to set STA flag: -22 (Invalid argument)`
> > due to imprecise netlink attributes.
>
> Would you be able to share a debug log showing that? I was unable to
> reproduce that in my testing.
>

It can be reproduced by connecting a MLD AP with an assoc link which
link_id != 0. (with mac80211 driver)

> > For MLD, NL80211_ATTR_MLO_LINK_ID is required. Otherwise,
> > sta_link_apply_parameters() cannot get the target link.
>
> That sounds wrong since Authorization status is at MLD level and mixing
> this with link ID would seem to imply this is being done at a different
> level, but it is possible that there are some inconvenient constraints
> in how NL80211_CMD_SET_STATION might need to be used.
>

NL80211_CMD_SET_STATION
  nl80211_set_station
    rdev_change_station
      ieee80211_change_station <mac80211>
        sta_apply_parameters
          sta_link_apply_parameters

u32 link_id = params->link_id < 0 ? 0 : params->link_id;
[...]

Because @params->link_id is not filled, if assoc link id is not 0 either,
@link_id cannot map to any link in the following. Finally, an error returns.

This looked like an inconvenient constraint. And, I originally thought it's not
harmful to let wpa_supplicant pass the assoc link id at authorization status.
But, prehapes, there will be other suggestions for this problem.

> > Additionally, for MLD, NL80211_ATTR_MAC describes "link" address.
> > And, NL80211_ATTR_MLD_ADDR describes MLD address.
>
> Adding NL80211_ATTR_MLD_ADDR would seem reasonable, but that need for
> link specific information needs to be documented more clearly since this
> function is for an MLD level operation and specifying a link ID here is
> quite confusing.
>

For this problem, adding NL80211_ATTR_MLD_ADDR is not really required.
To make it less confusing, I can remove the filling of NL80211_ATTR_MLD_ADDR
in the next version.

> > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> > @@ -7730,6 +7732,16 @@ static int wpa_driver_nl80211_set_supp_port(void *priv, int authorized)
>
> > +     if (!mld)
> > +             goto send;
> > +
> > +     if (nla_put_u8(msg, NL80211_ATTR_MLO_LINK_ID, mld->assoc_link_id) ||
> > +         nla_put(msg, NL80211_ATTR_MLD_ADDR, ETH_ALEN, mld->ap_mld_addr)) {
> > +             nlmsg_free(msg);
> > +             return -ENOBUFS;
> > +     }
> > +
> > +send:
> >       ret = send_and_recv_cmd(drv, msg);
>
> That would look nicer if the unnecessary goto were removed:
>
>         if (mld &&
>             (nla_put_u8(...) ||
>              nla_put(...)) {
>                 ...
>         }
>

will do it like this.