rhashtable: Kill harmless RCU warning in rhashtable_walk_init
diff mbox series

Message ID 20151218062450.GA19749@gondor.apana.org.au
State New, archived
Headers show
Series
  • rhashtable: Kill harmless RCU warning in rhashtable_walk_init
Related show

Commit Message

Herbert Xu Dec. 18, 2015, 6:24 a.m. UTC
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>

Comments

Eric Dumazet Dec. 18, 2015, 12:54 p.m. UTC | #1
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Herbert Xu Dec. 18, 2015, 1:14 p.m. UTC | #2
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,
David Miller Dec. 18, 2015, 9:27 p.m. UTC | #3
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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);