[RFC] staging: wlan-ng: Driver broken since kernel 5.15

[RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Philipp Hortmann]

Hi,

I would remove the driver from the mainline kernel. What are your thoughts?

I bought two WLAN devices (DUT: D-Link DWL-122 and T-Sinus 111 data) 
that are supported by wlan-ng driver. Issue is that the driver is not 
working anymore.

The error picture is that the device does not receive any packets.
The dmesg says:
[  123.695917] prism2_usb 2-1.6:1.0 wlan0: Unknown mgmt request message 
0x0e4f9800
[  127.508211] prism2_usb 2-1.6:1.0 wlan0: Unknown mgmt request message 
0x04f0d000
...

A working commit 8fc4fb1728855a22f9149079ba51877f5ee61fc9 (HEAD) Date: 
Mon Jul 5 11:16:28 2021 -0700
A failing commit  d980cc0620ae77ab2572235a1300bf22519f2e86 (HEAD) Date: 
  Fri Jul 16 19:08:09 2021 -0700

This means that the devices are unusable since kernel 5.15.

A look into the bitrates shows that only up to 11MBits are supported.
static const struct ieee80211_rate prism2_rates[] = {
...
	{ .bitrate = 110 }
};

Would be interesting to see why this happened. But it is difficult for 
me to find it.

Thanks for your support.

Bye Philipp

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Dan Carpenter]

On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
> Hi,
> 
> I would remove the driver from the mainline kernel. What are your thoughts?
> 
> I bought two WLAN devices (DUT: D-Link DWL-122 and T-Sinus 111 data) that
> are supported by wlan-ng driver. Issue is that the driver is not working
> anymore.
> 
> The error picture is that the device does not receive any packets.
> The dmesg says:
> [  123.695917] prism2_usb 2-1.6:1.0 wlan0: Unknown mgmt request message
> 0x0e4f9800
> [  127.508211] prism2_usb 2-1.6:1.0 wlan0: Unknown mgmt request message
> 0x04f0d000
> ...
> 
> A working commit 8fc4fb1728855a22f9149079ba51877f5ee61fc9 (HEAD) Date: Mon
> Jul 5 11:16:28 2021 -0700
> A failing commit  d980cc0620ae77ab2572235a1300bf22519f2e86 (HEAD) Date:  Fri
> Jul 16 19:08:09 2021 -0700

Those dates are 11 days apart during the v5.14 merge window.  You're
saying 5.15 is broken but the broken commit is in 5.14-rc2 so it really
was broken earlier.

There were only 3 patches to wlan-ng between v5.13 and v5.14.

$ git log --oneline v5.13..v5.14 drivers/staging/wlan-ng/
b1e9109aeff3 staging: wlan-ng: silence incorrect type in argument 1 (different address spaces)
ad843f392035 staging: wlan-ng: remove redundant initialization of variable txresult
ea82ff749587 staging: wlan-ng: cfg80211: Move large struct onto the heap

Obviously I'm going to suspect the largest patch.  Reviewing that patch
now, I see we removed a memset() from the loop.  That seems like a bug.

-               memset(&msg2, 0, sizeof(msg2));
-               msg2.msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
-               msg2.bssindex.data = i;
+               msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
+               msg2->bssindex.data = i;

That's the only interesting change so I suspect it's the issue...
Could you test this patch?  I feel like if you're the first person to
complain since Aug 29 2021 then probably we should just remove the
driver.  Greg is on vacation so lets hold off on removing it until he
comes back.

regards,
dan carpenter


diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
index 471bb310176f..0c270ed8ce67 100644
--- a/drivers/staging/wlan-ng/cfg80211.c
+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -347,6 +347,7 @@ static int prism2_scan(struct wiphy *wiphy,
 	for (i = 0; i < numbss; i++) {
 		int freq;
 
+		memset(msg2, 0, sizeof(*msg2));
 		msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
 		msg2->bssindex.data = i;
 

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Arnd Bergmann]

On Sat, Mar 9, 2024, at 23:09, Philipp Hortmann wrote:
> Hi,
>
> I would remove the driver from the mainline kernel. What are your thoughts?
>
> I bought two WLAN devices (DUT: D-Link DWL-122 and T-Sinus 111 data) 
> that are supported by wlan-ng driver. Issue is that the driver is not 
> working anymore.

I don't think it matters much either way. I did send the patches
to remove wlan drivers that were either using the old pcmcia
interfaces or the old wireless extensions that I think we should
try to eventually kill off entirely, but this driver uses neither
of them and as a result has a lower maintenance cost.

On the other hand, there is also little benefit in keeping the
driver if it's known to be broken, given how better USB wireless
adapters are dirt cheap and widely available. Even if we find and
fix all the bugs in this driver, any supported 802.11n device
would be better in practice.

       Arnd

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Philipp Hortmann]

