| Message ID | 20151218062450.GA19749@gondor.apana.org.au |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2015-12-18 at 14:24 +0800, Herbert Xu wrote: > On Fri, Dec 18, 2015 at 01:34:16PM +0800, Herbert Xu wrote: > > On Fri, Dec 18, 2015 at 09:39:22AM +0800, kernel test robot wrote: > > > FYI, we noticed the below changes on > > > > > > https://github.com/0day-ci/linux Herbert-Xu/rhashtable-Fix-walker-list-corruption/20151216-164833 > > > commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") > > > > > > [ 8.933376] =============================== > > > [ 8.933376] =============================== > > > [ 8.934629] [ INFO: suspicious RCU usage. ] > > > [ 8.934629] [ INFO: suspicious RCU usage. ] > > > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > > > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > > > [ 8.937494] ------------------------------- > > > [ 8.937494] ------------------------------- > > > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > > > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > > > > This is actually a false positive because the new spin lock that > > we hold prevents ht->tbl from disappearing under us. So here is > > a patch to kill the warning with a comment. > > Resent with a proper patch subject and reported-by. > > ---8<--- > The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: > Fix walker list corruption") causes a suspicious RCU usage warning > because we no longer hold ht->mutex when we dereference ht->tbl. > > However, this is a false positive because we now hold ht->lock > which also guarantees that ht->tbl won't disppear from under us. > > This patch kills the warning by using rcu_dereference_raw and > adding a comment. > > Reported-by: kernel test robot <ying.huang@linux.intel.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index eb9240c..3404b06 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -519,7 +519,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) > return -ENOMEM; > > spin_lock(&ht->lock); > - iter->walker->tbl = rht_dereference(ht->tbl, ht); > + /* We do not need RCU protection because we hold ht->lock > + * which guarantees that if we see ht->tbl then it won't > + * die on us. > + */ > + iter->walker->tbl = rcu_dereference_raw(ht->tbl); You can avoid the comment by using the self documented and lockdep enabled primitive iter->walker->tbl = rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); But, storing the ht->tbl and then releasing the lock immediately after escapes RCU protection. So why do we store ht->tbl in the first place ? What exactly prevents it from disappearing after lock is released ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: > > You can avoid the comment by using the self documented and lockdep > enabled primitive > > iter->walker->tbl = rcu_dereference_protected(ht->tbl, > lockdep_is_held(&ht->lock)); That is just gross. I think a comment is much better in this case. If we were to have more place where ht->lock is taken and we had to do the RCU dereference on ht->tbl then we could add a helper for it. For now it's just a single place and I think a comment is the best way to deal with it. > But, storing the ht->tbl and then releasing the lock immediately after > escapes RCU protection. > > So why do we store ht->tbl in the first place ? > > What exactly prevents it from disappearing after lock is released ? We add ourselves to the walker list before we release the lock. The only entity that can destroy ht->tbl will take care of all walkers before doing so. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 18 Dec 2015 21:14:08 +0800 > On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: >> >> You can avoid the comment by using the self documented and lockdep >> enabled primitive >> >> iter->walker->tbl = rcu_dereference_protected(ht->tbl, >> lockdep_is_held(&ht->lock)); > > That is just gross. I think a comment is much better in this case. Herbert, this macro was created exactly to handle this situation, and this is what we do everywhere else in the tree. Please use it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index eb9240c..3404b06 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -519,7 +519,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) return -ENOMEM; spin_lock(&ht->lock); - iter->walker->tbl = rht_dereference(ht->tbl, ht); + /* We do not need RCU protection because we hold ht->lock + * which guarantees that if we see ht->tbl then it won't + * die on us. + */ + iter->walker->tbl = rcu_dereference_raw(ht->tbl); list_add(&iter->walker->list, &iter->walker->tbl->walkers); spin_unlock(&ht->lock);