| Message ID | 1557958906-1432-1-git-send-email-thomas@eero.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: mesh: fix RCU warning | expand |
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 >
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
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 >
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;
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(+)