Re: [PATCH bpf v3] bpf: Fix UAF due to concurrent consumption of ttrace lists in alloc_bulk

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4340 bytes --]

> This is expected because, as noted in commit 57b23c0f612d ("bpf: Retire
> rcu_trace_implies_rcu_gp()"), an RCU Tasks Trace GP currently implies
> (and will continue to imply in the future) a normal RCU GP. Since
> alloc_bulk() runs in an RCU read-side CS (!PREEMPT_RT runs in IRQ
> context, PREEMPT_RT runs with guard(rcu)), an RCU Tasks Trace GP cannot
> complete while alloc_bulk() is accessing the list.

Is this claim about the RCU Tasks Trace GP accurate? Looking at the SRCU
implementation, synchronize_rcu() sits in the middle of the SRCU scan
(kernel/rcu/srcutree.c srcu_readers_active_idx_check()), not at the end.

An RCU GP is only required to wait for read-side critical sections that
were already in progress when the GP started. Once a CPU reports its
quiescent state, a new hardirq handler on that CPU does not delay the GP.
So an alloc_bulk() that starts after the synchronize_rcu() call cannot
hold off the RCU Tasks Trace GP at all.

Even if the GP could not complete during alloc_bulk(), that would not
prevent the callback from running during alloc_bulk(). Callbacks are
invoked asynchronously after the GP ends (srcu_invoke_callbacks workqueue
in current kernels, or rcu_tasks_invoke_cbs in the 6.6 kernel that
produced the splat).

The race diagram three paragraphs later shows __free_rcu executing
concurrently with alloc_bulk's llist_del_first, which would not be
possible if the statement above were true.

Would it be more accurate to say that RCU read-side protection is not the
missing ingredient here, but rather mutual exclusion between the two
llist consumers?

> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
> index e9662db7198f..5927943c765a 100644
> --- a/kernel/bpf/memalloc.c
> +++ b/kernel/bpf/memalloc.c

[ ... ]

> @@ -214,25 +215,25 @@ static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
>  	gfp = __GFP_NOWARN | __GFP_ACCOUNT;
>  	gfp |= atomic ? GFP_NOWAIT : GFP_KERNEL;
>
> -	for (i = 0; i < cnt; i++) {
> -		/*
> -		 * For every 'c' llist_del_first(&c->free_by_rcu_ttrace); is
> -		 * done only by one CPU == current CPU. Other CPUs might
> -		 * llist_add() and llist_del_all() in parallel.
> -		 */
> -		obj = llist_del_first(&c->free_by_rcu_ttrace);
> -		if (!obj)
> -			break;
> -		add_obj_to_free_list(c, obj);
> -	}
> -	if (i >= cnt)
> -		return;
> +	scoped_guard(raw_spinlock_irqsave, &c->lock) {
> +		for (i = 0; i < cnt; i++) {
> +			/*
> +			 * For every 'c' llist_del_first(&c->free_by_rcu_ttrace); is
> +			 * done only by one CPU == current CPU. Other CPUs might
> +			 * llist_add() and llist_del_all() in parallel.
> +			 */
> +			obj = llist_del_first(&c->free_by_rcu_ttrace);
> +			if (!obj)
> +				break;
> +			add_obj_to_free_list(c, obj);
> +		}
>
> -	for (; i < cnt; i++) {
> -		obj = llist_del_first(&c->waiting_for_gp_ttrace);
> -		if (!obj)
> -			break;
> -		add_obj_to_free_list(c, obj);
> +		for (; i < cnt; i++) {
> +			obj = llist_del_first(&c->waiting_for_gp_ttrace);
> +			if (!obj)
> +				break;
> +			add_obj_to_free_list(c, obj);
> +		}
>  	}
>  	if (i >= cnt)
>  		return;

The comment moved inside the new critical section still states the
lockless rationale:

    /*
     * For every 'c' llist_del_first(&c->free_by_rcu_ttrace); is
     * done only by one CPU == current CPU. Other CPUs might
     * llist_add() and llist_del_all() in parallel.
     */

The last sentence asserts that concurrent llist_del_all() is fine.
According to include/linux/llist.h, the del_first x del_all combination
requires a lock, and the commit message's first race diagram shows exactly
this combination producing the KASAN splat.

Should this comment be updated to reflect that c->lock now serializes
against __free_rcu() and the draining branch of do_call_rcu_ttrace(),
rather than asserting that lockless operation is safe?

The related comments in enque_to_free() ("Nothing races to add to
free_by_rcu_ttrace list.") and drain_mem_cache() ("Except for
waiting_for_gp_ttrace list, there are no concurrent operations on these
lists") may also need updates, though they are outside this diff.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33525009657