Re: [PATCH slab/for-next v4 5/8] mm/slab: allow kfree_rcu_sheaf() on PREEMPT_RT

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Calling `local_trylock()` inside `kvfree_call_rcu()` on PREEMPT_RT violates lockdep wait-type nesting rules and is unsafe in hardirq/NMI context.

Pre-existing issues:
- [High] Preexisting bug: Using GFP_NOWAIT in __kfree_rcu_sheaf() can cause lock recursion deadlocks when called under scheduler locks.
--

--- Patch [5]: [PATCH slab/for-next v4 5/8] mm/slab: allow kfree_rcu_sheaf() on PREEMPT_RT ---
commit 9ccf08cd866ca28e999f59be7998db3bfc32fb7a
Author: Harry Yoo (Oracle) <[email protected]>
Subject: mm/slab: allow kfree_rcu_sheaf() on PREEMPT_RT

This patch allows kfree_rcu_sheaf() to be used on PREEMPT_RT by setting
SLAB_FREE_NOLOCK when PREEMPT_RT is enabled. It relaxes the limitation
in kvfree_call_rcu() and skips the lockdep wait-type override map on
PREEMPT_RT to avoid suppressing valid warnings.

Link: https://lore.kernel.org/linux-mm/[email protected] [1]
Link: https://lore.kernel.org/linux-mm/[email protected] [2]

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 9c9ae1384f476..8c631bf97cd5b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
[ ... ]
> @@ -1954,7 +1962,7 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
>  	if (!head)
>  		might_sleep();
>  
> -	if (!IS_ENABLED(CONFIG_PREEMPT_RT) && kfree_rcu_sheaf(ptr))
> +	if (kfree_rcu_sheaf(ptr))
>  		return;
>  
>  	// Queue the object but don't yet schedule the batch.
> diff --git a/mm/slub.c b/mm/slub.c
> index 8afa6b47b1f2b..deac315d0f23e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
[ ... ]
> @@ -6080,10 +6081,10 @@ bool __kfree_rcu_sheaf(struct kmem_cache *s, void *obj, unsigned int free_flags)
>  	struct slab_sheaf *rcu_sheaf;
>  	bool allow_spin = free_flags_allow_spinning(free_flags);
>  
> -	if (WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_RT)))
> -		return false;
> +	VM_WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_RT) && allow_spin);
>  
> -	lock_map_acquire_try(&kfree_rcu_sheaf_map);
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +		lock_map_acquire_try(&kfree_rcu_sheaf_map);
>  
>  	if (!local_trylock(&s->cpu_sheaves->lock))
             ^^^^^^^^^^^^^

[Severity: High]
Will this cause lockdep wait-type violations and potential crashes on
PREEMPT_RT?

Since this patch removes the !IS_ENABLED(CONFIG_PREEMPT_RT) guard in
kvfree_call_rcu() inside mm/slab_common.c, kvfree_call_rcu() can now reach
this path on PREEMPT_RT.

Because kvfree_call_rcu() can be called while holding a raw spinlock (like
the scheduler's pi_lock) or in hardirq context, it seems this local_trylock()
will attempt to acquire a sleeping lock (rt_mutex on PREEMPT_RT) in an
invalid context.

Since the lockdep override map is intentionally skipped for PREEMPT_RT just
above, will this correctly trigger lockdep "invalid wait context" splats
and potentially cause panics if the trylock is attempted from hardirq/NMI
context?

[Severity: High]
This is a pre-existing issue, but since we are enabling this path for
PREEMPT_RT, does the use of GFP_NOWAIT later in this function cause
lock recursion deadlocks?

When the fast path fails, __kfree_rcu_sheaf() calls:

    empty = alloc_empty_sheaf(s, GFP_NOWAIT, alloc_flags);

GFP_NOWAIT includes __GFP_KSWAPD_RECLAIM, which invokes wakeup_kswapd().
The wake-up path enters the scheduler via try_to_wake_up() and attempts to
acquire scheduler locks like pi_lock and the runqueue lock.

If kvfree_call_rcu() was called while the CPU already holds these scheduler
locks, will this wake-up attempt cause a lock recursion self-deadlock?

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