2024-02-08 08:56:45

by Alexey Berezhok

[permalink] [raw]
Subject: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

In 'brcmf_cfg80211_start_ap()', not assume that
NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
an explicit check instead. Compile tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Alexey Berezhok <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 736b2ada6..63f6e9436 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5272,7 +5272,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
settings->hidden_ssid);
if (err) {
bphy_err(drvr, "%s closednet error (%d)\n",
- settings->hidden_ssid ?
+ (settings->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE) ?
"enabled" : "disabled",
err);
goto exit;
--
2.39.3



2024-02-08 14:16:09

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On 2/8/2024 9:51 AM, Alexey Berezhok wrote:
> In 'brcmf_cfg80211_start_ap()', not assume that
> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
> an explicit check instead. Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Thanks, Alexey

Makes sense, but ....

> Signed-off-by: Alexey Berezhok <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 736b2ada6..63f6e9436 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5272,7 +5272,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
> settings->hidden_ssid);

settings->hidden_ssid has already been used above in following call:

err = brcmf_fil_iovar_int_set(ifp, "closednet",
settings->hidden_ssid);

So we pass the value as is to firmware using the same assumption, ie.
NL80211_HIDDEN_SSID_NOT_IN_USE. Is this not ABI thus very unlikely to
change?

@Johannes:
Actually not quite understanding the reason for having this setting in
nl80211. hidden_ssid means SSID element length is zero, right?

Regards,
Arend

> if (err) {
> bphy_err(drvr, "%s closednet error (%d)\n",
> - settings->hidden_ssid ?
> + (settings->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE) ?
> "enabled" : "disabled",
> err);
> goto exit;


Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-08 14:16:13

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On Thu, 2024-02-08 at 15:06 +0100, Arend van Spriel wrote:
>
> settings->hidden_ssid has already been used above in following call:
>
> err = brcmf_fil_iovar_int_set(ifp, "closednet",
> settings->hidden_ssid);
>
> So we pass the value as is to firmware using the same assumption, ie.
> NL80211_HIDDEN_SSID_NOT_IN_USE. Is this not ABI thus very unlikely to
> change?

The ABI won't change, that'd break all the users of nl80211 that use
this :-)

> @Johannes:
> Actually not quite understanding the reason for having this setting in
> nl80211. hidden_ssid means SSID element length is zero, right?
>

Well, there at least _were_ APs doing the correct SSID length but
setting all octets to zero ... Not sure that's still a thing though.

johannes

2024-02-08 15:57:07

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On February 8, 2024 3:15:25 PM Johannes Berg <[email protected]> wrote:

> On Thu, 2024-02-08 at 15:06 +0100, Arend van Spriel wrote:
>>
>> settings->hidden_ssid has already been used above in following call:
>>
>> err = brcmf_fil_iovar_int_set(ifp, "closednet",
>> settings->hidden_ssid);
>>
>> So we pass the value as is to firmware using the same assumption, ie.
>> NL80211_HIDDEN_SSID_NOT_IN_USE. Is this not ABI thus very unlikely to
>> change?
>
> The ABI won't change, that'd break all the users of nl80211 that use
> this :-)

Right. So basically the assumption that NL80211_HIDDEN_SSID_NOT_IN_USE is
and will be zero is a safe one.

>
>> @Johannes:
>> Actually not quite understanding the reason for having this setting in
>> nl80211. hidden_ssid means SSID element length is zero, right?
>
> Well, there at least _were_ APs doing the correct SSID length but
> setting all octets to zero ... Not sure that's still a thing though.

I now looked at the definition and see indeed two distinct flavours of
"hidden SSID". Interesting although I have never considered it a useful
feature.

Regards,
Arend




Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-12 15:39:04

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

Alexey Berezhok <[email protected]> wrote:

> In 'brcmf_cfg80211_start_ap()', not assume that
> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
> an explicit check instead. Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Alexey Berezhok <[email protected]>

Patch applied to wireless-next.git, thanks.

f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


2024-02-12 16:01:17

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On 2/12/2024 4:38 PM, Kalle Valo wrote:
> Alexey Berezhok <[email protected]> wrote:
>
>> In 'brcmf_cfg80211_start_ap()', not assume that
>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>> an explicit check instead. Compile tested only.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Alexey Berezhok <[email protected]>
>
> Patch applied to wireless-next.git, thanks.
>
> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

Alexey,

Can you do a follow-up patch addressing my comment? If not I will do it
myself.

Regards,
Arend
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/


Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-12 16:03:22

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

Arend van Spriel <[email protected]> writes:

> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>> Alexey Berezhok <[email protected]> wrote:
>>
>>> In 'brcmf_cfg80211_start_ap()', not assume that
>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>>> an explicit check instead. Compile tested only.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>
>>> Signed-off-by: Alexey Berezhok <[email protected]>
>> Patch applied to wireless-next.git, thanks.
>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value
>> to boolean
>
> Alexey,
>
> Can you do a follow-up patch addressing my comment? If not I will do
> it myself.

