Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash

[Herbert Xu]

On Mon, Nov 02, 2026 at 08:49:23PM -0500, Liam R. Howlett (Google) wrote:
>
> I don't follow.  Reverting that patch gives the same functional flow as
> the patch I already sent; kzalloc() for one branch and kvzalloc() for
> the other. 

I'm not talking about a literal revert.  But that patch is what
introduced the use of an unconditional kvzalloc for the !GFP_KERNEL
case which is what we are trying to undo.

So it should at least be mentioned prominently in the commit
description.

> We will, however, restore the useless __GFP_NORETRY flag to one branch,
> and a broken check.
> 
> -       if (gfp != GFP_KERNEL)
> -               tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
> 
> gfp != GFP_KERNEL is supposed to just send GFP_ATOMIC in here, but
> instead you are sending GFP_KERNEL | __GFP_NOFAIL as well.
> gfpflags_allow_blocking() should be used here, right?

That made me dig deeper into the original change and why it was
made.  It was actually Linus who suggested merging the two calls.
It's a shame that we have to bring this pattern back.

https://lore.kernel.org/all/CA+55aFxnrwFniJq6nCFn4pWRHLH-Wac9bVFKieBC-TixTsFg=Q@xxxxxxxxxxxxxx/

Let's add him to the discussion and see if he can offer any insights
on how we can avoid undoing this change.

To recap, kvmalloc was modified in 2025 such that it can use vmalloc
even when called with GFP_ATOMIC (commit c6307674ed82).  Unfortunately,
it does so in a way where it has to wait for other CPUs to do certain
work.  As a result, if you call kvmalloc with GFP_ATOMIC under an
external lock, this easily leads to a dead-lock when other CPUs end
up blocking on the same external lock.

Hence rhashtable is being asked to undo the change and reintroduce
this pattern to avoid the dead-lock:

	if (!gfpflags_allow_blocking(gfp))
		tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
	else
		tbl = kvzalloc(size, gfp);

My feeling is that the new kvmalloc is simply broken but its author
disagrees.

> You are saying to add it to both branches of the if statement, right?

Nevermind, I didn't look at how alloc_hooks_tag was implemented.
This is simply disgusting.

But instead of

	if (gfpflags_allow_blocking(gfp))
		tbl = alloc_hooks_tag(ht->alloc_tag,
				      kvmalloc_node_align_noprof(
					struct_size(tbl, buckets, nbuckets),
					1, gfp | __GFP_ZERO, NUMA_NO_NODE));
	else
		tbl = alloc_hooks_tag(ht->alloc_tag,
				      kmalloc_noprof(
					struct_size(tbl, buckets, nbuckets),
					gfp | __GFP_ZERO));

, plesae do

	if (gfpflags_allow_blocking(gfp))
		tbl = alloc_hooks_tag(ht->alloc_tag,
				      kvmalloc_noprof(
				        struct_size(tbl, buckets, nbuckets),
				        gfp | __GFP_ZERO));
	else
		tbl = alloc_hooks_tag(ht->alloc_tag,
				      kzalloc_noprof(
				        struct_size(tbl, buckets, nbuckets),
				        gfp));

It would be even nicer if you could add a kvzalloc_noprof helper but
it's not a deal-breaker.

> The fixes tag should still point to c6307674ed82, since that's when the
> issue became visible.

Sure that sounds good.

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