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: <b5de662d-fd49-7b94-f617-89d4761c175b@quicinc.com>
Date:   Wed, 26 Jul 2023 14:30:42 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: parsing the multi-link element with STA profile wifi: mac80211:
 support MLO authentication/association with one link
Content-Language: en-US
From:   Wen Gong <quic_wgong@quicinc.com>
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless <linux-wireless@vger.kernel.org>
CC:     <ath11k@lists.infradead.org>
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>
 <6a33a41b5aa6c98d95ff934fbb6a10dfe78e6e6e.camel@sipsolutions.net>
 <2d3a6578-68c5-a0f3-b18b-bce2e67d3fdc@quicinc.com>
In-Reply-To: <2d3a6578-68c5-a0f3-b18b-bce2e67d3fdc@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Johannes,

I guess you have already fix it in some patch, right?

On 5/11/2023 7:01 PM, Wen Gong wrote:
> 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.
>