diff mbox

[4.2.y-ckt,stable] Patch "rhashtable: Fix walker list corruption" has been added to staging queue

Message ID 1452020494-20349-1-git-send-email-kamal@canonical.com
State New
Headers show

Commit Message

Kamal Mostafa Jan. 5, 2016, 7:01 p.m. UTC 
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(-)

--
1.9.1

Comments

Colin Ian King Jan. 6, 2016, 10:44 a.m. UTC | #1 
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
>
Kamal Mostafa Jan. 6, 2016, 7:22 p.m. UTC | #2 
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
> > 
>
Colin Ian King Jan. 6, 2016, 7:23 p.m. UTC | #3 
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
>>>
>>
> 
>
David Miller Jan. 6, 2016, 7:33 p.m. UTC | #4 
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 mbox

Patch

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