On 4/18/2023 5:11 PM, Johannes Berg wrote:
On Tue, 2023-04-18 at 17:02 +0800, Wen Gong wrote:
On 4/18/2023 4:48 PM, Johannes Berg wrote:
Hi,
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,
When asssocResp from AP is not success(such as status_code==1), the
ieee80211_link_data of 2nd link(sdata->link[link_id])
is NULL because ieee80211_assoc_success()->ieee80211_vif_update_links()
is not called.
Then struct cfg80211_rx_assoc_resp resp in cfg80211_rx_assoc_resp() and
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() only
call cfg80211_unhold_bss()
for the 1st link, then BSS of the 2nd link will never free because its
hold is awlays > 0 now.
Hah, yes, ouch.
I found it is not easy to refine it, so do you have any advise/idea?
for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
struct ieee80211_link_data *link;
link = sdata_dereference(sdata->link[link_id], sdata);
if (!link)
continue;
if (!assoc_data->link[link_id].bss)
continue;
resp.links[link_id].bss = assoc_data->link[link_id].bss;
resp.links[link_id].addr = link->conf->addr;
resp.links[link_id].status = assoc_data->link[link_id].status;
But is it really so hard? We only need link for link->conf->addr, and we
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?
Oh and there's the uapsd stuff but that only matters in the success case
anyway. In fact I'm not even sure the address matters in the
unsuccessful case but we have it pretty easily.
johannes
OK. So I guess you already have good way to refine it?
No, not really, was just thinking out loud here :)
johannes
Hi Johannes, if you have any patch to fix it, I can download and test.
