[PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing bitmap obtained from AP
Kang Yang
quic_kangyang at quicinc.com
Tue Mar 5 20:44:29 PST 2024
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
>>
>
More information about the ath12k
mailing list