[PATCH v4] Set ssid when authenticating

[PATCH v4] Set ssid when authenticating

[Marc Bornand]

changes since v3:
- add missing NULL check
- add missing break

changes since v2:
- The code was tottaly rewritten based on the disscution of the
  v2 patch.
- the ssid is set in __cfg80211_connect_result() and only if the ssid is
  not already set.
- Do not add an other ssid reset path since it is already done in
  __cfg80211_disconnected()

When a connexion was established without going through
NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
Now we set it in __cfg80211_connect_result() when it is not already set.

Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
Cc: linux-wireless@vger.kernel.org
Cc: stable@vger.kernel.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216711
Signed-off-by: Marc Bornand <dev.mbornand@systemb.ch>
---
 net/wireless/sme.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 4b5b6ee0fe01..b552d6c20a26 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -723,6 +723,7 @@ void __cfg80211_connect_result(struct net_device *dev,
 			       bool wextev)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	const struct element *ssid;
 	const struct element *country_elem = NULL;
 	const u8 *country_data;
 	u8 country_datalen;
@@ -883,6 +884,22 @@ void __cfg80211_connect_result(struct net_device *dev,
 				   country_data, country_datalen);
 	kfree(country_data);

+	if (wdev->u.client.ssid_len == 0) {
+		rcu_read_lock();
+		for_each_valid_link(cr, link) {
+			ssid = ieee80211_bss_get_elem(cr->links[link].bss,
+						      WLAN_EID_SSID);
+
+			if (!ssid || ssid->datalen == 0)
+				continue;
+
+			memcpy(wdev->u.client.ssid, ssid->data, ssid->datalen);
+			wdev->u.client.ssid_len = ssid->datalen;
+			break;
+		}
+		rcu_read_unlock();
+	}
+
 	return;
 out:
 	for_each_valid_link(cr, link)
--
2.39.1

Re: [PATCH v4] Set ssid when authenticating

[Denis Kirjanov]

On 2/14/23 16:20, Marc Bornand wrote:
> changes since v3:
> - add missing NULL check
> - add missing break
> 
> changes since v2:
> - The code was tottaly rewritten based on the disscution of the
>   v2 patch.
> - the ssid is set in __cfg80211_connect_result() and only if the ssid is
>   not already set.
> - Do not add an other ssid reset path since it is already done in
>   __cfg80211_disconnected()
> 
> When a connexion was established without going through
> NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> Now we set it in __cfg80211_connect_result() when it is not already set.

A couple of small nits

> 
> Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
> Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
Please add a test description to the fixes tag

> Cc: linux-wireless@vger.kernel.org
> Cc: stable@vger.kernel.org
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216711
> Signed-off-by: Marc Bornand <dev.mbornand@systemb.ch>
> ---
>  net/wireless/sme.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 4b5b6ee0fe01..b552d6c20a26 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -723,6 +723,7 @@ void __cfg80211_connect_result(struct net_device *dev,
>  			       bool wextev)
>  {
>  	struct wireless_dev *wdev = dev->ieee80211_ptr;
> +	const struct element *ssid;

Please use reverse xmas tree

>  	const struct element *country_elem = NULL;
>  	const u8 *country_data;
>  	u8 country_datalen;
> @@ -883,6 +884,22 @@ void __cfg80211_connect_result(struct net_device *dev,
>  				   country_data, country_datalen);
>  	kfree(country_data);
> 
> +	if (wdev->u.client.ssid_len == 0) {
> +		rcu_read_lock();
> +		for_each_valid_link(cr, link) {
> +			ssid = ieee80211_bss_get_elem(cr->links[link].bss,
> +						      WLAN_EID_SSID);
> +
> +			if (!ssid || ssid->datalen == 0)
> +				continue;
> +
> +			memcpy(wdev->u.client.ssid, ssid->data, ssid->datalen);
> +			wdev->u.client.ssid_len = ssid->datalen;
> +			break;
> +		}
> +		rcu_read_unlock();
> +	}
> +
>  	return;
>  out:
>  	for_each_valid_link(cr, link)
> --
> 2.39.1
> 
> 

Re: [PATCH v4] Set ssid when authenticating

[Marc Bornand]

On Tue, Feb 14, 2023 at 04:27:27PM +0300, Denis Kirjanov wrote:
>
>
> On 2/14/23 16:20, Marc Bornand wrote:
> > changes since v3:
> > - add missing NULL check
> > - add missing break
> >
> > changes since v2:
> > - The code was tottaly rewritten based on the disscution of the
> >   v2 patch.
> > - the ssid is set in __cfg80211_connect_result() and only if the ssid is
> >   not already set.
> > - Do not add an other ssid reset path since it is already done in
> >   __cfg80211_disconnected()
> >
> > When a connexion was established without going through
> > NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> > Now we set it in __cfg80211_connect_result() when it is not already set.
>
> A couple of small nits
>
> >
> > Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
> > Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
> Please add a test description to the fixes tag

What do you mean by "test description" ?

Marc

Re: [PATCH v4] Set ssid when authenticating

[Conor Dooley]

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

On Tue, Feb 14, 2023 at 10:01:35PM +0000, Marc Bornand wrote:
> On Tue, Feb 14, 2023 at 04:27:27PM +0300, Denis Kirjanov wrote:

> > On 2/14/23 16:20, Marc Bornand wrote:
> > > changes since v3:
> > > - add missing NULL check
> > > - add missing break
> > >
> > > changes since v2:
> > > - The code was tottaly rewritten based on the disscution of the
> > >   v2 patch.
> > > - the ssid is set in __cfg80211_connect_result() and only if the ssid is
> > >   not already set.
> > > - Do not add an other ssid reset path since it is already done in
> > >   __cfg80211_disconnected()
> > >
> > > When a connexion was established without going through
> > > NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> > > Now we set it in __cfg80211_connect_result() when it is not already set.
> >
> > A couple of small nits
> >
> > >
> > > Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
> > > Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
> > Please add a test description to the fixes tag
> 
> What do you mean by "test description" ?

s/s/x/ ;)