Sorry, was I not supposed to apply the patch? What did I miss?

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-02-12 16:45:54

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On February 12, 2024 5:03:14 PM Kalle Valo <[email protected]> wrote:

> Arend van Spriel <[email protected]> writes:
>
>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>> Alexey Berezhok <[email protected]> wrote:
>>>
>>>> In 'brcmf_cfg80211_start_ap()', not assume that
>>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>>>> an explicit check instead. Compile tested only.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>
>>>> Signed-off-by: Alexey Berezhok <[email protected]>
>>> Patch applied to wireless-next.git, thanks.
>>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value
>>> to boolean
>>
>> Alexey,
>>
>> Can you do a follow-up patch addressing my comment? If not I will do
>> it myself.
>
> Sorry, was I not supposed to apply the patch? What did I miss?

Nothing serious. settings->hidden_ssid value is used as-is to configure
firmware. I wanted Alexey to address that in a v2.

Regards,
Arend

>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches




Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-13 13:34:34

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

Arend Van Spriel <[email protected]> writes:

> On February 12, 2024 5:03:14 PM Kalle Valo <[email protected]> wrote:
>
>> Arend van Spriel <[email protected]> writes:
>>
>>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>>> Alexey Berezhok <[email protected]> wrote:
>>>>
>>>>> In 'brcmf_cfg80211_start_ap()', not assume that
>>>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>>>>> an explicit check instead. Compile tested only.
>>>>>
>>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>>
>>>>> Signed-off-by: Alexey Berezhok <[email protected]>
>>>> Patch applied to wireless-next.git, thanks.
>>>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value
>>>> to boolean
>>>
>>> Alexey,
>>>
>>> Can you do a follow-up patch addressing my comment? If not I will do
>>> it myself.
>>
>> Sorry, was I not supposed to apply the patch? What did I miss?
>
> Nothing serious. settings->hidden_ssid value is used as-is to
> configure firmware. I wanted Alexey to address that in a v2.

My bad, I misunderstood your intentions. Luckily this time it wasn't
serious.

BTW to make super clear to me I would prefer that you (Arend) use
Acked-by. It shows up in my script like the number '1' here:

*[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - - 2 5d Colin Ian Ki Under Review

So if I don't see your Acked-by then I will not even look at the patch :)

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-02-13 14:02:09

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On 2/13/2024 2:34 PM, Kalle Valo wrote:
> Arend Van Spriel <[email protected]> writes:
>
>> On February 12, 2024 5:03:14 PM Kalle Valo <[email protected]> wrote:
>>
>>> Arend van Spriel <[email protected]> writes:
>>>
>>>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>>>> Alexey Berezhok <[email protected]> wrote:
>>>>>
>>>>>> In 'brcmf_cfg80211_start_ap()', not assume that
>>>>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>>>>>> an explicit check instead. Compile tested only.
>>>>>>
>>>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>>>
>>>>>> Signed-off-by: Alexey Berezhok <[email protected]>
>>>>> Patch applied to wireless-next.git, thanks.
>>>>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value
>>>>> to boolean
>>>>
>>>> Alexey,
>>>>
>>>> Can you do a follow-up patch addressing my comment? If not I will do
>>>> it myself.
>>>
>>> Sorry, was I not supposed to apply the patch? What did I miss?
>>
>> Nothing serious. settings->hidden_ssid value is used as-is to
>> configure firmware. I wanted Alexey to address that in a v2.
>
> My bad, I misunderstood your intentions. Luckily this time it wasn't
> serious.
>
> BTW to make super clear to me I would prefer that you (Arend) use
> Acked-by. It shows up in my script like the number '1' here:
>
> *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - - 2 5d Colin Ian Ki Under Review
>
> So if I don't see your Acked-by then I will not even look at the patch :)

Sure. I tend to use Acked-by if things look sane a quick glance. If I
need to dig further I prefer to use Reviewed-by. If I have comments to
revise the patch I will refrain using them. Is that ok or you really
want it to be Acked-by?

Regards,
Arend


Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-13 15:46:40

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

Arend van Spriel <[email protected]> writes:

>> My bad, I misunderstood your intentions. Luckily this time it wasn't
>> serious.
>> BTW to make super clear to me I would prefer that you (Arend) use
>> Acked-by. It shows up in my script like the number '1' here:
>> *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - -
>> 2 5d Colin Ian Ki Under Review
>> So if I don't see your Acked-by then I will not even look at the
>> patch :)
>
> Sure. I tend to use Acked-by if things look sane a quick glance. If I
> need to dig further I prefer to use Reviewed-by. If I have comments to
> revise the patch I will refrain using them. Is that ok or you really
> want it to be Acked-by?

Ah, now I understand better. My understanding is that the maintainer of
the driver uses Acked-by and others use Reviewed-by. This says the same:

https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

