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!
[ 8.941705]
[ 8.941705] other info that might help us debug this:
[ 8.941705]
[ 8.941705]
[ 8.941705] other info that might help us debug this:
[ 8.941705]
[ 8.944161]
[ 8.944161] rcu_scheduler_active = 1, debug_locks = 0
[ 8.944161]
[ 8.944161] rcu_scheduler_active = 1, debug_locks = 0
[ 8.946244] 1 lock held by swapper/0/1:
[ 8.946244] 1 lock held by swapper/0/1:
[ 8.947463] #0:
[ 8.947463] #0: ( (&(&ht->lock)->rlock&(&ht->lock)->rlock){+.+...}){+.+...}, at: , at: [<ffffffff814b8900>] rhashtable_walk_init+0x70/0x150
[<ffffffff814b8900>] rhashtable_walk_init+0x70/0x150
[ 8.950428]
[ 8.950428] stack backtrace:
[ 8.950428]
[ 8.950428] stack backtrace:
[ 8.951770] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc3-00995-gf9f51b8 #2
[ 8.951770] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc3-00995-gf9f51b8 #2
[ 8.954245] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[ 8.954245] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[ 8.956973] 0000000000000001
[ 8.956973] 0000000000000001 ffff880078393d30 ffff880078393d30 ffffffff81493238 ffffffff81493238 ffff88007838c040 ffff88007838c040
[ 8.959333] ffff880078393d60
[ 8.959333] ffff880078393d60 ffffffff8112cb9f ffffffff8112cb9f ffff880078393da0 ffff880078393da0 ffffffff83e9d6c0 ffffffff83e9d6c0
[ 8.961684] ffffffff83e9d7f0
[ 8.961684] ffffffff83e9d7f0 ffff880061720e00 ffff880061720e00 ffff880078393d90 ffff880078393d90 ffffffff814b89c8 ffffffff814b89c8
[ 8.964148] Call Trace:
[ 8.964148] Call Trace:
[ 8.964955] [<ffffffff81493238>] dump_stack+0x7c/0xb4
[ 8.964955] [<ffffffff81493238>] dump_stack+0x7c/0xb4
[ 8.966728] [<ffffffff8112cb9f>] lockdep_rcu_suspicious+0x14f/0x1c0
[ 8.966728] [<ffffffff8112cb9f>] lockdep_rcu_suspicious+0x14f/0x1c0
[ 8.968753] [<ffffffff814b89c8>] rhashtable_walk_init+0x138/0x150
[ 8.968753] [<ffffffff814b89c8>] rhashtable_walk_init+0x138/0x150
[ 8.970567] [<ffffffff815021d8>] test_bucket_stats+0x22/0x17d
[ 8.970567] [<ffffffff815021d8>] test_bucket_stats+0x22/0x17d
[ 8.972682] [<ffffffff82dda0fa>] test_rhashtable+0xe0/0x12ac
[ 8.972682] [<ffffffff82dda0fa>] test_rhashtable+0xe0/0x12ac
[ 8.974746] [<ffffffff816157db>] ? get_random_bytes+0x2b/0x40
[ 8.974746] [<ffffffff816157db>] ? get_random_bytes+0x2b/0x40
[ 8.976467] [<ffffffff814b6c33>] ? bucket_table_alloc+0x173/0x280
[ 8.976467] [<ffffffff814b6c33>] ? bucket_table_alloc+0x173/0x280
[ 8.978548] [<ffffffff82ddb3d5>] test_rht_init+0x10f/0x523
[ 8.978548] [<ffffffff82ddb3d5>] test_rht_init+0x10f/0x523
[ 8.980179] [<ffffffff82ddb2c6>] ? test_rhashtable+0x12ac/0x12ac
[ 8.980179] [<ffffffff82ddb2c6>] ? test_rhashtable+0x12ac/0x12ac
[ 8.982424] [<ffffffff82d9f757>] do_one_initcall+0x16b/0x248
[ 8.982424] [<ffffffff82d9f757>] do_one_initcall+0x16b/0x248
[ 8.984208] [<ffffffff82d9f9f8>] kernel_init_freeable+0x1c4/0x2b8
[ 8.984208] [<ffffffff82d9f9f8>] kernel_init_freeable+0x1c4/0x2b8
[ 8.986165] [<ffffffff81ed5b60>] ? rest_init+0x200/0x200
[ 8.986165] [<ffffffff81ed5b60>] ? rest_init+0x200/0x200
[ 8.987925] [<ffffffff81ed5b71>] kernel_init+0x11/0x190
[ 8.987925] [<ffffffff81ed5b71>] kernel_init+0x11/0x190
[ 8.989608] [<ffffffff81ee406f>] ret_from_fork+0x3f/0x70
[ 8.989608] [<ffffffff81ee406f>] ret_from_fork+0x3f/0x70
[ 8.991270] [<ffffffff81ed5b60>] ? rest_init+0x200/0x200
[ 8.991270] [<ffffffff81ed5b60>] ? rest_init+0x200/0x200
Thanks,
Kernel Test Robot
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.
---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.
Signed-off-by: Herbert Xu <[email protected]>
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);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
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);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
>
> 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 ?
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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <[email protected]>
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.
On Fri, Dec 18, 2015 at 04:27:31PM -0500, David Miller wrote:
> From: Herbert Xu <[email protected]>
> 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.
OK.
---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_protected.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index eb9240c..51282f5 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -519,7 +519,8 @@ 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);
+ iter->walker->tbl =
+ rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock));
list_add(&iter->walker->list, &iter->walker->tbl->walkers);
spin_unlock(&ht->lock);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <[email protected]>
Date: Sat, 19 Dec 2015 10:45:28 +0800
> On Fri, Dec 18, 2015 at 04:27:31PM -0500, David Miller wrote:
>> From: Herbert Xu <[email protected]>
>> 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.
>
> OK.
>
> ---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_protected.
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
The correct commti SHA1 is c6ff5268293ef98e48a99597e765ffc417e39fa5.
Or at least, when I run:
git show f9f51b8070be3e829100614a7372b219723b864f
I get:
fatal: bad object f9f51b8070be3e829100614a7372b219723b864f
:-)
I fixed this up and applied this, thanks!
On Fri, Dec 18, 2015 at 11:42:59PM -0500, David Miller wrote:
> From: Herbert Xu <[email protected]>
> Date: Sat, 19 Dec 2015 10:45:28 +0800
>
> > On Fri, Dec 18, 2015 at 04:27:31PM -0500, David Miller wrote:
> >> From: Herbert Xu <[email protected]>
> >> 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.
> >
> > OK.
> >
> > ---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_protected.
> >
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Herbert Xu <[email protected]>
>
> The correct commti SHA1 is c6ff5268293ef98e48a99597e765ffc417e39fa5.
>
> Or at least, when I run:
>
> git show f9f51b8070be3e829100614a7372b219723b864f
>
> I get:
>
> fatal: bad object f9f51b8070be3e829100614a7372b219723b864f
>
> :-)
Oops, that commit comes from 0day robot :-)
> https://github.com/0day-ci/linux Herbert-Xu/rhashtable-Fix-walker-list-corruption/20151216-164833
> commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption")
commit f9f51b8070be3e829100614a7372b219723b864f
Author: Herbert Xu <[email protected]>
AuthorDate: Wed Dec 16 16:45:54 2015 +0800
Commit: 0day robot <[email protected]>
CommitDate: Wed Dec 16 16:48:36 2015 +0800
rhashtable: Fix walker list corruption
Thanks,
Fengguang