Received-SPF: pass (google.com: domain of linux-wireless+bounces-4413-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <c9af14d1-2202-493c-9dae-7e3924e4e149@quicinc.com>
Date: Wed, 6 Mar 2024 12:44:29 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing
 bitmap obtained from AP
Content-Language: en-US
From: Kang Yang <quic_kangyang@quicinc.com>
To: Johannes Berg <johannes@sipsolutions.net>, <ath12k@lists.infradead.org>,
        Kalle Valo <kvalo@kernel.org>, Kalle Valo <quic_kvalo@quicinc.com>
CC: <linux-wireless@vger.kernel.org>
References: <20230928055022.9670-1-quic_kangyang@quicinc.com>
 <20230928055022.9670-2-quic_kangyang@quicinc.com>
 <08ff36664eb34bce8e7beb425233b1e8d4a4971c.camel@sipsolutions.net>
 <d85bd8f7-a6aa-6ff7-09a8-57005e4d0779@quicinc.com>
In-Reply-To: <d85bd8f7-a6aa-6ff7-09a8-57005e4d0779@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


Due to Johannes's refactor of Puncturing bitmap, this patchset can be 
abandoned now.


Will prepare a new patch about puncturing bitmap for ath12k.


On 10/19/2023 11:25 AM, Kang Yang wrote:
> 
> 
> On 10/18/2023 7:39 PM, Johannes Berg wrote:
>>>   static bool ieee80211_config_puncturing(struct ieee80211_link_data 
>>> *link,
>>>                       const struct ieee80211_eht_operation *eht_oper,
>>>                       u64 *changed)
>>>   {
>>> +    struct cfg80211_chan_def rx_chandef = link->conf->chandef;
>>>       u16 bitmap = 0, extracted;
>>> +    u8 bw = 0;
>>>       if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) &&
>>>           (eht_oper->params &
>>> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct 
>>> ieee80211_link_data *link,
>>>           const u8 *disable_subchannel_bitmap = info->optional;
>>>           bitmap = get_unaligned_le16(disable_subchannel_bitmap);
>>> +        bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH);
>>> +        rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw);
>>
>> But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_*
>> is a purely internal API, has nothing to do with the spec.
>>
>> All this might even be "accidentally correct", but it really isn't right
>> at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are
>> IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*.
>>
> 
> 
> 
> Oh, sorry that i didn't notice IEEE80211_EHT_OPER_CHAN_WIDTH_*, will 
> replace them.
> 
> 
>>
>>
>> More generally though, I don't even understand the change.
> 
> 
> During assoc, downgrade may happen in func ieee80211_config_bw(). In 
> this situation, the bandwidth in beacon and the bandwidth after 
> downgrade(chandef->width, maybe i should call it local bandwidth during 
> below context, will use this name in next version) during assoc will be 
> different.
> 
> The change is based on this point.
> 
> 
>>
>>> +        if (rx_chandef.width == NL80211_CHAN_WIDTH_80)
>>> +            rx_chandef.center_freq1 =
>>> +                ieee80211_channel_to_frequency(info->ccfs0,
>>> +                                   rx_chandef.chan->band);
>>> +        else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 ||
>>> +             rx_chandef.width == NL80211_CHAN_WIDTH_320)
>>> +            rx_chandef.center_freq1 =
>>> +                ieee80211_channel_to_frequency(info->ccfs1,
>>> +                                   rx_chandef.chan->band);
>>> +    }
>>> +
>>> +    if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>>> +                              &rx_chandef)) {
> 
> 
> Here i change you code 
> cfg80211_valid_disable_subchannel_bitmap(&bitmap,&link->conf->chandef) to
> cfg80211_valid_disable_subchannel_bitmap(&bitmap,&rx_chandef)
> 
> As described above, downgrade may happen before enter 
> ieee80211_config_puncturing(), so the bandwidth in link->conf->chandef 
> may be different with bandwidth in beacon.
> 
> Here, the bitmap you used is from eht_oper in beacon, but the bandwidth 
> you used is local bandwidth. They are not match. This is the first issue.
> 
> 
>>> +        link_info(link,
>>> +              "Got an invalid disable subchannel bitmap from AP %pM: 
>>> bitmap = 0x%x, bw = 0x%x. disconnect\n",
>>> +              link->u.mgd.bssid,
>>> +              bitmap,
>>> +              rx_chandef.width);
>>> +        return false;
>>>       }
>>>       extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
>> // I've filled in the context here in the patch
> 
> 
> Here is move the cfg80211_valid_disable_subchannel_bitmap() before the 
> ieee80211_extract_dis_subch_bmap(), cause i think check should done 
> before extract.
> 
> This is the second issue i said(perhaps not a issue).
> 
> 
> 
>>>                                                       
>>> &link->conf->chandef,
>>>                                                       bitmap);
>>>
>>>          /* accept if there are no changes */
>>>          if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
>>>              extracted == link->conf->eht_puncturing)
>>>                  return true;
>>
>> but ... ieee80211_extract_dis_subch_bmap actually already takes the
>> bandwidth from eht_oper into account!
> 
> Yes, the ieee80211_extract_dis_subch_bmap realy takes the bandwidth from 
> eht_oper into account, and the local_bw in this func is the local 
> bandwidth i discuss.
> 
> You already notice the difference between bandwidth from eht_oper and 
> local bandwidth in ieee80211_extract_dis_subch_bmap(). I think you 
> should also take it into account when you use 
> cfg80211_valid_disable_subchannel_bitmap(), right?
> 
> BTW, do you want to verify the bitmap from eht_oper, or the extracted 
> bitmap?
> 
> Anyway, whether you want to verify the bitmap from eht_oper or extracted 
> bitmap in cfg80211_valid_disable_subchannel_bitmap(), the bitmap and 
> bandwidth must correspond.
> 
> 
> 
>>> -    if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>>> -                              &link->conf->chandef)) {
>>
>> So are you saying that the real bug is that we're missing to update the
>> link->conf->chandef with the EHT operation from the assoc response?
>>
>> But you didn't fix that issue ... so not sure?
> 
> 
> I have described my patch with the comments above, maybe i should make 
> my commit log more coherent.
> 
> 
> Besides, this is you first version, i made some comments on Nov. 21, 
> 2022, 7:29 a.m. Maybe you already forget them.
> https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/
> 
> 
>>
>> johannes
>>
>