Re: [PATCH] mm/slab: don't use kfree_rcu sheaves on PREEMPT_RT in kvfree_call_rcu()

[sashiko-bot]

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

New issues:
- [Medium] The commit message and comments incorrectly recommend using kfree_call_rcu_nolock() under a raw spinlock on PREEMPT_RT, but doing so will trigger the exact same lockdep violation.
- [Low] The patch introduces unreachable dead code inside `kfree_rcu_sheaf()`.

Pre-existing issues:
- [High] Headless kfree_rcu() calls might_sleep() and synchronize_rcu(), making it unsafe to call under a raw spinlock.
--

commit 766d83bf3f72fb4bed74c42f068169aae388d06f
Author: ThangNN99 <ngocthang2710.1999@xxxxxxxxx>
mm/slab: don't use kfree_rcu sheaves on PREEMPT_RT in kvfree_call_rcu()

This patch skips the sheaves fast path on PREEMPT_RT in kvfree_call_rcu()
to avoid a lockdep circular dependency when the caller holds a raw
spinlock. It modifies kvfree_call_rcu() to avoid calling into the sheaf
machinery when PREEMPT_RT is enabled, falling back to the krcp list. The
commit message and comments also suggest raw spinlock callers needing
the fast path should migrate to kfree_call_rcu_nolock() instead.

Reported-by: syzbot+acf142088e0182172e58@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=acf142088e0182172e58
Fixes: 2a8bb29ec9b2 ("mm/slab: allow kfree_rcu_sheaf() on PREEMPT_RT")

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index b19ba1b31484c..6d6cd78d00c4b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -2034,7 +2034,14 @@ void kvfree_call_rcu(struct kvfree_rcu_head *head, void *ptr)
>  	if (!head)
>  		might_sleep();
>  

[Severity: High]
This is a pre-existing issue, but does using headless kfree_rcu() under a raw
spinlock actually avoid sleeping?

The commit message states that set_cpus_allowed_force() uses kfree_rcu()
because it holds the raw p->pi_lock and cannot sleep. However, if head is
NULL (which is the case for the single-argument kfree_rcu() macro), this
might_sleep() is unconditionally called. Does this mean set_cpus_allowed_force()
is actually triggering a "sleeping function called from invalid context" bug?

> -	if (kfree_rcu_sheaf(ptr))
> +	/*
> +	 * Callers may hold a raw spinlock here on PREEMPT_RT (e.g.
> +	 * set_cpus_allowed_force() with p->pi_lock held), and the sheaf/barn
> +	 * locks are also taken as blocking locks elsewhere, so trying them
> +	 * here creates a lockdep-visible ordering conflict. Skip sheaves on
> +	 * PREEMPT_RT; use kfree_rcu_nolock() instead if this doesn't apply.
> +	 */
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT) && kfree_rcu_sheaf(ptr))

[Severity: Medium]
Will callers migrating to kfree_call_rcu_nolock() on PREEMPT_RT hit the same
issue this patch is trying to fix?

Looking at kfree_call_rcu_nolock(), it unconditionally calls
__kfree_rcu_sheaf() with SLAB_FREE_NOLOCK:

bool kfree_call_rcu_nolock(void *ptr)
{
    ...
    if (unlikely(!__kfree_rcu_sheaf(slab->slab_cache, ptr, SLAB_FREE_NOLOCK)))
    ...
}

This in turn calls local_trylock(&s->cpu_sheaves->lock). On PREEMPT_RT, this
lock is a sleepable spinlock_t. Doesn't acquiring a sleepable lock under a raw
spinlock violate lock nesting rules, even with a trylock?

[Severity: Low]
Does skipping kfree_rcu_sheaf() when PREEMPT_RT is enabled turn the early
check inside kfree_rcu_sheaf() into unreachable dead code?

kfree_rcu_sheaf() has an early check:

static bool kfree_rcu_sheaf(void *obj)
{
    ...
    if (IS_ENABLED(CONFIG_PREEMPT_RT))
        free_flags = SLAB_FREE_NOLOCK;
    ...
}

Since kvfree_call_rcu() is the only caller of kfree_rcu_sheaf(), that code
appears to be completely unreachable now.

>  		return;
>  

[ ... ]

> diff --git a/mm/slub.c b/mm/slub.c
> index f9b56cb439e70..83bc322557f89 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6088,10 +6088,11 @@ static void rcu_free_sheaf(struct rcu_head *head)
>  /*
>   * kvfree_call_rcu() can be called while holding a raw_spinlock_t. Since
>   * __kfree_rcu_sheaf() may acquire a spinlock_t (sleeping lock on PREEMPT_RT),
> - * this would violate lock nesting rules. Therefore, kvfree_call_rcu() avoids
> - * this problem by passing SLAB_FREE_NOLOCK on PREEMPT_RT.
> + * this would violate lock nesting rules. kvfree_call_rcu() avoids this by
> + * bypassing the sheaves layer on PREEMPT_RT; use kfree_call_rcu_nolock()
> + * instead for atomic/unknown-context callers that need the sheaves path.
>   *
> - * However, lockdep still complains that it is invalid to acquire spinlock_t
> + * lockdep still complains that it is invalid to acquire spinlock_t

[Severity: Medium]
As mentioned above, if callers follow this comment and use
kfree_call_rcu_nolock() under a raw spinlock on PREEMPT_RT, won't lockdep
still complain because kfree_call_rcu_nolock() will call __kfree_rcu_sheaf()
which attempts to acquire the sleepable spinlock_t?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831053846.107974-1-ngocthang2710.1999@xxxxxxxxx?part=1