v4.15-rc5 warning: suspicious RCU usage in net/wireless/util.c:778

v4.15-rc5 warning: suspicious RCU usage in net/wireless/util.c:778

[Dominik Brodowski]

On Fri, Dec 22, 2017 at 08:20:12AM +0100, Dominik Brodowski wrote:
> Dear all,
> 
> once the (wifi) link becomes ready, the following warning is emitted on
> mainline (v4.15-rc4-202-gead68f216110) on my notebook:

... and it is still present as of v4.15-rc5-149-g5aa90a845892

> [   22.770422] IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready
> 
> [   22.772364] =============================
> [   22.772369] WARNING: suspicious RCU usage
> [   22.772375] 4.15.0-rc4+ #5 Not tainted
> [   22.772380] -----------------------------
> [   22.772386] /home/brodo/local/kernel/git/linux/net/wireless/util.c:778 suspicious rcu_dereference_check() usage!
> [   22.772391] 
> [   22.772397] 
> [   22.772402] 4 locks held by wpa_supplicant/774:
> [   22.772407]  #0:  (cb_lock){++++}, at: [<00000000276dc3a0>] genl_rcv+0x15/0x40
> [   22.772437]  #1:  (genl_mutex){+.+.}, at: [<0000000024d83eb3>] genl_rcv_msg+0x7a/0x90
> [   22.772463]  #2:  (rtnl_mutex){+.+.}, at: [<000000009de25a59>] nl80211_pre_doit+0xe9/0x190
> [   22.772489]  #3:  (&wdev->mtx){+.+.}, at: [<0000000089bf2cfd>] nl80211_send_iface+0x317/0x8d0
> [   22.772516] 
> [   22.772522] CPU: 3 PID: 774 Comm: wpa_supplicant Not tainted 4.15.0-rc4+ #5
> [   22.772528] Hardware name: Dell Inc. XPS 13 9343/0TM99H, BIOS A11 12/08/2016
> [   22.772532] Call Trace:
> [   22.772544]  dump_stack+0x67/0x95
> [   22.772553]  ieee80211_bss_get_ie+0x66/0x70
> [   22.772562]  nl80211_send_iface+0x344/0x8d0
> [   22.772585]  nl80211_get_interface+0x4b/0xa0
> [   22.772598]  genl_family_rcv_msg+0x32e/0x3f0
> [   22.772607]  ? preempt_count_sub+0x92/0xd0
> [   22.772645]  genl_rcv_msg+0x47/0x90
> [   22.772652]  ? genl_family_rcv_msg+0x3f0/0x3f0
> [   22.772661]  netlink_rcv_skb+0x8a/0x120
> [   22.772677]  genl_rcv+0x24/0x40
> [   22.772684]  netlink_unicast+0x174/0x1f0
> [   22.772698]  netlink_sendmsg+0x386/0x3d0
> [   22.772719]  sock_sendmsg+0x2d/0x40
> [   22.772728]  ___sys_sendmsg+0x2a7/0x300
> [   22.772748]  ? netlink_sendmsg+0x13d/0x3d0
> [   22.772791]  ? __sys_sendmsg+0x67/0xb0
> [   22.772797]  __sys_sendmsg+0x67/0xb0
> [   22.772822]  entry_SYSCALL_64_fastpath+0x18/0x85
> 
> This warning wasn't present in 4.15. Despite of it, networking seems to
> work fine. Nonetheless, the code seems to need a bugfix.

Thanks,
	Dominik

v4.15-rc7 regression/warning: suspicious RCU usage in net/wireless/util.c:778

[Dominik Brodowski]

On Sat, Dec 30, 2017 at 02:11:33PM +0100, Dominik Brodowski wrote:
> On Fri, Dec 22, 2017 at 08:20:12AM +0100, Dominik Brodowski wrote:
> > Dear all,
> > 
> > once the (wifi) link becomes ready, the following warning is emitted on
> > mainline (v4.15-rc4-202-gead68f216110) on my notebook:
> 
> ... and it is still present as of v4.15-rc5-149-g5aa90a845892

... and still present in v4.15-rc7:

[    0.000000] Linux version 4.15.0-rc7+ (brodo@light) (gcc version 7.2.1 20171224 (GCC)) #2 SMP PREEMPT Mon Jan 8 08:58:57 CET 2018
...
[   19.182497] wlan0: associated
[   19.323473] IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready

[   19.324512] =============================
[   19.324525] WARNING: suspicious RCU usage
[   19.324531] 4.15.0-rc7+ #2 Not tainted
[   19.324535] -----------------------------
[   19.324541] /home/brodo/local/kernel/git/linux/net/wireless/util.c:778 suspicious rcu_dereference_check() usage!
[   19.324545] 
               other info that might help us debug this:

[   19.324551] 
               rcu_scheduler_active = 2, debug_locks = 1
[   19.324556] 4 locks held by wpa_supplicant/777:
[   19.324561]  #0:  (cb_lock){++++}, at: [<00000000a93b95ca>] genl_rcv+0x15/0x40
[   19.324590]  #1:  (genl_mutex){+.+.}, at: [<0000000088de9868>] genl_rcv_msg+0x7a/0x90
[   19.324614]  #2:  (rtnl_mutex){+.+.}, at: [<00000000d05b811d>] nl80211_pre_doit+0xe9/0x190
[   19.324639]  #3:  (&wdev->mtx){+.+.}, at: [<0000000015e6766b>] nl80211_send_iface+0x319/0x8d0
[   19.324663] 
               stack backtrace:
[   19.324670] CPU: 3 PID: 777 Comm: wpa_supplicant Not tainted 4.15.0-rc7+ #2
[   19.324674] Hardware name: Dell Inc. XPS 13 9343/0TM99H, BIOS A11 12/08/2016
[   19.324679] Call Trace:
[   19.324690]  dump_stack+0x67/0x95
[   19.324698]  ieee80211_bss_get_ie+0x66/0x70
[   19.324707]  nl80211_send_iface+0x346/0x8d0
[   19.324726]  nl80211_get_interface+0x4b/0xa0
[   19.324738]  genl_family_rcv_msg+0x32e/0x3f0
[   19.324747]  ? preempt_count_sub+0x92/0xd0
[   19.324780]  genl_rcv_msg+0x47/0x90
[   19.324787]  ? genl_family_rcv_msg+0x3f0/0x3f0
[   19.324796]  netlink_rcv_skb+0x8a/0x120
[   19.324811]  genl_rcv+0x24/0x40
[   19.324818]  netlink_unicast+0x174/0x1f0
[   19.324831]  netlink_sendmsg+0x383/0x3d0
[   19.324851]  sock_sendmsg+0x2d/0x40
[   19.324859]  ___sys_sendmsg+0x2a8/0x300
[   19.324877]  ? netlink_sendmsg+0x13a/0x3d0
[   19.324919]  ? __sys_sendmsg+0x67/0xb0
[   19.324925]  __sys_sendmsg+0x67/0xb0
[   19.324949]  entry_SYSCALL_64_fastpath+0x18/0x85
[   19.324955] RIP: 0033:0x779ea30ac037
[   19.324960] RSP: 002b:00007ffd9ea8aa28 EFLAGS: 00000246

Anyone?

Thanks,
	Dominik

[PATCH] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Dominik Brodowski]

As ieee80211_bss_get_ie() derefences an RCU, it needs to be called with
rcu_read_lock held.

Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---

This patch fixes the regression I reported in the last couple of weeks for
various v4.15-rcX revisions to netdev, where a "suspicious RCU usage"
showed up in net/wireless/util.c:778.

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2b3dbcd40e46..1eecc249fb5e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2618,8 +2618,10 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
 		const u8 *ssid_ie;
 		if (!wdev->current_bss)
 			break;
+		rcu_read_lock();
 		ssid_ie = ieee80211_bss_get_ie(&wdev->current_bss->pub,
 					       WLAN_EID_SSID);
+		rcu_read_unlock();
 		if (!ssid_ie)
 			break;
 		if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))

Re: [PATCH] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

Hi,

> Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> ---
> 
> This patch fixes the regression I reported in the last couple of weeks for
> various v4.15-rcX revisions to netdev, where a "suspicious RCU usage"
> showed up in net/wireless/util.c:778.

Huh. You should added linux-wireless to those reports, I simply didn't
see them!

> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2b3dbcd40e46..1eecc249fb5e 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -2618,8 +2618,10 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
>  		const u8 *ssid_ie;
>  		if (!wdev->current_bss)
>  			break;
> +		rcu_read_lock();
>  		ssid_ie = ieee80211_bss_get_ie(&wdev->current_bss->pub,
>  					       WLAN_EID_SSID);
> +		rcu_read_unlock();
>  		if (!ssid_ie)
>  			break;
>  		if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))