Fixes: 7b0a0e3c3a88 ("wifi: cfg80211: do some rework towards MLO link APIs")

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4] Set ssid when authenticating

[Kalle Valo]

Marc Bornand <dev.mbornand@systemb.ch> writes:

> changes since v3:
> - add missing NULL check
> - add missing break
>
> changes since v2:
> - The code was tottaly rewritten based on the disscution of the
>   v2 patch.
> - the ssid is set in __cfg80211_connect_result() and only if the ssid is
>   not already set.
> - Do not add an other ssid reset path since it is already done in
>   __cfg80211_disconnected()
>
> When a connexion was established without going through
> NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> Now we set it in __cfg80211_connect_result() when it is not already set.
>
> Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
> Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
> Cc: linux-wireless@vger.kernel.org
> Cc: stable@vger.kernel.org
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216711
> Signed-off-by: Marc Bornand <dev.mbornand@systemb.ch>
> ---
>  net/wireless/sme.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

The change log ("changes since v3" etc) should be after "---" line and
the title should start with "wifi: cfg80211:". Please read the wiki link
below.

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

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

Re: [PATCH v4] Set ssid when authenticating

[Marc Bornand]

On Wed, Feb 15, 2023 at 07:35:09AM +0200, Kalle Valo wrote:
> Marc Bornand <dev.mbornand@systemb.ch> writes:
>
> > changes since v3:
> > - add missing NULL check
> > - add missing break
> >
> > changes since v2:
> > - The code was tottaly rewritten based on the disscution of the
> >   v2 patch.
> > - the ssid is set in __cfg80211_connect_result() and only if the ssid is
> >   not already set.
> > - Do not add an other ssid reset path since it is already done in
> >   __cfg80211_disconnected()
> >
> > When a connexion was established without going through
> > NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> > Now we set it in __cfg80211_connect_result() when it is not already set.
> >
> > Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
> > Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
> > Cc: linux-wireless@vger.kernel.org
> > Cc: stable@vger.kernel.org
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216711
> > Signed-off-by: Marc Bornand <dev.mbornand@systemb.ch>
> > ---
> >  net/wireless/sme.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
>
> The change log ("changes since v3" etc) should be after "---" line and

Does it need another "---" after the change log?
something like:

"---"
"changes since v3:"
"(CHANGES)"
"---"

> the title should start with "wifi: cfg80211:". Please read the wiki link
> below.

Should i start with the version 1 with the new title?
and since i am already changing the title, the following might better
discribe the patch, or should i keep the old title after the ":" ?

[PATCH wireless] wifi: cfg80211: Set SSID if it is not already set

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

Re: [PATCH v4] Set ssid when authenticating

[Kalle Valo]

Marc Bornand <dev.mbornand@systemb.ch> writes:

> On Wed, Feb 15, 2023 at 07:35:09AM +0200, Kalle Valo wrote:
>> Marc Bornand <dev.mbornand@systemb.ch> writes:
>>
>> > changes since v3:
>> > - add missing NULL check
>> > - add missing break
>> >
>> > changes since v2:
>> > - The code was tottaly rewritten based on the disscution of the
>> >   v2 patch.
>> > - the ssid is set in __cfg80211_connect_result() and only if the ssid is
>> >   not already set.
>> > - Do not add an other ssid reset path since it is already done in
>> >   __cfg80211_disconnected()
>> >
>> > When a connexion was established without going through
>> > NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
>> > Now we set it in __cfg80211_connect_result() when it is not already set.
>> >
>> > Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
>> > Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
>> > Cc: linux-wireless@vger.kernel.org
>> > Cc: stable@vger.kernel.org
>> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216711
>> > Signed-off-by: Marc Bornand <dev.mbornand@systemb.ch>
>> > ---
>> >  net/wireless/sme.c | 17 +++++++++++++++++
>> >  1 file changed, 17 insertions(+)
>>
>> The change log ("changes since v3" etc) should be after "---" line and
>
> Does it need another "---" after the change log?
> something like:
>
> "---"
> "changes since v3:"
> "(CHANGES)"
> "---"

No need to add a second "---" line.

>> the title should start with "wifi: cfg80211:". Please read the wiki link
>> below.
>
> Should i start with the version 1 with the new title?

If you reset the version number that might confuse the reviewers, so my
recommendation is to use v5 in the next version. That makes it more
obvious that there are earlier versions available.

> and since i am already changing the title, the following might better
> discribe the patch, or should i keep the old title after the ":" ?
>
> [PATCH wireless] wifi: cfg80211: Set SSID if it is not already set

Changing the title is fine.

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

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