[PATCH] mac80211: mesh: fix RCU warning

[PATCH] mac80211: mesh: fix RCU warning

[Thomas Pedersen]

ifmsh->csa was being dereferenced without the RCU read
lock held.

fixes the following warning:

[   74.930435] =============================
[   74.932066] WARNING: suspicious RCU usage
[   74.933671] 4.20.13 #5 Tainted: G        W
[   74.935804] -----------------------------
[   74.937427] net/mac80211/mesh.c:1218 suspicious rcu_dereference_check() usage!
[   74.940473] other info that might help us debug this:
[   74.943654] rcu_scheduler_active = 2, debug_locks = 1
[   74.946311] 5 locks held by kworker/u8:3/107:
[   74.948087]  #0: 000000007623c1f0 ((wq_completion)"%s"wiphy_name(local->hw.wiphy)){+.+.}, at: process_one_work+0x1a2/0x610
[   74.952464]  #1: 00000000077b4215 ((work_completion)(&sdata->csa_finalize_work)){+.+.}, at: process_one_work+0x1a2/0x610
[   74.957228]  #2: 00000000e02b12da (&wdev->mtx){+.+.}, at: ieee80211_csa_finalize_work+0x2f/0x90
[   74.959870]  #3: 00000000e6855095 (&local->mtx){+.+.}, at: ieee80211_csa_finalize_work+0x47/0x90
[   74.962937]  #4: 00000000bb5e3bca (&local->chanctx_mtx){+.+.}, at: ieee80211_csa_finalize_work+0x51/0x90

Signed-off-by: Thomas Pedersen <thomas@eero.com>
---
 net/mac80211/mesh.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 766e5e5..70aeb34 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -1220,10 +1220,12 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
 	ifmsh->chsw_ttl = 0;
 
 	/* Remove the CSA and MCSP elements from the beacon */
+	rcu_read_lock();
 	tmp_csa_settings = rcu_dereference(ifmsh->csa);
 	RCU_INIT_POINTER(ifmsh->csa, NULL);
 	if (tmp_csa_settings)
 		kfree_rcu(tmp_csa_settings, rcu_head);
+	rcu_read_unlock();
 	ret = ieee80211_mesh_rebuild_beacon(sdata);
 	if (ret)
 		return -EINVAL;
-- 
2.7.4

Re: [PATCH] mac80211: mesh: fix RCU warning

[Thomas Pedersen]

On Wed, May 15, 2019 at 3:21 PM Thomas Pedersen <thomas@eero.com> wrote:
>
> ifmsh->csa was being dereferenced without the RCU read
> lock held.
>
> fixes the following warning:
>
> [   74.930435] =============================
> [   74.932066] WARNING: suspicious RCU usage
> [   74.933671] 4.20.13 #5 Tainted: G        W
> [   74.935804] -----------------------------
> [   74.937427] net/mac80211/mesh.c:1218 suspicious rcu_dereference_check() usage!
> [   74.940473] other info that might help us debug this:
> [   74.943654] rcu_scheduler_active = 2, debug_locks = 1
> [   74.946311] 5 locks held by kworker/u8:3/107:
> [   74.948087]  #0: 000000007623c1f0 ((wq_completion)"%s"wiphy_name(local->hw.wiphy)){+.+.}, at: process_one_work+0x1a2/0x610
> [   74.952464]  #1: 00000000077b4215 ((work_completion)(&sdata->csa_finalize_work)){+.+.}, at: process_one_work+0x1a2/0x610
> [   74.957228]  #2: 00000000e02b12da (&wdev->mtx){+.+.}, at: ieee80211_csa_finalize_work+0x2f/0x90
> [   74.959870]  #3: 00000000e6855095 (&local->mtx){+.+.}, at: ieee80211_csa_finalize_work+0x47/0x90
> [   74.962937]  #4: 00000000bb5e3bca (&local->chanctx_mtx){+.+.}, at: ieee80211_csa_finalize_work+0x51/0x90

Sorry the commit message is a little out of date, I actually tested on
5.1.0-rc7-wt as well.

> Signed-off-by: Thomas Pedersen <thomas@eero.com>
> ---
>  net/mac80211/mesh.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
> index 766e5e5..70aeb34 100644
> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -1220,10 +1220,12 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
>         ifmsh->chsw_ttl = 0;
>
>         /* Remove the CSA and MCSP elements from the beacon */
> +       rcu_read_lock();
>         tmp_csa_settings = rcu_dereference(ifmsh->csa);
>         RCU_INIT_POINTER(ifmsh->csa, NULL);
>         if (tmp_csa_settings)
>                 kfree_rcu(tmp_csa_settings, rcu_head);
> +       rcu_read_unlock();
>         ret = ieee80211_mesh_rebuild_beacon(sdata);
>         if (ret)
>                 return -EINVAL;
> --
> 2.7.4
>


