rhashtable: Fix potential crash on destroy in rhashtable_shrink

rhashtable: Fix potential crash on destroy in rhashtable_shrink

[Herbert Xu]

The current being_destroyed check in rhashtable_expand is not
enough since if we start a shrinking process after freeing all
elements in the table that's also going to crash.

This patch adds a being_destroyed check to the deferred worker
thread so that we bail out as soon as we take the lock.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 69a4eb0..4c3da1f 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -489,6 +489,9 @@ static void rht_deferred_worker(struct work_struct *work)
 
 	ht = container_of(work, struct rhashtable, run_work);
 	mutex_lock(&ht->mutex);
+	if (ht->being_destroyed)
+		goto unlock;
+
 	tbl = rht_dereference(ht->tbl, ht);
 
 	list_for_each_entry(walker, &ht->walkers, list)
@@ -499,6 +502,7 @@ static void rht_deferred_worker(struct work_struct *work)
 	else if (ht->p.shrink_decision && ht->p.shrink_decision(ht, tbl->size))
 		rhashtable_shrink(ht);
 
+unlock:
 	mutex_unlock(&ht->mutex);
 }
 
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: rhashtable: Fix potential crash on destroy in rhashtable_shrink

[Thomas Graf]

On 01/31/15 at 08:36pm, Herbert Xu wrote:
> The current being_destroyed check in rhashtable_expand is not
> enough since if we start a shrinking process after freeing all
> elements in the table that's also going to crash.

(The check in expand() is just an optimization to drop out of
 work cycles if it does not make sense to continue anymore.)

> 
> This patch adds a being_destroyed check to the deferred worker
> thread so that we bail out as soon as we take the lock.

Shouldn't the cancel_work_sync() in rhashtable_destroy() block
until the deferred worker is done and cancelled?

Re: rhashtable: Fix potential crash on destroy in rhashtable_shrink

[Herbert Xu]

On Sat, Jan 31, 2015 at 11:16:52AM +0000, Thomas Graf wrote:
> On 01/31/15 at 08:36pm, Herbert Xu wrote:
> > The current being_destroyed check in rhashtable_expand is not
> > enough since if we start a shrinking process after freeing all
> > elements in the table that's also going to crash.
> 
> (The check in expand() is just an optimization to drop out of
>  work cycles if it does not make sense to continue anymore.)
> 
> > 
> > This patch adds a being_destroyed check to the deferred worker
> > thread so that we bail out as soon as we take the lock.
> 
> Shouldn't the cancel_work_sync() in rhashtable_destroy() block
> until the deferred worker is done and cancelled?

That's too late.  nft_hash will have freed all the elements
before rhashtable_destroy gets called.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: rhashtable: Fix potential crash on destroy in rhashtable_shrink

[Thomas Graf]

On 01/31/15 at 10:22pm, Herbert Xu wrote:
> That's too late.  nft_hash will have freed all the elements
> before rhashtable_destroy gets called.

I see, so this is to accomodate nft_hash which doesn't remove
the elements from the hash but just frees them.

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: rhashtable: Fix potential crash on destroy in rhashtable_shrink

[Ying Xue]

On 01/31/2015 05:36 PM, Herbert Xu wrote:
> The current being_destroyed check in rhashtable_expand is not
> enough since if we start a shrinking process after freeing all
> elements in the table that's also going to crash.
> 

Sorry, I cannot understand the scenario.

When we free the table in rhashtable_destroy(), we call
cancel_work_sync() to synchronously cancel the work. So, why does your
described crash still happen?

Please give a more explanation.

Thanks,
Ying

> This patch adds a being_destroyed check to the deferred worker
> thread so that we bail out as soon as we take the lock.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index 69a4eb0..4c3da1f 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -489,6 +489,9 @@ static void rht_deferred_worker(struct work_struct *work)
>  
>  	ht = container_of(work, struct rhashtable, run_work);
>  	mutex_lock(&ht->mutex);
> +	if (ht->being_destroyed)
> +		goto unlock;
> +
>  	tbl = rht_dereference(ht->tbl, ht);
>  
>  	list_for_each_entry(walker, &ht->walkers, list)
> @@ -499,6 +502,7 @@ static void rht_deferred_worker(struct work_struct *work)
>  	else if (ht->p.shrink_decision && ht->p.shrink_decision(ht, tbl->size))
>  		rhashtable_shrink(ht);
>  
> +unlock:
>  	mutex_unlock(&ht->mutex);
>  }
>  
> 

Re: rhashtable: Fix potential crash on destroy in rhashtable_shrink

[Thomas Graf]

On 02/02/15 at 05:34pm, Ying Xue wrote:
> On 01/31/2015 05:36 PM, Herbert Xu wrote:
> > The current being_destroyed check in rhashtable_expand is not
> > enough since if we start a shrinking process after freeing all
> > elements in the table that's also going to crash.
> > 
> 
> Sorry, I cannot understand the scenario.
> 
> When we free the table in rhashtable_destroy(), we call
> cancel_work_sync() to synchronously cancel the work. So, why does your
> described crash still happen?

It's nft_hash specific. nft_hash frees all entries under ht->mutex without
unlinking them upon destroy and sets being_destroyed before doing. Therefore
it must be ensured that a resize is not started afterwards because the table
contains freed entries.

Re: rhashtable: Fix potential crash on destroy in rhashtable_shrink

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 31 Jan 2015 20:36:38 +1100

> The current being_destroyed check in rhashtable_expand is not
> enough since if we start a shrinking process after freeing all
> elements in the table that's also going to crash.
> 
> This patch adds a being_destroyed check to the deferred worker
> thread so that we bail out as soon as we take the lock.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied to net-next