On 3/11/24 08:04, Dan Carpenter wrote:
> On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
>> Hi,
>>
>> I would remove the driver from the mainline kernel. What are your thoughts?
>>
>> I bought two WLAN devices (DUT: D-Link DWL-122 and T-Sinus 111 data) that
>> are supported by wlan-ng driver. Issue is that the driver is not working
>> anymore.
>>
>> The error picture is that the device does not receive any packets.
>> The dmesg says:
>> [  123.695917] prism2_usb 2-1.6:1.0 wlan0: Unknown mgmt request message
>> 0x0e4f9800
>> [  127.508211] prism2_usb 2-1.6:1.0 wlan0: Unknown mgmt request message
>> 0x04f0d000
>> ...
>>
>> A working commit 8fc4fb1728855a22f9149079ba51877f5ee61fc9 (HEAD) Date: Mon
>> Jul 5 11:16:28 2021 -0700
>> A failing commit  d980cc0620ae77ab2572235a1300bf22519f2e86 (HEAD) Date:  Fri
>> Jul 16 19:08:09 2021 -0700
> 
> Those dates are 11 days apart during the v5.14 merge window.  You're
> saying 5.15 is broken but the broken commit is in 5.14-rc2 so it really
> was broken earlier.
> 
> There were only 3 patches to wlan-ng between v5.13 and v5.14.
> 
> $ git log --oneline v5.13..v5.14 drivers/staging/wlan-ng/
> b1e9109aeff3 staging: wlan-ng: silence incorrect type in argument 1 (different address spaces)
> ad843f392035 staging: wlan-ng: remove redundant initialization of variable txresult
> ea82ff749587 staging: wlan-ng: cfg80211: Move large struct onto the heap
> 
> Obviously I'm going to suspect the largest patch.  Reviewing that patch
> now, I see we removed a memset() from the loop.  That seems like a bug.
> 
> -               memset(&msg2, 0, sizeof(msg2));
> -               msg2.msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
> -               msg2.bssindex.data = i;
> +               msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
> +               msg2->bssindex.data = i;
> 
> That's the only interesting change so I suspect it's the issue...
> Could you test this patch?  I feel like if you're the first person to
> complain since Aug 29 2021 then probably we should just remove the
> driver.  Greg is on vacation so lets hold off on removing it until he
> comes back.
> 
> regards,
> dan carpenter
> 
> 
> diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
> index 471bb310176f..0c270ed8ce67 100644
> --- a/drivers/staging/wlan-ng/cfg80211.c
> +++ b/drivers/staging/wlan-ng/cfg80211.c
> @@ -347,6 +347,7 @@ static int prism2_scan(struct wiphy *wiphy,
>   	for (i = 0; i < numbss; i++) {
>   		int freq;
>   
> +		memset(msg2, 0, sizeof(*msg2));
>   		msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
>   		msg2->bssindex.data = i;
>   


Hi Dan,

sorry it is so crowded here.

You are right with the statement that it is this commit.
commit ea82ff749587807fa48e3277c977ff3cec266f25 (HEAD)
Author: Lee Jones <lee.jones@linaro.org>
Date:   Wed Apr 14 19:10:39 2021 +0100

     staging: wlan-ng: cfg80211: Move large struct onto the heap

     Fixes the following W=1 kernel build warning(s):

      drivers/staging/wlan-ng/cfg80211.c: In function ‘prism2_scan’:
      drivers/staging/wlan-ng/cfg80211.c:388:1: warning: the frame size 
of 1296 bytes is larger than 1024 bytes [-Wframe-larger-than=]

But It is not depending on the line you pointed to.

I need another week to look into this.

Thanks for your support.

Bye Philipp

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Arnd Bergmann]

On Sun, Mar 17, 2024, at 21:07, Philipp Hortmann wrote:
> On 3/11/24 08:04, Dan Carpenter wrote:
>> On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
> You are right with the statement that it is this commit.
> commit ea82ff749587807fa48e3277c977ff3cec266f25 (HEAD)
> Author: Lee Jones <lee.jones@linaro.org>
> Date:   Wed Apr 14 19:10:39 2021 +0100
>
>      staging: wlan-ng: cfg80211: Move large struct onto the heap
>
>      Fixes the following W=1 kernel build warning(s):
>
>       drivers/staging/wlan-ng/cfg80211.c: In function ‘prism2_scan’:
>       drivers/staging/wlan-ng/cfg80211.c:388:1: warning: the frame size 
> of 1296 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> But It is not depending on the line you pointed to.

Right, the kzalloc() already clears the data, so the memset
is not needed.

> I need another week to look into this.

I'm fairly sure this fixes the bug, the problem here was that
the cast to (u8 *) hides the incorrect conversion:

diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
index 471bb310176f..9d6a2dd35ba9 100644
--- a/drivers/staging/wlan-ng/cfg80211.c
+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -350,7 +350,7 @@ static int prism2_scan(struct wiphy *wiphy,
 		msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
 		msg2->bssindex.data = i;
 
-		result = p80211req_dorequest(wlandev, (u8 *)&msg2);
+		result = p80211req_dorequest(wlandev, (u8 *)msg2);
 		if ((result != 0) ||
 		    (msg2->resultcode.data != P80211ENUM_resultcode_success)) {
 			break;

     Arnd

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Dan Carpenter]

On Sun, Mar 17, 2024 at 09:20:34PM +0100, Arnd Bergmann wrote:
> On Sun, Mar 17, 2024, at 21:07, Philipp Hortmann wrote:
> > On 3/11/24 08:04, Dan Carpenter wrote:
> >> On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
> > You are right with the statement that it is this commit.
> > commit ea82ff749587807fa48e3277c977ff3cec266f25 (HEAD)
> > Author: Lee Jones <lee.jones@linaro.org>
> > Date:   Wed Apr 14 19:10:39 2021 +0100
> >
> >      staging: wlan-ng: cfg80211: Move large struct onto the heap
> >
> >      Fixes the following W=1 kernel build warning(s):
> >
> >       drivers/staging/wlan-ng/cfg80211.c: In function ‘prism2_scan’:
> >       drivers/staging/wlan-ng/cfg80211.c:388:1: warning: the frame size 
> > of 1296 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> >
> > But It is not depending on the line you pointed to.
> 
> Right, the kzalloc() already clears the data, so the memset
> is not needed.
> 

No, it's inside a loop so it needs to be cleared on each iteration.

> > I need another week to look into this.
> 
> I'm fairly sure this fixes the bug, the problem here was that
> the cast to (u8 *) hides the incorrect conversion:
> 
> diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
> index 471bb310176f..9d6a2dd35ba9 100644
> --- a/drivers/staging/wlan-ng/cfg80211.c
> +++ b/drivers/staging/wlan-ng/cfg80211.c
> @@ -350,7 +350,7 @@ static int prism2_scan(struct wiphy *wiphy,
>  		msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
>  		msg2->bssindex.data = i;
>  
> -		result = p80211req_dorequest(wlandev, (u8 *)&msg2);
> +		result = p80211req_dorequest(wlandev, (u8 *)msg2);

Ah, well done.

It feels like this is the kind of bug which should be caught with
static analysis.  One of the things that people want from static
analysis is looking at what a patch does.  So if we pass &msg2 and the
patch moved msg from the stack to be kmalloc()ed, then print a warning.
It's not something that Smatch does.

I have my rename_rev.pl script (https://github.com/error27/rename_rev)
which I use to filter out variable renames or see if (1 << foo) is
converted to BIT(foo) correctly.  Maybe I could extend that to check
"move stack to heap" patches...

regards,
dan carpenter

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Arnd Bergmann]

On Mon, Mar 18, 2024, at 09:01, Dan Carpenter wrote:
> On Sun, Mar 17, 2024 at 09:20:34PM +0100, Arnd Bergmann wrote:
>> On Sun, Mar 17, 2024, at 21:07, Philipp Hortmann wrote:
>> > On 3/11/24 08:04, Dan Carpenter wrote:
>> >> On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
>> > You are right with the statement that it is this commit.
>> > commit ea82ff749587807fa48e3277c977ff3cec266f25 (HEAD)
>> > Author: Lee Jones <lee.jones@linaro.org>
>> > Date:   Wed Apr 14 19:10:39 2021 +0100
>> >
>> >      staging: wlan-ng: cfg80211: Move large struct onto the heap
>> >
>> >      Fixes the following W=1 kernel build warning(s):
>> >
>> >       drivers/staging/wlan-ng/cfg80211.c: In function ‘prism2_scan’:
>> >       drivers/staging/wlan-ng/cfg80211.c:388:1: warning: the frame size 
>> > of 1296 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> >
>> > But It is not depending on the line you pointed to.
>> 
>> Right, the kzalloc() already clears the data, so the memset
>> is not needed.
>> 
>
> No, it's inside a loop so it needs to be cleared on each iteration.

Right, at least the conversion could not remove the memset()
without a deeper analysis. It's still likely that each
field of the structure still gets initialized properly
inside the loop and the repeated memset() wasn't necessary
in the first place, but that is a completely separate question.

    Arnd

Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Philipp Hortmann]

On 3/18/24 09:01, Dan Carpenter wrote:
> On Sun, Mar 17, 2024 at 09:20:34PM +0100, Arnd Bergmann wrote:
>> On Sun, Mar 17, 2024, at 21:07, Philipp Hortmann wrote:
>>> On 3/11/24 08:04, Dan Carpenter wrote:
>>>> On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
>>> You are right with the statement that it is this commit.
>>> commit ea82ff749587807fa48e3277c977ff3cec266f25 (HEAD)
>>> Author: Lee Jones <lee.jones@linaro.org>
>>> Date:   Wed Apr 14 19:10:39 2021 +0100
>>>
>>>       staging: wlan-ng: cfg80211: Move large struct onto the heap
>>>
>>>       Fixes the following W=1 kernel build warning(s):
>>>
>>>        drivers/staging/wlan-ng/cfg80211.c: In function ‘prism2_scan’:
>>>        drivers/staging/wlan-ng/cfg80211.c:388:1: warning: the frame size
>>> of 1296 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>>>
>>> But It is not depending on the line you pointed to.
>>
>> Right, the kzalloc() already clears the data, so the memset
>> is not needed.
>>
> 
> No, it's inside a loop so it needs to be cleared on each iteration.
> 
>>> I need another week to look into this.
>>
>> I'm fairly sure this fixes the bug, the problem here was that
>> the cast to (u8 *) hides the incorrect conversion:
>>
>> diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
>> index 471bb310176f..9d6a2dd35ba9 100644
>> --- a/drivers/staging/wlan-ng/cfg80211.c
>> +++ b/drivers/staging/wlan-ng/cfg80211.c
>> @@ -350,7 +350,7 @@ static int prism2_scan(struct wiphy *wiphy,
>>   		msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
>>   		msg2->bssindex.data = i;
>>   
>> -		result = p80211req_dorequest(wlandev, (u8 *)&msg2);
>> +		result = p80211req_dorequest(wlandev, (u8 *)msg2);
> 
> Ah, well done.
> 
> It feels like this is the kind of bug which should be caught with
> static analysis.  One of the things that people want from static
> analysis is looking at what a patch does.  So if we pass &msg2 and the
> patch moved msg from the stack to be kmalloc()ed, then print a warning.
> It's not something that Smatch does.
> 
> I have my rename_rev.pl script (https://github.com/error27/rename_rev)
> which I use to filter out variable renames or see if (1 << foo) is
> converted to BIT(foo) correctly.  Maybe I could extend that to check
> "move stack to heap" patches...
> 
> regards,
> dan carpenter
> 

Hi,

This change alone.

+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -350,7 +350,7 @@ static int prism2_scan(struct wiphy *wiphy,
                 msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
                 msg2->bssindex.data = i;

-               result = p80211req_dorequest(wlandev, (u8 *)&msg2);
+               result = p80211req_dorequest(wlandev, (u8 *)msg2);
                 if ((result != 0) ||
                     (msg2->resultcode.data != 
P80211ENUM_resultcode_success)) {
                         break;
makes the driver work again under latest kernel.

I can only use unencrypted transmission. Throughput is 200 to 800kByte/s
WEP would need further investigation.

Thanks for your support.

Bye Philipp