| Message ID | 20240119151201.8670-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [wireless,v2] nl80211/cfg80211: add nla_policy for S1G band | expand |
On Fri, 2024-01-19 at 23:12 +0800, Lin Ma wrote: > > V1 -> V2: change net-next to wireless as suggested For the record, I don't think anyone is using scheduled scan with S1G or even _could_ be doing that (is there even a driver other than hwsim?), so I don't think I'll apply this for wireless. No need to resend (again) though :) johannes
On 1/19/2024 7:12 AM, Lin Ma wrote: > Our detector has identified another case of an incomplete policy. > Specifically, the commit df78a0c0b67d ("nl80211: S1G band and channel > definitions") introduced the NL80211_BAND_S1GHZ attribute to > nl80211_band, but it neglected to update the > nl80211_match_band_rssi_policy accordingly. > > Similar commits that add new band types, such as the initial > commit 1e1b11b6a111 ("nl80211/cfg80211: Specify band specific min RSSI > thresholds with sched scan"), the commit e548a1c36b11 ("cfg80211: add 6GHz > in code handling array with NUM_NL80211_BANDS entries"), and the > commit 63fa04266629 ("nl80211: Add LC placeholder band definition to > nl80211_band"), all require updates to the policy. > Failure to do so could result in accessing an attribute of unexpected > length in the function nl80211_parse_sched_scan_per_band_rssi. > > To resolve this issue, this commit adds the policy for the > NL80211_BAND_S1GHZ attribute. > > Fixes: df78a0c0b67d ("nl80211: S1G band and channel definitions") > Signed-off-by: Lin Ma <linma@zju.edu.cn> > --- > V1 -> V2: change net-next to wireless as suggested > > net/wireless/nl80211.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index 60877b532993..980300621a60 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -911,6 +911,7 @@ nl80211_match_band_rssi_policy[NUM_NL80211_BANDS] = { > [NL80211_BAND_5GHZ] = { .type = NLA_S32 }, > [NL80211_BAND_6GHZ] = { .type = NLA_S32 }, > [NL80211_BAND_60GHZ] = { .type = NLA_S32 }, > + [NL80211_BAND_S1GHZ] = { .type = NLA_S32 }, > [NL80211_BAND_LC] = { .type = NLA_S32 }, > }; > something is really suspicious since the NL80211_BAND_* enums are *value* enums, not attribute ID enums, and hence they should never be used in an nla_policy. what is actually using these as attribute IDs, noting that NL80211_BAND_2GHZ == 0 and hence cannot be used as an attribute ID seems the logic that introduced this policy needs to be revisited.
On Fri, 2024-01-19 at 15:47 -0800, Jeff Johnson wrote: > > --- a/net/wireless/nl80211.c > > +++ b/net/wireless/nl80211.c > > @@ -911,6 +911,7 @@ nl80211_match_band_rssi_policy[NUM_NL80211_BANDS] = { > > [NL80211_BAND_5GHZ] = { .type = NLA_S32 }, > > [NL80211_BAND_6GHZ] = { .type = NLA_S32 }, > > [NL80211_BAND_60GHZ] = { .type = NLA_S32 }, > > + [NL80211_BAND_S1GHZ] = { .type = NLA_S32 }, > > [NL80211_BAND_LC] = { .type = NLA_S32 }, > > }; > > > something is really suspicious since the NL80211_BAND_* enums are > *value* enums, not attribute ID enums, and hence they should never be > used in an nla_policy. Yeah, that's what it looks like first, but then they do get used anyway... > what is actually using these as attribute IDs, noting that > NL80211_BAND_2GHZ == 0 and hence cannot be used as an attribute ID Ohh. Good catch! > seems the logic that introduced this policy needs to be revisited. > Let's just remove it? commit 1e1b11b6a1111cd9e8af1fd6ccda270a9fa3eacf Author: vamsi krishna <vamsin@codeaurora.org> Date: Fri Feb 1 18:34:51 2019 +0530 nl80211/cfg80211: Specify band specific min RSSI thresholds with sched scan As far as I can tell nothing is using that in the first place ... Certainly not in the kernel, nor wpa_s, nor anything else I could find really ... We can't completely revert it since we need the attribute number to stay allocated, but that's all we cannot remove. johannes
On 1/20/2024 12:27 PM, Johannes Berg wrote: > On Fri, 2024-01-19 at 15:47 -0800, Jeff Johnson wrote: >>> --- a/net/wireless/nl80211.c >>> +++ b/net/wireless/nl80211.c >>> @@ -911,6 +911,7 @@ nl80211_match_band_rssi_policy[NUM_NL80211_BANDS] = { >>> [NL80211_BAND_5GHZ] = { .type = NLA_S32 }, >>> [NL80211_BAND_6GHZ] = { .type = NLA_S32 }, >>> [NL80211_BAND_60GHZ] = { .type = NLA_S32 }, >>> + [NL80211_BAND_S1GHZ] = { .type = NLA_S32 }, >>> [NL80211_BAND_LC] = { .type = NLA_S32 }, >>> }; >>> >> something is really suspicious since the NL80211_BAND_* enums are >> *value* enums, not attribute ID enums, and hence they should never be >> used in an nla_policy. > > Yeah, that's what it looks like first, but then they do get used > anyway... > >> what is actually using these as attribute IDs, noting that >> NL80211_BAND_2GHZ == 0 and hence cannot be used as an attribute ID > > Ohh. Good catch! > >> seems the logic that introduced this policy needs to be revisited. >> > > Let's just remove it? > > commit 1e1b11b6a1111cd9e8af1fd6ccda270a9fa3eacf > Author: vamsi krishna <vamsin@codeaurora.org> > Date: Fri Feb 1 18:34:51 2019 +0530 > > nl80211/cfg80211: Specify band specific min RSSI thresholds with sched scan > > > As far as I can tell nothing is using that in the first place ... > Certainly not in the kernel, nor wpa_s, nor anything else I could find > really ... > > We can't completely revert it since we need the attribute number to stay > allocated, but that's all we cannot remove. I'm investigating this and will report back.
On 1/22/2024 10:33 AM, Jeff Johnson wrote: > On 1/20/2024 12:27 PM, Johannes Berg wrote: >> On Fri, 2024-01-19 at 15:47 -0800, Jeff Johnson wrote: >>>> --- a/net/wireless/nl80211.c >>>> +++ b/net/wireless/nl80211.c >>>> @@ -911,6 +911,7 @@ nl80211_match_band_rssi_policy[NUM_NL80211_BANDS] = { >>>> [NL80211_BAND_5GHZ] = { .type = NLA_S32 }, >>>> [NL80211_BAND_6GHZ] = { .type = NLA_S32 }, >>>> [NL80211_BAND_60GHZ] = { .type = NLA_S32 }, >>>> + [NL80211_BAND_S1GHZ] = { .type = NLA_S32 }, >>>> [NL80211_BAND_LC] = { .type = NLA_S32 }, >>>> }; >>>> >>> something is really suspicious since the NL80211_BAND_* enums are >>> *value* enums, not attribute ID enums, and hence they should never be >>> used in an nla_policy. >> >> Yeah, that's what it looks like first, but then they do get used >> anyway... >> >>> what is actually using these as attribute IDs, noting that >>> NL80211_BAND_2GHZ == 0 and hence cannot be used as an attribute ID >> >> Ohh. Good catch! >> >>> seems the logic that introduced this policy needs to be revisited. >>> >> >> Let's just remove it? >> >> commit 1e1b11b6a1111cd9e8af1fd6ccda270a9fa3eacf >> Author: vamsi krishna <vamsin@codeaurora.org> >> Date: Fri Feb 1 18:34:51 2019 +0530 >> >> nl80211/cfg80211: Specify band specific min RSSI thresholds with sched scan >> >> >> As far as I can tell nothing is using that in the first place ... >> Certainly not in the kernel, nor wpa_s, nor anything else I could find >> really ... >> >> We can't completely revert it since we need the attribute number to stay >> allocated, but that's all we cannot remove. > > I'm investigating this and will report back. OK, I have investigated this and based upon the investigation this can be removed (except for keeping the now obsolete uapi bits). This was done in preparation for supporting a new Android interface in the out-of-tree Android driver, but that interface was subsequently withdrawn by Google. Johannes, do you want to handle this? Or should I? /jeff
On Thu, 2024-01-25 at 10:32 -0800, Jeff Johnson wrote: > > OK, I have investigated this and based upon the investigation this can > be removed (except for keeping the now obsolete uapi bits). This was > done in preparation for supporting a new Android interface in the > out-of-tree Android driver, but that interface was subsequently > withdrawn by Google. > > Johannes, do you want to handle this? Or should I? Would be great if you could send a patch, thanks! johannes
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 60877b532993..980300621a60 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -911,6 +911,7 @@ nl80211_match_band_rssi_policy[NUM_NL80211_BANDS] = { [NL80211_BAND_5GHZ] = { .type = NLA_S32 }, [NL80211_BAND_6GHZ] = { .type = NLA_S32 }, [NL80211_BAND_60GHZ] = { .type = NLA_S32 }, + [NL80211_BAND_S1GHZ] = { .type = NLA_S32 }, [NL80211_BAND_LC] = { .type = NLA_S32 }, };
Our detector has identified another case of an incomplete policy. Specifically, the commit df78a0c0b67d ("nl80211: S1G band and channel definitions") introduced the NL80211_BAND_S1GHZ attribute to nl80211_band, but it neglected to update the nl80211_match_band_rssi_policy accordingly. Similar commits that add new band types, such as the initial commit 1e1b11b6a111 ("nl80211/cfg80211: Specify band specific min RSSI thresholds with sched scan"), the commit e548a1c36b11 ("cfg80211: add 6GHz in code handling array with NUM_NL80211_BANDS entries"), and the commit 63fa04266629 ("nl80211: Add LC placeholder band definition to nl80211_band"), all require updates to the policy. Failure to do so could result in accessing an attribute of unexpected length in the function nl80211_parse_sched_scan_per_band_rssi. To resolve this issue, this commit adds the policy for the NL80211_BAND_S1GHZ attribute. Fixes: df78a0c0b67d ("nl80211: S1G band and channel definitions") Signed-off-by: Lin Ma <linma@zju.edu.cn> --- V1 -> V2: change net-next to wireless as suggested net/wireless/nl80211.c | 1 + 1 file changed, 1 insertion(+)