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

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

[Herbert Xu]

On Wed, Oct 28, 2026 at 11:42:18AM -0400, Liam R. Howlett (Google) wrote:
>
> Below, one CPU adds an FDB entry for an RTM_NEWNEIGH netlink request,
> while another CPU is inside vmalloc() or vfree():
> 
>     RTM_NEWNEIGH                      vmalloc() or vfree()
>     ============                      ====================
>     spin_lock_bh(&br->hash_lock)
>                                       spin_lock(&vn->busy.lock)
>     rhashtable_insert_rehash()
>       kvmalloc(GFP_ATOMIC)
>         vmalloc()
>           spin_lock(&vn->busy.lock)
>             waits for the other CPU
>                                       softirq: a frame for the bridge
>                                         spin_lock(&br->hash_lock)
>                                           waits for RTM_NEWNEIGH

I think this is a fundamental bug in the kvmalloc non-blocking
support patch.  How can it be used at all if it is susceptible
to such dead-locks?

And if you can't use it safely why don't we just get rid of the
broken kvmalloc non-block path and have it always go to kmalloc
like it did before?
 
> Fixes: c6307674ed82 ("mm: kvmalloc: add non-blocking support for vmalloc")

BTW, rhashtable patches should be sent to linux-crypto.

Thanks,
-- 
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: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash

[Uladzislau Rezki]

On Thu, Oct 29, 2026 at 10:16:36AM +0800, Herbert Xu wrote:
> On Wed, Oct 28, 2026 at 11:42:18AM -0400, Liam R. Howlett (Google) wrote:
> >
> > Below, one CPU adds an FDB entry for an RTM_NEWNEIGH netlink request,
> > while another CPU is inside vmalloc() or vfree():
> > 
> >     RTM_NEWNEIGH                      vmalloc() or vfree()
> >     ============                      ====================
> >     spin_lock_bh(&br->hash_lock)
> >                                       spin_lock(&vn->busy.lock)
> >     rhashtable_insert_rehash()
> >       kvmalloc(GFP_ATOMIC)
> >         vmalloc()
> >           spin_lock(&vn->busy.lock)
> >             waits for the other CPU
> >                                       softirq: a frame for the bridge
> >                                         spin_lock(&br->hash_lock)
> >                                           waits for RTM_NEWNEIGH
> 
> I think this is a fundamental bug in the kvmalloc non-blocking
> support patch.  How can it be used at all if it is susceptible
> to such dead-locks?
> 
> And if you can't use it safely why don't we just get rid of the
> broken kvmalloc non-block path and have it always go to kmalloc
> like it did before?
>  
If we have a look at this as we did not have vmalloc() here:


     RTM_NEWNEIGH                      foo() or ...
     ============                      ====================
     lock_bh(&br->hash_lock)
                                       foo()
                                        lock(B)
     rhashtable_insert_rehash()
         foo()
           lock(B)
             waits for the other CPU
                                       softirq: a frame for the bridge
                                         lock(&br->hash_lock)
                                           waits for RTM_NEWNEIGH

Same issue?

I have a question:

GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee
not hitting ABBA dead-locks or they are not interrupted?

--
Uladzislau Rezki

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

[Liam R. Howlett (Google)]

Sorry for missing the ML in my initial email, I missed it from
get_maintainers.  I should have really clued in with such a small cc
list ;)

On 26/10/29 11:08AM, Uladzislau Rezki wrote:
> On Thu, Oct 29, 2026 at 10:16:36AM +0800, Herbert Xu wrote:
> > On Wed, Oct 28, 2026 at 11:42:18AM -0400, Liam R. Howlett (Google) wrote:
> > >
> > > Below, one CPU adds an FDB entry for an RTM_NEWNEIGH netlink request,
> > > while another CPU is inside vmalloc() or vfree():
> > > 
> > >     RTM_NEWNEIGH                      vmalloc() or vfree()
> > >     ============                      ====================
> > >     spin_lock_bh(&br->hash_lock)
> > >                                       spin_lock(&vn->busy.lock)
> > >     rhashtable_insert_rehash()
> > >       kvmalloc(GFP_ATOMIC)
> > >         vmalloc()
> > >           spin_lock(&vn->busy.lock)
> > >             waits for the other CPU
> > >                                       softirq: a frame for the bridge
> > >                                         spin_lock(&br->hash_lock)
> > >                                           waits for RTM_NEWNEIGH
> > 
> > I think this is a fundamental bug in the kvmalloc non-blocking
> > support patch.  How can it be used at all if it is susceptible
> > to such dead-locks?
> > 
> > And if you can't use it safely why don't we just get rid of the
> > broken kvmalloc non-block path and have it always go to kmalloc
> > like it did before?
> >  
> If we have a look at this as we did not have vmalloc() here:
> 
> 
>      RTM_NEWNEIGH                      foo() or ...
>      ============                      ====================
>      lock_bh(&br->hash_lock)
>                                        foo()
>                                         lock(B)
>      rhashtable_insert_rehash()
>          foo()
>            lock(B)
>              waits for the other CPU
>                                        softirq: a frame for the bridge
>                                          lock(&br->hash_lock)
>                                            waits for RTM_NEWNEIGH
> 
> Same issue?

Looks like it to me.

> 
> I have a question:
> 
> GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee
> not hitting ABBA dead-locks or they are not interrupted?

GFP_ATOMIC and GFP_NOWAIT can be interrupted.  It's more about not
waiting for reclaim (kswapd might still run) and GFP_ATOMIC might dip
into reserves.

The issue is that kvmalloc() now falls back to vmalloc() for
non-blocking flags (I am restating this to make sure I'm not missing
something here, please correct me).

