2019-05-15 22:26:31

by Thomas Pedersen

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

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 <[email protected]>
---
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

2019-05-15 22:26:50

by Thomas Pedersen

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

On Wed, May 15, 2019 at 3:21 PM Thomas Pedersen <[email protected]> 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 <[email protected]>
> ---
> 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

2019-05-24 08:30:15

by Johannes Berg

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

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

2019-05-25 03:49:32

by Thomas Pedersen

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

On Fri, May 24, 2019 at 1:29 AM Johannes Berg <[email protected]> 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