[PATCH net 0/2] rhashtable rehashing fixes

Some rhashtable rehashing bugs found while testing with the
next rhashtable self-test queued up for the next devel cycle:

https://github.com/tgraf/net-next/commits/rht

Thomas Graf (2):
  rhashtable: Schedule async resize when sync realloc fails
  rhashtable: Do not schedule more than one rehash if we can't grow
    further

 include/linux/rhashtable.h | 2 ++
 lib/rhashtable.c           | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.3.5

When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
we can't allocate the necessary memory in the current context but the
limits as set by the user would still allow to grow.

Thus attempt an async resize in the background where we can allocate
using GFP_KERNEL which is more likely to succeed. The insertion itself
will still fail to indicate pressure.

This fixes a bug where the table would never continue growing once the
utilization is above 100%.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 include/linux/rhashtable.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index e23d242..7040b5c 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h

@@ -593,6 +593,8 @@ slow_path:
 		spin_unlock_bh(lock);
 		err = rhashtable_insert_rehash(ht);
 		rcu_read_unlock();
+		if (err == -ENOMEM)
+			schedule_work(&ht->run_work);
 		if (err)
 			return err;
 

-- 
2.3.5

The current code currently only stops inserting rehashes into the
chain when no resizes are currently scheduled. As long as resizes
are scheduled and while inserting above the utilization watermark,
more and more rehashes will be scheduled.

This lead to a perfect DoS storm with thousands of rehashes
scheduled which lead to thousands of spinlocks to be taken
sequentially.

Instead, only allow either a series of resizes or a single rehash.
Drop any further rehashes and return -EBUSY.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 lib/rhashtable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 4898442..cb819ed 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c

@@ -405,8 +405,8 @@ int rhashtable_insert_rehash(struct rhashtable *ht)
 
 	if (rht_grow_above_75(ht, tbl))
 		size *= 2;
-	/* More than two rehashes (not resizes) detected. */
-	else if (WARN_ON(old_tbl != tbl && old_tbl->size == size))
+	/* Do not schedule more than one rehash */
+	else if (old_tbl != tbl)
 		return -EBUSY;
 
 	new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC);

-- 
2.3.5

On Tue, Apr 21, 2015 at 02:55:34PM +0200, Thomas Graf wrote:

When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
we can't allocate the necessary memory in the current context but the
limits as set by the user would still allow to grow.

Thus attempt an async resize in the background where we can allocate
using GFP_KERNEL which is more likely to succeed. The insertion itself
will still fail to indicate pressure.

This fixes a bug where the table would never continue growing once the
utilization is above 100%.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>

Good catch.  But I think this call should happen in
rhashtable_insert_rehash since it's on the slow-path.

Thanks,
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Tue, Apr 21, 2015 at 02:55:35PM +0200, Thomas Graf wrote:

The current code currently only stops inserting rehashes into the
chain when no resizes are currently scheduled. As long as resizes
are scheduled and while inserting above the utilization watermark,
more and more rehashes will be scheduled.

This lead to a perfect DoS storm with thousands of rehashes
scheduled which lead to thousands of spinlocks to be taken
sequentially.

Instead, only allow either a series of resizes or a single rehash.
Drop any further rehashes and return -EBUSY.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 22 Apr 2015 08:36:34 +0800

On Tue, Apr 21, 2015 at 02:55:34PM +0200, Thomas Graf wrote:

quoted

When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
we can't allocate the necessary memory in the current context but the
limits as set by the user would still allow to grow.

Thus attempt an async resize in the background where we can allocate
using GFP_KERNEL which is more likely to succeed. The insertion itself
will still fail to indicate pressure.

This fixes a bug where the table would never continue growing once the
utilization is above 100%.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>

Good catch.  But I think this call should happen in
rhashtable_insert_rehash since it's on the slow-path.

Ok, then I expect a respin of this series.

On 04/21/15 at 10:10pm, David Miller wrote:

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 22 Apr 2015 08:36:34 +0800

quoted

On Tue, Apr 21, 2015 at 02:55:34PM +0200, Thomas Graf wrote:

quoted

When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
we can't allocate the necessary memory in the current context but the
limits as set by the user would still allow to grow.

Thus attempt an async resize in the background where we can allocate
using GFP_KERNEL which is more likely to succeed. The insertion itself
will still fail to indicate pressure.

This fixes a bug where the table would never continue growing once the
utilization is above 100%.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>

Good catch.  But I think this call should happen in
rhashtable_insert_rehash since it's on the slow-path.

Ok, then I expect a respin of this series.

Agreed, respinning.