Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6a33a41b5aa6c98d95ff934fbb6a10dfe78e6e6e.camel@sipsolutions.net>
Subject: Re: parsing the multi-link element with STA profile wifi: mac80211:
 support MLO authentication/association with one link
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Wen Gong <quic_wgong@quicinc.com>,
        linux-wireless <linux-wireless@vger.kernel.org>
Cc:     ath11k@lists.infradead.org
Date:   Tue, 18 Apr 2023 11:11:59 +0200
In-Reply-To: <2842f57c-e383-fb9e-d994-95336cb65e0d@quicinc.com>
References: <48715509-62fd-2307-e38f-176234b482c1@quicinc.com>
         <f240b33d507daf898480b0a11eb27d4475e45164.camel@sipsolutions.net>
         <571d8ecf-2ca6-8b7b-6e15-be12c56e9f88@quicinc.com>
         <e40ee96ab2a8d9d079d3a06b7b1c615ab25a2403.camel@sipsolutions.net>
         <2842f57c-e383-fb9e-d994-95336cb65e0d@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-04-18 at 17:02 +0800, Wen Gong wrote:
> On 4/18/2023 4:48 PM, Johannes Berg wrote:
> > Hi,
> >=20
> > > My case is:
> > > When connect with 2 links AP, the cfg80211_hold_bss() is called by
> > > cfg80211_mlme_assoc()
> > > for each BSS of the 2 links,
> > >=20
> > > When asssocResp from AP is not success(such as status_code=3D=3D1), t=
he
> > > ieee80211_link_data of 2nd link(sdata->link[link_id])
> > > is NULL because ieee80211_assoc_success()->ieee80211_vif_update_links=
()
> > > is not called.
> > >=20
> > > Then struct cfg80211_rx_assoc_resp resp in cfg80211_rx_assoc_resp() a=
nd
> > > struct cfg80211_connect_resp_params cr in __cfg80211_connect_result()
> > > will only have the data of
> > > the 1st link, and finally cfg80211_connect_result_release_bsses() onl=
y
> > > call cfg80211_unhold_bss()
> > > for the 1st link, then BSS of the 2nd link will never free because it=
s
> > > hold is awlays > 0 now.
> > Hah, yes, ouch.
> >=20
> > > I found it is not easy to refine it, so do you have any advise/idea?
> > >=20
> > > for (link_id =3D 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++)=
 {
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct ieee80211_link_da=
ta *link;
> > >=20
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 link =3D sdata_dereferen=
ce(sdata->link[link_id], sdata);
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!link)
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
continue;
> > >=20
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!assoc_data->link[li=
nk_id].bss)
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
continue;
> > >=20
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resp.links[link_id].bss =
=3D assoc_data->link[link_id].bss;
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resp.links[link_id].addr=
 =3D link->conf->addr;
> > >   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 resp.links[link_id].stat=
us =3D assoc_data->link[link_id].status;
> > >=20
> > But is it really so hard? We only need link for link->conf->addr, and w=
e
> > can use assoc_data->link[link_id].addr for that instead, no? We need to
> > store those locally to avoid a use-after-free, but that's at most 15
> > addresses on the stack, which seems acceptable?
> >=20
> > Oh and there's the uapsd stuff but that only matters in the success cas=
e
> > anyway. In fact I'm not even sure the address matters in the
> > unsuccessful case but we have it pretty easily.
> >=20
> > johannes
> OK. So I guess you already have good way to refine it?
>=20

No, not really, was just thinking out loud here :)

johannes