`iwlist scan` fails with many networks available

`iwlist scan` fails with many networks available

[James Nylen]

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

I've spent the past few days in a large hotel and noticed that `iwlist
scan` fails with the error message "Argument list too long".

This seems to be because there are a great many wireless networks in
my vicinity.  See:  https://bugzilla.kernel.org/show_bug.cgi?id=16384

Using `iw` as recommended in that ticket works, but it's not a great
option for me because `wicd` doesn't support it.

If I'm understanding the relevant code correctly, the attached patch
works around the issue by skipping networks that would not fit in the
scan-results buffer and returning partial results instead of just
erroring out.

This is the first time I've modified the kernel code directly.  I have
a couple of questions:

- This seems to be working like I intended, but the condition was a
bit hard to reproduce consistently.  Am I inadvertently breaking
things?
- Is there a better way to accomplish this?  Reading the `iwlist` code
I'm not entirely sure why this is occurring, because it looks like it
should be allocating a dynamic buffer length.

[-- Attachment #2: wireless-scan-skip-extras.diff --]
[-- Type: text/plain, Size: 1225 bytes --]

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 21be56b..fc74091 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1699,6 +1699,7 @@ static int ieee80211_scan_results(struct cfg80211_registered_device *rdev,
 				  struct iw_request_info *info,
 				  char *buf, size_t len)
 {
+	char *maybe_current_ev;
 	char *current_ev = buf;
 	char *end_buf = buf + len;
 	struct cfg80211_internal_bss *bss;
@@ -1709,14 +1710,20 @@ static int ieee80211_scan_results(struct cfg80211_registered_device *rdev,
 
 	list_for_each_entry(bss, &rdev->bss_list, list) {
 		if (buf + len - current_ev <= IW_EV_ADDR_LEN) {
-			err = -E2BIG;
+			// Buffer too big to hold another BSS; ignore
 			break;
 		}
-		current_ev = ieee80211_bss(&rdev->wiphy, info, bss,
-					   current_ev, end_buf);
-		if (IS_ERR(current_ev)) {
-			err = PTR_ERR(current_ev);
+		maybe_current_ev = ieee80211_bss(&rdev->wiphy, info, bss,
+					         current_ev, end_buf);
+		if (IS_ERR(maybe_current_ev)) {
+			err = PTR_ERR(maybe_current_ev);
+			if (err == -E2BIG) {
+				// Last BSS failed to copy into buffer; ignore
+				err = 0;
+			}
 			break;
+		} else {
+			current_ev = maybe_current_ev;
 		}
 	}
 	spin_unlock_bh(&rdev->bss_lock);

[PATCH] `iwlist scan` fails with many networks available

[James Nylen]

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

In 5.x it's still possible for `ieee80211_scan_results` (`iwlist
scan`) to fail when too many wireless networks are available.  This
code path is used by `wicd`.

Previously: https://lkml.org/lkml/2017/4/2/192

I've been applying this updated patch to my own kernels since 2017 with
no issues.  I am sure it is not the ideal way to solve this problem, but
I'm making my fix available in case it helps others.

Please advise on next steps or if this is a dead end.

[-- Attachment #2: wireless-scan-less-e2big.diff --]
[-- Type: text/plain, Size: 1993 bytes --]

commit 8e80dcb0df71ac8f5d3640bcdb1bba9c7693d63a
Author: James Nylen <jnylen@gmail.com>
Date:   Wed Apr 26 14:38:58 2017 +0200

    Hack: Make `ieee80211_scan_results` (`iwlist scan`) return less E2BIG
    
    See: https://lkml.org/lkml/2017/4/2/192
    
    (and branch `jcn/hack/wireless-scan-no-e2big`)
    
    This should really be done with a bigger limit inside the `iwlist` code
    instead, if possible (or even better: modify `wicd` to use `iw scan`
    instead).

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 21be56b3128e..08fa9cb68f59 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1699,6 +1699,7 @@ static int ieee80211_scan_results(struct cfg80211_registered_device *rdev,
 				  struct iw_request_info *info,
 				  char *buf, size_t len)
 {
+	char *maybe_current_ev;
 	char *current_ev = buf;
 	char *end_buf = buf + len;
 	struct cfg80211_internal_bss *bss;
@@ -1709,14 +1710,29 @@ static int ieee80211_scan_results(struct cfg80211_registered_device *rdev,
 
 	list_for_each_entry(bss, &rdev->bss_list, list) {
 		if (buf + len - current_ev <= IW_EV_ADDR_LEN) {
-			err = -E2BIG;
+			// Buffer too small to hold another BSS.  Only report
+			// an error if we have not yet reached the maximum
+			// buffer size that `iwlist` can handle.
+			if (len < 0xFFFF) {
+				err = -E2BIG;
+			}
 			break;
 		}
-		current_ev = ieee80211_bss(&rdev->wiphy, info, bss,
-					   current_ev, end_buf);
-		if (IS_ERR(current_ev)) {
-			err = PTR_ERR(current_ev);
+		maybe_current_ev = ieee80211_bss(&rdev->wiphy, info, bss,
+					         current_ev, end_buf);
+		if (IS_ERR(maybe_current_ev)) {
+			err = PTR_ERR(maybe_current_ev);
+			if (err == -E2BIG) {
+				// Last BSS failed to copy into buffer.  As
+				// above, only report an error if `iwlist` will
+				// retry again with a larger buffer.
+				if (len >= 0xFFFF) {
+					err = 0;
+				}
+			}
 			break;
+		} else {
+			current_ev = maybe_current_ev;
 		}
 	}
 	spin_unlock_bh(&rdev->bss_lock);

Re: [PATCH] `iwlist scan` fails with many networks available

[Johannes Berg]

On Sun, 2019-08-11 at 02:08 +0000, James Nylen wrote:
> In 5.x it's still possible for `ieee80211_scan_results` (`iwlist
> scan`) to fail when too many wireless networks are available.  This
> code path is used by `wicd`.
> 
> Previously: https://lkml.org/lkml/2017/4/2/192

This has been known for probably a decade or longer. I don't know why
'wicd' still insists on using wext, unless it's no longer maintained at
all. nl80211 doesn't have this problem at all, and I think gives more
details about the networks found too.

> I've been applying this updated patch to my own kernels since 2017 with
> no issues.  I am sure it is not the ideal way to solve this problem, but
> I'm making my fix available in case it helps others.

I don't think silently dropping data is a good solution.

I suppose we could consider applying a workaround like this if it has a
condition checking that the buffer passed in is the maximum possible
buffer (65535 bytes, due to iw_point::length being u16), but below that
-E2BIG serves well-written implementations as an indicator that they
need to retry with a bigger buffer.

> Please advise on next steps or if this is a dead end.

I think wireless extensions are in fact a dead end and all software
(even 'wicd', which seems to be the lone holdout) should migrate to
nl80211 instead.

johannes

Re: [PATCH] `iwlist scan` fails with many networks available

[James Nylen]

>I suppose we could consider applying a workaround like this if it has a
>condition checking that the buffer passed in is the maximum possible
>buffer (65535 bytes, due to iw_point::length being u16)

This is what the latest patch does (attached to my email from
yesterday / https://lkml.org/lkml/2019/8/10/452 ).

If you'd like to apply it, I'm happy to make any needed revisions.
Otherwise I'm going to have to keep patching my kernels for this
issue, unfortunately I don't have the time to try to get wicd to
migrate to a better solution.

On 8/11/19, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2019-08-11 at 02:08 +0000, James Nylen wrote:
>> In 5.x it's still possible for `ieee80211_scan_results` (`iwlist
>> scan`) to fail when too many wireless networks are available.  This
>> code path is used by `wicd`.
>>
>> Previously: https://lkml.org/lkml/2017/4/2/192
>
> This has been known for probably a decade or longer. I don't know why
> 'wicd' still insists on using wext, unless it's no longer maintained at
> all. nl80211 doesn't have this problem at all, and I think gives more
> details about the networks found too.
>
>> I've been applying this updated patch to my own kernels since 2017 with
>> no issues.  I am sure it is not the ideal way to solve this problem, but
>> I'm making my fix available in case it helps others.
>
> I don't think silently dropping data is a good solution.
>
> I suppose we could consider applying a workaround like this if it has a
> condition checking that the buffer passed in is the maximum possible
> buffer (65535 bytes, due to iw_point::length being u16), but below that
> -E2BIG serves well-written implementations as an indicator that they
> need to retry with a bigger buffer.
>
>> Please advise on next steps or if this is a dead end.
>
> I think wireless extensions are in fact a dead end and all software
> (even 'wicd', which seems to be the lone holdout) should migrate to
> nl80211 instead.
>
> johannes
>
>

Re: [PATCH] `iwlist scan` fails with many networks available

[Johannes Berg]

On Tue, 2019-08-13 at 00:43 +0000, James Nylen wrote:
> > I suppose we could consider applying a workaround like this if it has a
> > condition checking that the buffer passed in is the maximum possible
> > buffer (65535 bytes, due to iw_point::length being u16)
> 
> This is what the latest patch does (attached to my email from
> yesterday / https://lkml.org/lkml/2019/8/10/452 ).

Hmm, yes, you're right. I evidently missed the comparisons to 0xFFFF
there, sorry about that.

> If you'd like to apply it, I'm happy to make any needed revisions.
> Otherwise I'm going to have to keep patching my kernels for this
> issue, unfortunately I don't have the time to try to get wicd to
> migrate to a better solution.

Not sure which would be easier, but ok :-)

Can you please fix the patch to
 1) use /* */ style comments (see
    https://www.kernel.org/doc/html/latest/process/coding-style.html)

 2) remove extra braces (also per coding style)

 3) use U16_MAX instead of 0xFFFF

I'd also consider renaming "maybe_current_ev" to "next_ev" or something
shorter anyway, and would probably argue that rewriting this

> +		if (IS_ERR(maybe_current_ev)) {
> +			err = PTR_ERR(maybe_current_ev);
> +			if (err == -E2BIG) {
> +				// Last BSS failed to copy into buffer.  As
> +				// above, only report an error if `iwlist` will
> +				// retry again with a larger buffer.
> +				if (len >= 0xFFFF) {
> +					err = 0;
> +				}
> +			}
>  			break;
> +		} else {
> +			current_ev = maybe_current_ev;
>  		}


to something like

	next_ev = ...
	if (IS_ERR(next_ev)) {
		err = PTR_ERR(next_ev);
		/* mask error and truncate in case buffer cannot be
                 * increased
                 */
		if (err == -E2BIG && len < U16_MAX)
			err = 0;
		break;
	}

	current_ev = next_ev;


could be more readable, but that's just editorial really.

Thanks,
johannes