Re: [PATCH v3 3/6] srcu: Make call_srcu() safe to call from any context

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] A TOCTOU race in `srcu_defer_flush()` locklessly checking `llist_empty()` can cause `srcu_barrier()` to return prematurely, leading to a Use-After-Free.
- [Medium] Expedited SRCU callbacks can be silently downgraded to normal callbacks due to a non-atomic update race on `sdp->defer_exp` between `__call_srcu()` and `__srcu_defer_drain()`.
--

--- Patch [3]: [PATCH v3 3/6] srcu: Make call_srcu() safe to call from any context ---
commit 137e7f3c08249a4bd260b0c0d6f275423685df28
Author: Puranjay Mohan <puranjay@xxxxxxxxxx>

srcu: Make call_srcu() safe to call from any context

call_srcu() has the same constraint as call_rcu(): its callback list and
locks are only touched with interrupts disabled. Defer as call_rcu()
does: stage the callback on the srcu_data's ->defer_cbs, chain that
srcu_data onto a per-CPU list, and raise a per-CPU irq_work that
re-issues it straight to the enqueue helper, never back through
__call_srcu().

> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 304112674e8a2..35fface51d50b 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
[ ... ]
> @@ -79,6 +80,45 @@ static void process_srcu(struct work_struct *work);
>  static void srcu_irq_work(struct irq_work *work);
>  static void srcu_delay_timer(struct timer_list *t);
>  
> +struct srcu_defer;
> +static void srcu_defer_drain(struct irq_work *iw);
> +static void __srcu_defer_drain(struct srcu_defer *sndp, bool guard);
> +
[ ... ]
> +/*
> + * Flush pending deferred callbacks so a following srcu_barrier() waits for them.
> + */
> +static void srcu_defer_flush(void)
> +{
> +	int cpu;
> +
> +	if (!IS_ENABLED(CONFIG_RCU_DEFER))
> +		return;
> +
> +	for_each_possible_cpu(cpu) {
> +		struct srcu_defer *sndp = &per_cpu(srcu_defer, cpu);
> +
> +		if (!llist_empty(&sndp->list))
> +			__srcu_defer_drain(sndp, false);

[Severity: Critical]
Can this lockless list check lead to a use-after-free during cleanup?

If an asynchronous irq_work is concurrently executing
__srcu_defer_drain() and has already detached the list:

CPU1 (irq_work)
__srcu_defer_drain()
    raw_spin_lock_irqsave(&sndp->lock, flags);
    llist_del_all(&sndp->list);  // list is now empty

CPU2 (cleanup_srcu_struct or srcu_barrier)
srcu_defer_flush()
    llist_empty(&sndp->list) // returns true, returns immediately

Then CPU2 could return prematurely from srcu_defer_flush(), and
cleanup_srcu_struct() might free the srcu_struct while CPU1 is still
iterating through the detached callbacks and accessing sdp->ssp.

> +	}
> +}
[ ... ]
> @@ -1423,6 +1473,111 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
[ ... ]
> +static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
> +			rcu_callback_t func, bool do_norm)
> +{
> +	if (should_rcu_defer()) {
> +		struct srcu_defer *sndp = this_cpu_ptr(&srcu_defer);
> +		struct srcu_data *sdp;
[ ... ]
> +		sdp = this_cpu_ptr(ssp->sda);
> +		rhp->func = func;
> +		if (!do_norm)
> +			WRITE_ONCE(sdp->defer_exp, true);
> +		if (llist_add((struct llist_node *)rhp, &sdp->defer_cbs)) {

[Severity: Medium]
Could this flag be overwritten by a concurrent __srcu_defer_drain()?

If __call_srcu() sets sdp->defer_exp to true just as another CPU is
draining the list (e.g., from srcu_defer_flush()), can the drainer's
delayed write to sdp->defer_exp clear the flag we just set here?

CPU1
__srcu_defer_drain()
    cnode = llist_del_all(&sdp->defer_cbs);

CPU2
__call_srcu()
    if (!do_norm)
        WRITE_ONCE(sdp->defer_exp, true);
    llist_add(rhp, &sdp->defer_cbs);

CPU1
__srcu_defer_drain()
    do_norm = !READ_ONCE(sdp->defer_exp);
    if (!do_norm)
        WRITE_ONCE(sdp->defer_exp, false); // Overwrites CPU2's write

This would cause CPU2's newly enqueued expedited callback to lose its
expedited status.

> +			/*
> +			 * Chain this srcu_data for the drain.  ->ssp must be
> +			 * published here: deferral skips check_init_srcu_struct(),
> +			 * so on a never-initialized static srcu_struct the
> +			 * statically zeroed ->sda still has a NULL ->ssp.
> +			 */
> +			sdp->ssp = ssp;
> +			if (llist_add(&sdp->defer_link, &sndp->list))
> +				irq_work_queue(&sndp->iw);
> +		}
> +		return;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805122346.269445-1-puranjay@xxxxxxxxxx?part=3