Re: [PATCH RFC 4/5] mm/slab: handle large_kmalloc objects in kfree_nolock()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Synchronous execution of __free_large_kmalloc_page in kfree_nolock() violates NMI wait-context and corrupts vmstat counters
- [High] Deferred clearing of PageLargeKmalloc bypasses double-free protection, causing infinite loops
--

--- Patch [4]: [PATCH RFC 4/5] mm/slab: handle large_kmalloc objects in kfree_nolock() ---
commit 9af1d6506b418b2d9de0922f293e12ee55babdd3
Author: Vlastimil Babka (SUSE) <[email protected]>

mm/slab: handle large_kmalloc objects in kfree_nolock()
    
Large kmalloc objects is the only remaining case that kfree_nolock()
cannot handle from kmalloc() allocations. Note kmalloc_nolock() does not
return large kmalloc objects.

> diff --git a/mm/slub.c b/mm/slub.c
> index 423b5bdb910bd..3be98faa9f0de 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c

[ ... ]

> @@ -6365,6 +6367,21 @@ static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
>  	}
>  }
>  
> +static inline void
> +__free_large_kmalloc_page(struct page *page, unsigned int free_flags)
> +{
> +	unsigned int order = compound_order(page);
> +
> +	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B,
> +			      -(PAGE_SIZE << order));
> +	__ClearPageLargeKmalloc(page);
> +
> +	if (free_flags & SLAB_FREE_NOLOCK)
> +		free_frozen_pages_nolock(page, order);

[Severity: High]
Is it safe to execute this synchronously in NMI context on PREEMPT_RT kernels?

When kmemleak is disabled, kmemleak_may_need_free() returns false, which
causes free_large_kmalloc() to call __free_large_kmalloc_page() synchronously
from kfree_nolock(), even in NMI context.

This results in free_frozen_pages_nolock() attempting to acquire the zone
lock, which is an rt_mutex (sleeping lock) on PREEMPT_RT, leading to a panic
when called from NMI.

Additionally, mod_lruvec_page_state() uses a non-atomic read-modify-write
protected by local_irq_save() on architectures without
CONFIG_HAVE_CMPXCHG_LOCAL. This fails to mask NMIs and can silently corrupt
vmstat counters.

Would it be better to always defer freeing large kmalloc pages in
kfree_nolock() to avoid these issues?

> +	else
> +		free_frozen_pages(page, order);
> +}
> +

[ ... ]

> @@ -6712,9 +6756,11 @@ size_t ksize(const void *objp)
>  }
>  EXPORT_SYMBOL(ksize);
>  
> -static void free_large_kmalloc(struct page *page, void *object)
> +static void free_large_kmalloc(struct page *page, void *object,
> +			       unsigned int free_flags)
>  {
>  	unsigned int order = compound_order(page);
> +	bool nolock = free_flags & SLAB_FREE_NOLOCK;
>  
>  	if (WARN_ON_ONCE(!PageLargeKmalloc(page))) {
>  		dump_page(page, "Not a kmalloc allocation");
> @@ -6724,14 +6770,16 @@ static void free_large_kmalloc(struct page *page, void *object)
>  	if (WARN_ON_ONCE(order == 0))
>  		pr_warn_once("object pointer: 0x%p\n", object);
>  
> -	kmemleak_free(object);
> +	if (!nolock)
> +		kmemleak_free(object);
> +
>  	kasan_kfree_large(object);
>  	kmsan_kfree_large(object);
>  
> -	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B,
> -			      -(PAGE_SIZE << order));
> -	__ClearPageLargeKmalloc(page);
> -	free_frozen_pages(page, order);
> +	if (unlikely(nolock && kmemleak_may_need_free(object)))
> +		defer_free_large_kmalloc(object);

[Severity: High]
Could moving the __ClearPageLargeKmalloc() call to the asynchronous
__free_large_kmalloc_page() bypass double-free protection?

When a free is deferred via defer_free_large_kmalloc(), the PageLargeKmalloc
flag remains set until the IRQ work executes. If the caller has a double-free
bug and kfree_nolock() is called again on the same object during this window,
the WARN_ON_ONCE(!PageLargeKmalloc(page)) check at the start of
free_large_kmalloc() will pass.

This allows the second free to proceed and call defer_free_large_kmalloc()
again. Since this uses llist_add() with the same llnode, it creates a circular
linked list (llnode->next = llnode). When deferred_percpu_work_fn() later
processes this list with llist_for_each_safe(), it will loop infinitely and
cause a hard lockup.

> +	else
> +		__free_large_kmalloc_page(page, free_flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4