And yes, I would prefer if you could use Acked-by always. It's up to
your judgement if you do just a peek or in-depth review :)

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2024-02-13 17:40:21

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On February 13, 2024 4:46:38 PM Kalle Valo <[email protected]> wrote:

> Arend van Spriel <[email protected]> writes:
>
>>> My bad, I misunderstood your intentions. Luckily this time it wasn't
>>> serious.
>>> BTW to make super clear to me I would prefer that you (Arend) use
>>> Acked-by. It shows up in my script like the number '1' here:
>>> *[ 4] [next] wifi: carl9170: Remove redundant assignment t... 1 - -
>>> 2 5d Colin Ian Ki Under Review
>>> So if I don't see your Acked-by then I will not even look at the
>>> patch :)
>>
>> Sure. I tend to use Acked-by if things look sane a quick glance. If I
>> need to dig further I prefer to use Reviewed-by. If I have comments to
>> revise the patch I will refrain using them. Is that ok or you really
>> want it to be Acked-by?
>
> Ah, now I understand better. My understanding is that the maintainer of
> the driver uses Acked-by and others use Reviewed-by. This says the same:
>
> https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
> And yes, I would prefer if you could use Acked-by always. It's up to
> your judgement if you do just a peek or in-depth review :)

No problem. I think I can do that.

Regards,
Arend



Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-14 10:28:45

by Alexey Berezhok

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

12.02.2024 19:00, Arend van Spriel wrote:
> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>> Alexey Berezhok <[email protected]> wrote:
>>
>>> In 'brcmf_cfg80211_start_ap()', not assume that
>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>>> an explicit check instead. Compile tested only.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>
>>> Signed-off-by: Alexey Berezhok <[email protected]>
>>
>> Patch applied to wireless-next.git, thanks.
>>
>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value
>> to boolean
>
> Alexey,
>
> Can you do a follow-up patch addressing my comment? If not I will do it
> myself.
>
> Regards,
> Arend
> --
> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

Hello, do you mean this kind of modification:
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 63f6e9436..d8f7bd5ce 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5269,7 +5269,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy,
struct net_device *ndev,
}

err = brcmf_fil_iovar_int_set(ifp, "closednet",
- settings->hidden_ssid);
+ (settings->hidden_ssid ==
NL80211_HIDDEN_SSID_NOT_IN_USE? 0 : 1));
if (err) {
bphy_err(drvr, "%s closednet error (%d)\n",
(settings->hidden_ssid !=
NL80211_HIDDEN_SSID_NOT_IN_USE) ?

If so, I will make additional patch.

Regards,
Alexey


2024-02-14 11:44:28

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

On February 14, 2024 11:28:28 AM Alexey Berezhok <[email protected]> wrote:

> 12.02.2024 19:00, Arend van Spriel wrote:
>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>> Alexey Berezhok <[email protected]> wrote:
>>>
>>>> In 'brcmf_cfg80211_start_ap()', not assume that
>>>> NL80211_HIDDEN_SSID_NOT_IN_USE is zero but prefer
>>>> an explicit check instead. Compile tested only.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>
>>>> Signed-off-by: Alexey Berezhok <[email protected]>
>>>
>>> Patch applied to wireless-next.git, thanks.
>>>
>>> f20073f50dfd wifi: brcmfmac: do not cast hidden SSID attribute value
>>> to boolean
>>
>> Alexey,
>>
>> Can you do a follow-up patch addressing my comment? If not I will do it
>> myself.
>>
>> Regards,
>> Arend
>> --
>> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>
> Hello, do you mean this kind of modification:
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 63f6e9436..d8f7bd5ce 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5269,7 +5269,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy,
> struct net_device *ndev,
> }
>
> err = brcmf_fil_iovar_int_set(ifp, "closednet",
> - settings->hidden_ssid);
> + (settings->hidden_ssid ==
> NL80211_HIDDEN_SSID_NOT_IN_USE? 0 : 1));
> if (err) {
> bphy_err(drvr, "%s closednet error (%d)\n",
> (settings->hidden_ssid !=
> NL80211_HIDDEN_SSID_NOT_IN_USE) ?
>
> If so, I will make additional patch.

That was indeed what I meant. Consider using local variable 'closednet' to
pass in function call and use for error message.

Regards,
Arend

> Regards,
> Alexey




Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-02-19 18:27:19

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] wifi: brcmfmac: do not cast hidden SSID attribute value to boolean

Thu, Feb 08, 2024 at 11:51:21AM +0300, Alexey Berezhok kirjoitti:

...

> - settings->hidden_ssid ?
> + (settings->hidden_ssid != NL80211_HIDDEN_SSID_NOT_IN_USE) ?
> "enabled" : "disabled",

Can somebody switch this to use str_enabled_disabled() from string_choices.h?
Ideally, add str_disabled_enabled() in that header (as macro with negation on
its argument) and use here as

str_disabled_enabled(settings->hidden_ssid == NL80211_HIDDEN_SSID_NOT_IN_USE)

--
With Best Regards,
Andy Shevchenko