-- 
thomas

Re: [PATCH] mac80211: mesh: fix RCU warning

[Johannes Berg]

On Wed, 2019-05-15 at 15:21 -0700, Thomas Pedersen wrote:
> ifmsh->csa was being dereferenced without the RCU read
> lock held.

> +++ b/net/mac80211/mesh.c
> @@ -1220,10 +1220,12 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
>  	ifmsh->chsw_ttl = 0;
>  
>  	/* Remove the CSA and MCSP elements from the beacon */
> +	rcu_read_lock();
>  	tmp_csa_settings = rcu_dereference(ifmsh->csa);
>  	RCU_INIT_POINTER(ifmsh->csa, NULL);
>  	if (tmp_csa_settings)
>  		kfree_rcu(tmp_csa_settings, rcu_head);
> +	rcu_read_unlock();

This seems wrong to me.

Really this code is the *writer* side, so you should do something like
this:

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 766e5e5bab8a..d578147ad7e8 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -1220,7 +1220,8 @@ int ieee80211_mesh_finish_csa(struct
ieee80211_sub_if_data *sdata)
 	ifmsh->chsw_ttl = 0;
 
 	/* Remove the CSA and MCSP elements from the beacon */
-	tmp_csa_settings = rcu_dereference(ifmsh->csa);
+	tmp_csa_settings = rcu_dereference_protected(ifmsh->csa,
+				lockdep_is_held(&sdata->wdev.mtx));
 	RCU_INIT_POINTER(ifmsh->csa, NULL);
 	if (tmp_csa_settings)
 		kfree_rcu(tmp_csa_settings, rcu_head);
@@ -1242,6 +1243,8 @@ int ieee80211_mesh_csa_beacon(struct
ieee80211_sub_if_data *sdata,
 	struct mesh_csa_settings *tmp_csa_settings;
 	int ret = 0;
 
+	lockdep_assert_held(&sdata->wdev.mtx);
+
 	tmp_csa_settings = kmalloc(sizeof(*tmp_csa_settings),
 				   GFP_ATOMIC);
 	if (!tmp_csa_settings)


Can you test that and send a proper patch?

johannes

Re: [PATCH] mac80211: mesh: fix RCU warning

[Thomas Pedersen]

On Fri, May 24, 2019 at 1:29 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2019-05-15 at 15:21 -0700, Thomas Pedersen wrote:
> > ifmsh->csa was being dereferenced without the RCU read
> > lock held.
>
> > +++ b/net/mac80211/mesh.c
> > @@ -1220,10 +1220,12 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
> >       ifmsh->chsw_ttl = 0;
> >
> >       /* Remove the CSA and MCSP elements from the beacon */
> > +     rcu_read_lock();
> >       tmp_csa_settings = rcu_dereference(ifmsh->csa);
> >       RCU_INIT_POINTER(ifmsh->csa, NULL);
> >       if (tmp_csa_settings)
> >               kfree_rcu(tmp_csa_settings, rcu_head);
> > +     rcu_read_unlock();
>
> This seems wrong to me.
>
> Really this code is the *writer* side, so you should do something like
> this:

Thanks this looks correct. I should've thought about this a tiny bit more ;)

> diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
> index 766e5e5bab8a..d578147ad7e8 100644
> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -1220,7 +1220,8 @@ int ieee80211_mesh_finish_csa(struct
> ieee80211_sub_if_data *sdata)
>         ifmsh->chsw_ttl = 0;
>
>         /* Remove the CSA and MCSP elements from the beacon */
> -       tmp_csa_settings = rcu_dereference(ifmsh->csa);
> +       tmp_csa_settings = rcu_dereference_protected(ifmsh->csa,
> +                               lockdep_is_held(&sdata->wdev.mtx));
>         RCU_INIT_POINTER(ifmsh->csa, NULL);
>         if (tmp_csa_settings)
>                 kfree_rcu(tmp_csa_settings, rcu_head);
> @@ -1242,6 +1243,8 @@ int ieee80211_mesh_csa_beacon(struct
> ieee80211_sub_if_data *sdata,
>         struct mesh_csa_settings *tmp_csa_settings;
>         int ret = 0;
>
> +       lockdep_assert_held(&sdata->wdev.mtx);
> +
>         tmp_csa_settings = kmalloc(sizeof(*tmp_csa_settings),
>                                    GFP_ATOMIC);
>         if (!tmp_csa_settings)
>
>
> Can you test that and send a proper patch?
>
> johannes
>


-- 
thomas