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

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

[Alexey Berezhok]

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 <a@bayrepo.ru>
---
 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

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

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

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 <a@bayrepo.ru>
> ---
>   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;

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Johannes Berg]

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

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

On February 8, 2024 3:15:25 PM Johannes Berg <johannes@sipsolutions.net> 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




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Kalle Valo]

Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>

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/20240208085121.2430-1-a@bayrepo.ru/

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

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

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

On 2/12/2024 4:38 PM, Kalle Valo wrote:
> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
> 
> 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/20240208085121.2430-1-a@bayrepo.ru/

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
>> 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

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]

On February 12, 2024 5:03:14 PM Kalle Valo <kvalo@kernel.org> wrote:

> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>
>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
>>> 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




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On February 12, 2024 5:03:14 PM Kalle Valo <kvalo@kernel.org> wrote:
>
>> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>>
>>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>>> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
>>>> 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

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

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On 2/13/2024 2:34 PM, Kalle Valo wrote:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> 
>> On February 12, 2024 5:03:14 PM Kalle Valo <kvalo@kernel.org> wrote:
>>
>>> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>>>
>>>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>>>> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
>>>>> 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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> 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

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On February 13, 2024 4:46:38 PM Kalle Valo <kvalo@kernel.org> wrote:

> Arend van Spriel <arend.vanspriel@broadcom.com> 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



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Alexey Berezhok]

12.02.2024 19:00, Arend van Spriel wrote:
> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
>>
>> 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/20240208085121.2430-1-a@bayrepo.ru/

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

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]

On February 14, 2024 11:28:28 AM Alexey Berezhok <a@bayrepo.ru> wrote:

> 12.02.2024 19:00, Arend van Spriel wrote:
>> On 2/12/2024 4:38 PM, Kalle Valo wrote:
>>> Alexey Berezhok <a@bayrepo.ru> 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 <a@bayrepo.ru>
>>>
>>> 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/20240208085121.2430-1-a@bayrepo.ru/
>
> 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




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[andy.shevchenko]

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