Re: [PATCH for-next v3 7/9] mm/slab: introduce kfree_rcu_nolock()

[Harry Yoo <harry@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4556 bytes --]



On 6/22/26 11:56 PM, XIAO WU wrote:
> Hi Harry,
> 
> On Mon, Jun 22, 2026 at 02:28:44PM +0900, Harry Yoo wrote:
>> On 6/21/26 9:29 AM, XIAO WU wrote:
>> > I was able to reproduce this in QEMU with KASAN.  The trigger is as
>> > simple as passing a large (>8KB) kmalloc buffer to the new function.
>>
>> Thanks for taking a look, but this was intentional.
>>
>> I should have documented that only kmalloc_nolock() ->
>> kfree_rcu_nolock() is allowed and kmalloc() -> kfree_rcu_nolock()
>> is not allowed (yet).
>>
>> Since kmalloc_nolock() does not support large kmalloc, the warning
>> is not supposed to trigger. That is why I added only debug warnings.
> 
> Thank you very much for taking the time to explain — I really
> appreciate it, especially since I'm still learning my way around the
> mm/ subsystem.  You are absolutely right that kmalloc_nolock() returns
> NULL for sizes above KMALLOC_MAX_CACHE_SIZE, so a proper caller using
> the kmalloc_nolock() → kfree_rcu_nolock() pairing would never hit this.
> 
> I did notice one small thing that I wanted to gently bring up, though.
> Please forgive me if I'm missing something obvious here.
> 
> When I was reading through the surrounding code to understand the
> pattern better, I noticed that kfree_nolock() — which has the same
> "only for kmalloc_nolock()" constraint (documented in the comment at
> mm/slub.c:6828-6835) — actually does check for a NULL slab:
> 
>   void kfree_nolock(const void *object)
>   {
>       ...
>       slab = virt_to_slab(object);
>       if (unlikely(!slab)) {
>           WARN_ONCE(1, "large_kmalloc is not supported by kfree_nolock()");
>           return;
>       }
>       s = slab->slab_cache;
>       ...
> 
> So kfree_nolock() gracefully returns with a warning even though it too
> expects only kmalloc_nolock() callers.  That pattern seemed really
> sensible to me — it costs almost nothing and prevents a panic if
> someone ever passes the wrong pointer (which they shouldn't, but as you
> mentioned, the constraint isn't documented on kfree_call_rcu_nolock()
> yet).
>
> I also wondered about the difference between WARN_ONCE (used in
> kfree_nolock) and VM_WARN_ON_ONCE (used in kfree_call_rcu_nolock). If
> I understand correctly, VM_WARN_ON_ONCE compiles away entirely on
> production kernels without CONFIG_DEBUG_VM, which would make the
> subsequent NULL dereference completely silent — no warning, just a
> panic.

It would crash without debug option anyway, but warnings are there to
make it easier to point what's gone wrong.

And not testing the code path at least once with debug option is a big
problem :)

> And since you mentioned that kmalloc() → kfree_rcu_nolock() support is
> planned for the future (the "yet") — wouldn't this code path need the
> NULL check at that point anyway?
> 
> I was thinking something like this would make the function consistent
> with kfree_nolock() and also make it forward-compatible with the
> planned kmalloc() support:
> 
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1266,10 +1266,16 @@ void kfree_call_rcu_nolock(struct rcu_head
> *head, void *ptr)
>  {
>      struct slab *slab;
>      struct kmem_cache *s;
> 
> -    VM_WARN_ON_ONCE(is_vmalloc_addr(ptr) || !virt_to_slab(ptr));
> -
>      slab = virt_to_slab(ptr);
> +    /*
> +     * kmalloc_nolock() never produces large-kmalloc or vmalloc
> +     * addresses, but be defensive: fall back to defer_kfree_rcu()
> +     * for unsupported pointer types, consistent with kfree_nolock().
> +     */
> +    if (unlikely(!slab))
> +        goto fallback;

Just FYI, virt_to_slab() and virt_to_page()
don't work correctly for vmalloc addresses.

And I don't think silently making it work is good.

> +
>      s = slab->slab_cache;
> 
>      if (__kfree_rcu_sheaf(s, ptr, /* allow_spin = */ false))
>          return;
> 
> +fallback:
>      defer_kfree_rcu(head);
>  }
> 
> Of course, this is just a suggestion — you know this code far better
> than I do.  If you feel the current code is fine as-is with proper
> documentation, I completely understand and won't press the point
> further.
> 
> Either way, thank you again for the explanation, and for working on
> this series — having kfree_rcu_nolock() available for BPF and other
> contexts will be really valuable.
> 
> Thanks,
> XIAO

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]