Re: [PATCH v4 1/6] rcu: Make call_rcu() safe to call from any context

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deferred callbacks bypass `debug_rcu_head_queue()` check, causing immediate list corruption on double `call_rcu()` and defeating `CONFIG_DEBUG_OBJECTS_RCU_HEAD`.
--

--- Patch [1]: [PATCH v4 1/6] rcu: Make call_rcu() safe to call from any context ---
commit 5205f0afe602a3f93ee865a26d94981f93553b7d
Author: Puranjay Mohan <[email protected]>

rcu: Make call_rcu() safe to call from any context

This patch defers call_rcu() via a per-CPU llist and irq_work when called
with interrupts disabled. This avoids deadlocks or list corruption if
call_rcu() is invoked from NMI or instrumentation.

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 96848fc1f02b8..ff9a2395c9e8e 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c

[ ... ]

> -static void
> -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
> +/*
> + * Also called by __rcu_defer_drain() to re-issue a deferred callback, so it
> + * must not re-check the deferral condition.  Either caller may have interrupts
> + * already disabled, and a drain of a remote CPU re-issues onto the draining
> + * CPU.
> + */
> +static void rcu_do_enqueue(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
>  {
>  	static atomic_t doublefrees;
>  	unsigned long flags;
>  	bool lazy;
>  	struct rcu_data *rdp;
>  
> -	/* Misaligned rcu_head! */
> -	WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1));
> -
> -	/* Avoid NULL dereference if callback is NULL. */
> -	if (WARN_ON_ONCE(!func))
> -		return;
> -
>  	if (debug_rcu_head_queue(head)) {

[ ... ]

> +static void call_rcu_defer(struct rcu_head *head, rcu_callback_t func)
> +{
> +	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> +
> +	/*
> +	 * Instrumentation on the enqueue path can re-enter here from inside the
> +	 * drain.  Re-queuing would livelock it, so drop the callback; an NMI
> +	 * cannot loop, so let it through.
> +	 */
> +	if (READ_ONCE(rdp->defer_draining) && !in_nmi()) {
> +		WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU),
> +			  "call_rcu() re-entered during callback drain; leaking callback\n");
> +		return;
> +	}
> +	head->func = func;
> +	if (llist_add((struct llist_node *)head, &rdp->defer_head))
> +		irq_work_queue(&rdp->defer_work);
> +}

[ ... ]

> +static void
> +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
> +{
> +	/* Misaligned rcu_head! */
> +	WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1));
> +
> +	/* Avoid NULL dereference if callback is NULL. */
> +	if (WARN_ON_ONCE(!func))
> +		return;
> +
> +	/* Record the caller: the irq_work's stack says nothing about it. */
> +	kasan_record_aux_stack(head);
> +
> +	if (should_rcu_defer()) {
> +		call_rcu_defer(head, func);

[Severity: High]
Does this bypass the debug check for double call_rcu(), potentially corrupting
the callback list?

By moving debug_rcu_head_queue() from __call_rcu_common() into rcu_do_enqueue(),
deferred callbacks are added to the defer_head list before being validated.

If a double call_rcu() occurs, llist_add() in call_rcu_defer() will overwrite
head->next. If the callback was already in a normal cblist, its next pointer is
overwritten with defer_head->first, immediately splitting the cblist and
splicing it into the deferred list.

Could we check debug_rcu_head_queue() here before deferring, so that
CONFIG_DEBUG_OBJECTS_RCU_HEAD can safely catch the bug without list corruption?

> +		return;
> +	}
> +
> +	/*
> +	 * Only reachable from an NMI when deferral is off: before the scheduler
> +	 * is up, or with CONFIG_RCU_DEFER=n.  The enqueue can then race.
> +	 */
> +	WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && in_nmi());
> +
> +	rcu_do_enqueue(head, func, lazy_in);
> +}

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