So this changes what is called internal to rhastable so it doesn't reach
vmalloc(), like before, if the caller cannot block - they go to
kmalloc() with the old fall back.

I *think* the question is if the change should be here in the caller,
internal to mm, or a new call to opt-in.

Considering this is the only kvmalloc() caller that forwards
non-blocking gfp from BH lock + softirq context, I think that it makes
sense to do it in the caller.

Given where we are in the release, this fixes a real issue that can
happen so maybe we take this now and settle the general handling later?

Thanks,
Liam

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

[Michal Hocko]

On Thu 29-10-26 10:53:56, Liam R. Howlett (Google) wrote:
> 
> Sorry for missing the ML in my initial email, I missed it from
> get_maintainers.  I should have really clued in with such a small cc
> list ;)
> 
> On 26/10/29 11:08AM, Uladzislau Rezki wrote:
> > On Thu, Oct 29, 2026 at 10:16:36AM +0800, Herbert Xu wrote:
> > > On Wed, Oct 28, 2026 at 11:42:18AM -0400, Liam R. Howlett (Google) wrote:
> > > >
> > > > Below, one CPU adds an FDB entry for an RTM_NEWNEIGH netlink request,
> > > > while another CPU is inside vmalloc() or vfree():
> > > > 
> > > >     RTM_NEWNEIGH                      vmalloc() or vfree()
> > > >     ============                      ====================
> > > >     spin_lock_bh(&br->hash_lock)
> > > >                                       spin_lock(&vn->busy.lock)
> > > >     rhashtable_insert_rehash()
> > > >       kvmalloc(GFP_ATOMIC)
> > > >         vmalloc()
> > > >           spin_lock(&vn->busy.lock)
> > > >             waits for the other CPU
> > > >                                       softirq: a frame for the bridge
> > > >                                         spin_lock(&br->hash_lock)
> > > >                                           waits for RTM_NEWNEIGH
> > > 
> > > I think this is a fundamental bug in the kvmalloc non-blocking
> > > support patch.  How can it be used at all if it is susceptible
> > > to such dead-locks?
> > > 
> > > And if you can't use it safely why don't we just get rid of the
> > > broken kvmalloc non-block path and have it always go to kmalloc
> > > like it did before?
> > >  
> > If we have a look at this as we did not have vmalloc() here:
> > 
> > 
> >      RTM_NEWNEIGH                      foo() or ...
> >      ============                      ====================
> >      lock_bh(&br->hash_lock)
> >                                        foo()
> >                                         lock(B)
> >      rhashtable_insert_rehash()
> >          foo()
> >            lock(B)
> >              waits for the other CPU
> >                                        softirq: a frame for the bridge
> >                                          lock(&br->hash_lock)
> >                                            waits for RTM_NEWNEIGH
> > 
> > Same issue?
> 
> Looks like it to me.
> 
> > 
> > I have a question:
> > 
> > GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee
> > not hitting ABBA dead-locks or they are not interrupted?
> 
> GFP_ATOMIC and GFP_NOWAIT can be interrupted.  It's more about not
> waiting for reclaim (kswapd might still run) and GFP_ATOMIC might dip
> into reserves.
> 
> The issue is that kvmalloc() now falls back to vmalloc() for
> non-blocking flags (I am restating this to make sure I'm not missing
> something here, please correct me).

Isn't the real problem that vmalloc should use irqsave version of the
spinlock to preserve the locking constrains of the caller?
-- 
Michal Hocko
SUSE Labs

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

[Herbert Xu]

On Thu, Oct 29, 2026 at 10:53:56AM -0400, Liam R. Howlett (Google) wrote:
>
> Given where we are in the release, this fixes a real issue that can
> happen so maybe we take this now and settle the general handling later?

There is no rush at all since the bug that you found has existed
since 2025:

commit c6307674ed82c0c57d6e1e3408e84ac449ab8e94
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Tue Oct 7 14:20:35 2025 +0200

    mm: kvmalloc: add non-blocking support for vmalloc

Let's take the time to fix this properly.

Thanks,
-- 
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: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash

[Uladzislau Rezki]

> On Thu, Oct 29, 2026 at 10:53:56AM -0400, Liam R. Howlett (Google) wrote:
> >
> > Given where we are in the release, this fixes a real issue that can
> > happen so maybe we take this now and settle the general handling later?
> 
> There is no rush at all since the bug that you found has existed
> since 2025:
> 
> commit c6307674ed82c0c57d6e1e3408e84ac449ab8e94
> Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Date:   Tue Oct 7 14:20:35 2025 +0200
> 
>     mm: kvmalloc: add non-blocking support for vmalloc
> 
> Let's take the time to fix this properly.
> 
The problem with spin_lock_irqsave() is that vmalloc() path can take
time to alloc and map pages. I am not sure this is worth to switch to
irq-save locking primitive. So this is the "heaviest" allocator in the
kernel. Callers have to be careful how the call vmalloc().

IMO, this caller has to be fixed.

--
Uladzislau Rezki

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

[Matthew Wilcox]

On Fri, Oct 30, 2026 at 10:52:37AM +0100, Uladzislau Rezki wrote:
> The problem with spin_lock_irqsave() is that vmalloc() path can take
> time to alloc and map pages. I am not sure this is worth to switch to
> irq-save locking primitive. So this is the "heaviest" allocator in the
> kernel. Callers have to be careful how the call vmalloc().
> 
> IMO, this caller has to be fixed.

Which caller, though?  Are you saying rhashtable_insert_rehash()
needs to call kmalloc() instead of kvmalloc()?