Message ID | 1452020494-20349-1-git-send-email-kamal@canonical.com |
---|---|
State | New |
Headers | show |
Hi Kamal, I believe you also need commit 179ccc0a73641ffd24e44ff10a7bd494efe98d8d ("rhashtable: Kill harmless RCU warning in rhashtable_walk_init") to go with that fix just because we got a kernel build bot warning on commit , see 70588ecea38070fe1f92ce5aafe97545c3463f7ehttps://bugs.launchpad.net/ubuntu/+source/linux/+bug/1526811/comments/3/+download I've queued these up for a SRU, namely bug LP#1526811, so I'm not sure how we deal with these kind of stable fixes that have been SRU'd. Colin On 05/01/16 19:01, Kamal Mostafa wrote: > This is a note to let you know that I have just added a patch titled > > rhashtable: Fix walker list corruption > > to the linux-4.2.y-queue branch of the 4.2.y-ckt extended stable tree > which can be found at: > > http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-4.2.y-queue > > This patch is scheduled to be released in version 4.2.8-ckt1. > > If you, or anyone else, feels it should not be added to this tree, please > reply to this email. > > For more information about the 4.2.y-ckt tree, see > https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable > > Thanks. > -Kamal > > ------ > > From 70588ecea38070fe1f92ce5aafe97545c3463f7e Mon Sep 17 00:00:00 2001 > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Wed, 16 Dec 2015 16:45:54 +0800 > Subject: rhashtable: Fix walker list corruption > > [ Upstream commit c6ff5268293ef98e48a99597e765ffc417e39fa5 ] > > The commit ba7c95ea3870fe7b847466d39a049ab6f156aa2c ("rhashtable: > Fix sleeping inside RCU critical section in walk_stop") introduced > a new spinlock for the walker list. However, it did not convert > all existing users of the list over to the new spin lock. Some > continued to use the old mutext for this purpose. This obviously > led to corruption of the list. > > The fix is to use the spin lock everywhere where we touch the list. > > This also allows us to do rcu_rad_lock before we take the lock in > rhashtable_walk_start. With the old mutex this would've deadlocked > but it's safe with the new spin lock. > > Fixes: ba7c95ea3870 ("rhashtable: Fix sleeping inside RCU...") > Reported-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: David S. Miller <davem@davemloft.net> > Signed-off-by: Kamal Mostafa <kamal@canonical.com> > --- > lib/rhashtable.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index 7d79983..c321134 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -506,10 +506,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) > if (!iter->walker) > return -ENOMEM; > > - mutex_lock(&ht->mutex); > - iter->walker->tbl = rht_dereference(ht->tbl, ht); > + spin_lock(&ht->lock); > + iter->walker->tbl = > + rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); > list_add(&iter->walker->list, &iter->walker->tbl->walkers); > - mutex_unlock(&ht->mutex); > + spin_unlock(&ht->lock); > > return 0; > } > @@ -523,10 +524,10 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_init); > */ > void rhashtable_walk_exit(struct rhashtable_iter *iter) > { > - mutex_lock(&iter->ht->mutex); > + spin_lock(&iter->ht->lock); > if (iter->walker->tbl) > list_del(&iter->walker->list); > - mutex_unlock(&iter->ht->mutex); > + spin_unlock(&iter->ht->lock); > kfree(iter->walker); > } > EXPORT_SYMBOL_GPL(rhashtable_walk_exit); > @@ -550,14 +551,12 @@ int rhashtable_walk_start(struct rhashtable_iter *iter) > { > struct rhashtable *ht = iter->ht; > > - mutex_lock(&ht->mutex); > + rcu_read_lock(); > > + spin_lock(&ht->lock); > if (iter->walker->tbl) > list_del(&iter->walker->list); > - > - rcu_read_lock(); > - > - mutex_unlock(&ht->mutex); > + spin_unlock(&ht->lock); > > if (!iter->walker->tbl) { > iter->walker->tbl = rht_dereference_rcu(ht->tbl, ht); > -- > 1.9.1 >
On Wed, 2016-01-06 at 10:44 +0000, Colin Ian King wrote: > Hi Kamal, > > I believe you also need commit > 179ccc0a73641ffd24e44ff10a7bd494efe98d8d ("rhashtable: Kill harmless RCU > warning in rhashtable_walk_init") to go with that fix Hi Colin- Thanks for reviewing this -- but it turns out that 4.2-stable backport of this commit[0] actually already has that "Kill harmless RCU" change folded in. -Kamal [0] "[PATCH 34/34] rhashtable: Fix walker list corruption" from David Miller's 2015-12-22 [PATCHES] Networking (net_43.mbox) patch set. > just because we > got a kernel build bot warning on commit , see > 70588ecea38070fe1f92ce5aafe97545c3463f7ehttps://bugs.launchpad.net/ubuntu/+source/linux/+bug/1526811/comments/3/+download > > I've queued these up for a SRU, namely bug LP#1526811, so I'm not sure > how we deal with these kind of stable fixes that have been SRU'd. > > Colin > > On 05/01/16 19:01, Kamal Mostafa wrote: > > This is a note to let you know that I have just added a patch titled > > > > rhashtable: Fix walker list corruption > > > > to the linux-4.2.y-queue branch of the 4.2.y-ckt extended stable tree > > which can be found at: > > > > http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-4.2.y-queue > > > > This patch is scheduled to be released in version 4.2.8-ckt1. > > > > If you, or anyone else, feels it should not be added to this tree, please > > reply to this email. > > > > For more information about the 4.2.y-ckt tree, see > > https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable > > > > Thanks. > > -Kamal > > > > ------ > > > > From 70588ecea38070fe1f92ce5aafe97545c3463f7e Mon Sep 17 00:00:00 2001 > > From: Herbert Xu <herbert@gondor.apana.org.au> > > Date: Wed, 16 Dec 2015 16:45:54 +0800 > > Subject: rhashtable: Fix walker list corruption > > > > [ Upstream commit c6ff5268293ef98e48a99597e765ffc417e39fa5 ] > > > > The commit ba7c95ea3870fe7b847466d39a049ab6f156aa2c ("rhashtable: > > Fix sleeping inside RCU critical section in walk_stop") introduced > > a new spinlock for the walker list. However, it did not convert > > all existing users of the list over to the new spin lock. Some > > continued to use the old mutext for this purpose. This obviously > > led to corruption of the list. > > > > The fix is to use the spin lock everywhere where we touch the list. > > > > This also allows us to do rcu_rad_lock before we take the lock in > > rhashtable_walk_start. With the old mutex this would've deadlocked > > but it's safe with the new spin lock. > > > > Fixes: ba7c95ea3870 ("rhashtable: Fix sleeping inside RCU...") > > Reported-by: Colin Ian King <colin.king@canonical.com> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > Signed-off-by: Kamal Mostafa <kamal@canonical.com> > > --- > > lib/rhashtable.c | 19 +++++++++---------- > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > > index 7d79983..c321134 100644 > > --- a/lib/rhashtable.c > > +++ b/lib/rhashtable.c > > @@ -506,10 +506,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) > > if (!iter->walker) > > return -ENOMEM; > > > > - mutex_lock(&ht->mutex); > > - iter->walker->tbl = rht_dereference(ht->tbl, ht); > > + spin_lock(&ht->lock); > > + iter->walker->tbl = > > + rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); > > list_add(&iter->walker->list, &iter->walker->tbl->walkers); > > - mutex_unlock(&ht->mutex); > > + spin_unlock(&ht->lock); > > > > return 0; > > } > > @@ -523,10 +524,10 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_init); > > */ > > void rhashtable_walk_exit(struct rhashtable_iter *iter) > > { > > - mutex_lock(&iter->ht->mutex); > > + spin_lock(&iter->ht->lock); > > if (iter->walker->tbl) > > list_del(&iter->walker->list); > > - mutex_unlock(&iter->ht->mutex); > > + spin_unlock(&iter->ht->lock); > > kfree(iter->walker); > > } > > EXPORT_SYMBOL_GPL(rhashtable_walk_exit); > > @@ -550,14 +551,12 @@ int rhashtable_walk_start(struct rhashtable_iter *iter) > > { > > struct rhashtable *ht = iter->ht; > > > > - mutex_lock(&ht->mutex); > > + rcu_read_lock(); > > > > + spin_lock(&ht->lock); > > if (iter->walker->tbl) > > list_del(&iter->walker->list); > > - > > - rcu_read_lock(); > > - > > - mutex_unlock(&ht->mutex); > > + spin_unlock(&ht->lock); > > > > if (!iter->walker->tbl) { > > iter->walker->tbl = rht_dereference_rcu(ht->tbl, ht); > > -- > > 1.9.1 > > >
On 06/01/16 19:22, Kamal Mostafa wrote: > On Wed, 2016-01-06 at 10:44 +0000, Colin Ian King wrote: >> Hi Kamal, >> >> I believe you also need commit >> 179ccc0a73641ffd24e44ff10a7bd494efe98d8d ("rhashtable: Kill harmless RCU >> warning in rhashtable_walk_init") to go with that fix > > Hi Colin- > > Thanks for reviewing this -- but it turns out that 4.2-stable backport > of this commit[0] actually already has that "Kill harmless RCU" change > folded in. > > -Kamal Ah, that explains the confusion over these two patches; I hadn't appreciated they were folded together. Thanks for letting me know Kamal. Colin > > [0] "[PATCH 34/34] rhashtable: Fix walker list corruption" from David > Miller's 2015-12-22 [PATCHES] Networking (net_43.mbox) patch set. > > >> just because we >> got a kernel build bot warning on commit , see >> 70588ecea38070fe1f92ce5aafe97545c3463f7ehttps://bugs.launchpad.net/ubuntu/+source/linux/+bug/1526811/comments/3/+download >> >> I've queued these up for a SRU, namely bug LP#1526811, so I'm not sure >> how we deal with these kind of stable fixes that have been SRU'd. >> >> Colin >> >> On 05/01/16 19:01, Kamal Mostafa wrote: >>> This is a note to let you know that I have just added a patch titled >>> >>> rhashtable: Fix walker list corruption >>> >>> to the linux-4.2.y-queue branch of the 4.2.y-ckt extended stable tree >>> which can be found at: >>> >>> http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-4.2.y-queue >>> >>> This patch is scheduled to be released in version 4.2.8-ckt1. >>> >>> If you, or anyone else, feels it should not be added to this tree, please >>> reply to this email. >>> >>> For more information about the 4.2.y-ckt tree, see >>> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable >>> >>> Thanks. >>> -Kamal >>> >>> ------ >>> >>> From 70588ecea38070fe1f92ce5aafe97545c3463f7e Mon Sep 17 00:00:00 2001 >>> From: Herbert Xu <herbert@gondor.apana.org.au> >>> Date: Wed, 16 Dec 2015 16:45:54 +0800 >>> Subject: rhashtable: Fix walker list corruption >>> >>> [ Upstream commit c6ff5268293ef98e48a99597e765ffc417e39fa5 ] >>> >>> The commit ba7c95ea3870fe7b847466d39a049ab6f156aa2c ("rhashtable: >>> Fix sleeping inside RCU critical section in walk_stop") introduced >>> a new spinlock for the walker list. However, it did not convert >>> all existing users of the list over to the new spin lock. Some >>> continued to use the old mutext for this purpose. This obviously >>> led to corruption of the list. >>> >>> The fix is to use the spin lock everywhere where we touch the list. >>> >>> This also allows us to do rcu_rad_lock before we take the lock in >>> rhashtable_walk_start. With the old mutex this would've deadlocked >>> but it's safe with the new spin lock. >>> >>> Fixes: ba7c95ea3870 ("rhashtable: Fix sleeping inside RCU...") >>> Reported-by: Colin Ian King <colin.king@canonical.com> >>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> >>> Signed-off-by: David S. Miller <davem@davemloft.net> >>> Signed-off-by: Kamal Mostafa <kamal@canonical.com> >>> --- >>> lib/rhashtable.c | 19 +++++++++---------- >>> 1 file changed, 9 insertions(+), 10 deletions(-) >>> >>> diff --git a/lib/rhashtable.c b/lib/rhashtable.c >>> index 7d79983..c321134 100644 >>> --- a/lib/rhashtable.c >>> +++ b/lib/rhashtable.c >>> @@ -506,10 +506,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) >>> if (!iter->walker) >>> return -ENOMEM; >>> >>> - mutex_lock(&ht->mutex); >>> - iter->walker->tbl = rht_dereference(ht->tbl, ht); >>> + spin_lock(&ht->lock); >>> + iter->walker->tbl = >>> + rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); >>> list_add(&iter->walker->list, &iter->walker->tbl->walkers); >>> - mutex_unlock(&ht->mutex); >>> + spin_unlock(&ht->lock); >>> >>> return 0; >>> } >>> @@ -523,10 +524,10 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_init); >>> */ >>> void rhashtable_walk_exit(struct rhashtable_iter *iter) >>> { >>> - mutex_lock(&iter->ht->mutex); >>> + spin_lock(&iter->ht->lock); >>> if (iter->walker->tbl) >>> list_del(&iter->walker->list); >>> - mutex_unlock(&iter->ht->mutex); >>> + spin_unlock(&iter->ht->lock); >>> kfree(iter->walker); >>> } >>> EXPORT_SYMBOL_GPL(rhashtable_walk_exit); >>> @@ -550,14 +551,12 @@ int rhashtable_walk_start(struct rhashtable_iter *iter) >>> { >>> struct rhashtable *ht = iter->ht; >>> >>> - mutex_lock(&ht->mutex); >>> + rcu_read_lock(); >>> >>> + spin_lock(&ht->lock); >>> if (iter->walker->tbl) >>> list_del(&iter->walker->list); >>> - >>> - rcu_read_lock(); >>> - >>> - mutex_unlock(&ht->mutex); >>> + spin_unlock(&ht->lock); >>> >>> if (!iter->walker->tbl) { >>> iter->walker->tbl = rht_dereference_rcu(ht->tbl, ht); >>> -- >>> 1.9.1 >>> >> > >
From: Colin Ian King <colin.king@canonical.com> Date: Wed, 6 Jan 2016 10:44:40 +0000 > I believe you also need commit > 179ccc0a73641ffd24e44ff10a7bd494efe98d8d ("rhashtable: Kill harmless RCU > warning in rhashtable_walk_init") to go with that fix just because we > got a kernel build bot warning on commit , see > 70588ecea38070fe1f92ce5aafe97545c3463f7ehttps://bugs.launchpad.net/ubuntu/+source/linux/+bug/1526811/comments/3/+download > > I've queued these up for a SRU, namely bug LP#1526811, so I'm not sure > how we deal with these kind of stable fixes that have been SRU'd. I think in some of my backports, I merged these two commits together. But I may be wrong, I haven't checked.
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 7d79983..c321134 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -506,10 +506,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) if (!iter->walker) return -ENOMEM; - mutex_lock(&ht->mutex); - iter->walker->tbl = rht_dereference(ht->tbl, ht); + spin_lock(&ht->lock); + iter->walker->tbl = + rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); list_add(&iter->walker->list, &iter->walker->tbl->walkers); - mutex_unlock(&ht->mutex); + spin_unlock(&ht->lock); return 0; } @@ -523,10 +524,10 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_init); */ void rhashtable_walk_exit(struct rhashtable_iter *iter) { - mutex_lock(&iter->ht->mutex); + spin_lock(&iter->ht->lock); if (iter->walker->tbl) list_del(&iter->walker->list); - mutex_unlock(&iter->ht->mutex); + spin_unlock(&iter->ht->lock); kfree(iter->walker); } EXPORT_SYMBOL_GPL(rhashtable_walk_exit); @@ -550,14 +551,12 @@ int rhashtable_walk_start(struct rhashtable_iter *iter) { struct rhashtable *ht = iter->ht; - mutex_lock(&ht->mutex); + rcu_read_lock(); + spin_lock(&ht->lock); if (iter->walker->tbl) list_del(&iter->walker->list); - - rcu_read_lock(); - - mutex_unlock(&ht->mutex); + spin_unlock(&ht->lock); if (!iter->walker->tbl) { iter->walker->tbl = rht_dereference_rcu(ht->tbl, ht);