This uses the ssid_ie, so that doesn't really seem right? The
protection should extend beyond the usage.

johannes

[PATCH v2] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Dominik Brodowski]

As ieee80211_bss_get_ie() derefences an RCU, it needs to be called with
rcu_read_lock held.

Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---

> This uses the ssid_ie, so that doesn't really seem right? The
> protection should extend beyond the usage.

Indeed -- I had misread the code and hadn't thought of ssid_ie also needing
the protection during its lifetime. So here's a new version 2 -- which I
will only be able to test tomorrow, though...

Thanks,
	Dominik

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2b3dbcd40e46..b53bd8db7974 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2618,12 +2618,15 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
 		const u8 *ssid_ie;
 		if (!wdev->current_bss)
 			break;
+		rcu_read_lock();
 		ssid_ie = ieee80211_bss_get_ie(&wdev->current_bss->pub,
 					       WLAN_EID_SSID);
 		if (!ssid_ie)
-			break;
+			goto nla_rcu_unlock;
 		if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
-			goto nla_put_failure_locked;
+			goto nla_put_failure_rcu_locked;
+ nla_rcu_unlock:
+		rcu_read_unlock();
 		break;
 		}
 	default:
@@ -2635,6 +2638,8 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
 	genlmsg_end(msg, hdr);
 	return 0;
 
+ nla_put_failure_rcu_locked:
+	rcu_read_unlock();
  nla_put_failure_locked:
 	wdev_unlock(wdev);
  nla_put_failure:

Re: [PATCH v2] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

On Sun, 2018-01-14 at 23:22 +0100, Dominik Brodowski wrote:
> 
> +		rcu_read_lock();
>  		ssid_ie = ieee80211_bss_get_ie(&wdev->current_bss->pub,
>  					       WLAN_EID_SSID);
>  		if (!ssid_ie)
> -			break;

nit-picking now: that "break" here may have been easier before these
changes

> +			goto nla_rcu_unlock;
>  		if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
> -			goto nla_put_failure_locked;
> +			goto nla_put_failure_rcu_locked;
> + nla_rcu_unlock:
> +		rcu_read_unlock();
>  		break;

but after, perhaps it's easier to just do

		if (ssid_ie &&
		    nla_put(...)
			goto nla_put_failure_rcu_locked;

and avoid the extra label (but yeah, it's getting late)

johannes

[PATCH v3] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Dominik Brodowski]

As ieee80211_bss_get_ie() derefences an RCU to return ssid_ie, both
the call to this function and any operation on this variable need
protection by the RCU read lock.

Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---

> but after, perhaps it's easier to just do
> 
> 		if (ssid_ie &&
> 		    nla_put(...)
> 			goto nla_put_failure_rcu_locked;
> 
> and avoid the extra label (but yeah, it's getting late)

OK, done that (and updated the commit message), and testet it.

Thanks,
	Dominik

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2b3dbcd40e46..ed87a97fcb0b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2618,12 +2618,13 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
 		const u8 *ssid_ie;
 		if (!wdev->current_bss)
 			break;
+		rcu_read_lock();
 		ssid_ie = ieee80211_bss_get_ie(&wdev->current_bss->pub,
 					       WLAN_EID_SSID);
-		if (!ssid_ie)
-			break;
-		if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
-			goto nla_put_failure_locked;
+		if (ssid_ie &&
+		    nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
+			goto nla_put_failure_rcu_locked;
+		rcu_read_unlock();
 		break;
 		}
 	default:
@@ -2635,6 +2636,8 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
 	genlmsg_end(msg, hdr);
 	return 0;
 
+ nla_put_failure_rcu_locked:
+	rcu_read_unlock();
  nla_put_failure_locked:
 	wdev_unlock(wdev);
  nla_put_failure:

Re: [PATCH v3] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

On Mon, 2018-01-15 at 08:12 +0100, Dominik Brodowski wrote:
> As ieee80211_bss_get_ie() derefences an RCU to return ssid_ie, both
> the call to this function and any operation on this variable need
> protection by the RCU read lock.
> 
> Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> ---
> 
> > but after, perhaps it's easier to just do
> > 
> > 		if (ssid_ie &&
> > 		    nla_put(...)
> > 			goto nla_put_failure_rcu_locked;
> > 
> > and avoid the extra label (but yeah, it's getting late)
> 
> OK, done that (and updated the commit message), and testet it.
> 

Applied, thanks!

johannes