Hi Johannes,
May I know the status for below work which is written in the patch below?
I think it is needed in
ieee80211_assoc_config_link()/ieee80211_assoc_success(), right?
Extending that to multiple links will require
* work on parsing the multi-link element with STA profile properly,
including element fragmentation;
https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?id=81151ce462e5
On Wed, 2022-09-28 at 23:33 +0800, Wen Gong wrote:
> Hi Johannes,
>
> May I know the status for below work which is written in the patch below?
> I think it is needed in
> ieee80211_assoc_config_link()/ieee80211_assoc_success(), right?
>
It passed the plugfest last week ;-)
Yes, I need to get this posted ... but now I got another urgent thing to
look at, so it'll be some time.
johannes
On 9/29/2022 3:28 AM, Johannes Berg wrote:
> On Wed, 2022-09-28 at 23:33 +0800, Wen Gong wrote:
>> Hi Johannes,
>>
>> May I know the status for below work which is written in the patch below?
>> I think it is needed in
>> ieee80211_assoc_config_link()/ieee80211_assoc_success(), right?
>>
> It passed the plugfest last week ;-)
it is good for that :) -)
> Yes, I need to get this posted ... but now I got another urgent thing to
> look at, so it'll be some time.
>
> johannes
On 9/29/2022 3:28 AM, Johannes Berg wrote:
> On Wed, 2022-09-28 at 23:33 +0800, Wen Gong wrote:
...
Hi Johannes,
The change below which added in ieee80211_rx_mgmt_assoc_resp() by
patch "wifi: mac80211: support MLO authentication/association with one
link (commit 81151ce462e5)"
maybe need refine to meet 2 links.
I hit issue that the BSS of the 2 link will always hold and never free.
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.
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;
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
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?
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
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.
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.
>
On Wed, 2023-07-26 at 14:30 +0800, Wen Gong wrote:
> Hi Johannes,
>
> I guess you have already fix it in some patch, right?
>
No, I don't. We've not seen this, it's not been a priority.
<rant>
Look, I told you how I think you can fix it _literally_ three months
ago. I was on vacation for pretty much exactly half of that time. And
during that time you ask if _I_ have a fix? What have you been doing for
the past three months that prevented you from fixing it?
Upstream is supposed to be a collaborative effort, not a place where
I/we do the work and you keep asking me to make the changes you need for
you!
Sure, it there's a bug, and we should probably be fixing it too, but if
that's not high on our list right now because we don't experience it,
then I don't see why you don't just send a patch!
I even get that you might not know whether something is a bug, and when
we determine that it is, how to fix it. But we cleared that up three
months ago, and all you've done in this thread is ask whether I have a
fix for you. Why?
johannes