Re: [PATCH bpf] bpf: Fix UAF due to concurrent consumption of waiting_for_gp_ttrace

[Pu Lehui <pulehui@huawei.com>]

Thanks very much!

The review comments on bpf ci look excellent, and I will extract the 
valuable information from them.

On 2026/8/26 19:16, bot+bpf-ci@kernel.org wrote:
>>      alloc_bulk() runs in an IRQ context (serving as an implicit normal RCU
>>      read-side critical section), an RCU Tasks Trace GP cannot complete while
>>      alloc_bulk() is accessing the list.
> 
> Is this accurate on CONFIG_PREEMPT_RT? Looking at init_refill_work() in
> kernel/bpf/memalloc.c, it uses init_irq_work() without IRQ_WORK_INIT_HARD,
> so on RT the irq_work goes to lazy_list and is executed by the preemptible
> per-CPU 'irq_work/N' kthread, not in hardirq context. The allocator's own
> comment says "In RT irq_work runs in per-cpu kthread."
> 
> The conclusion still holds because run_irq_workd() wraps
> irq_work_run_list(&lazy_list) in guard(rcu)(), making it an explicit RCU
> read-side critical section on RT. So alloc_bulk() is protected by RCU, but
> the stated reason ('IRQ context') is only true on !RT.
> 
> Could the commit message clarify that the RCU-reader property comes from
> hardirq on !RT and explicit rcu_read_lock() in run_irq_workd() on RT?
> 
>>      Thus, the callback __free_rcu cannot run concurrently, ruling out
>>      missing RCU read-side locks as the cause.
> 
> Can __free_rcu really not run concurrently with alloc_bulk()? The race
> diagram two paragraphs later shows exactly that happening:
> 
> CPU0                                           CPU1
>                                                 __free_rcu (RCU Tasks Trace callback)
> alloc_bulk (irq context)
>    llist_del_first(&c->waiting_for_gp_ttrace)
>      entry = smp_load_acquire(&head->first);
>                                                 free_all(llist_del_all(&c->waiting_for_gp_ttrace))
>      next = READ_ONCE(entry->next); <-- trigger UAF
> 
> Looking at the guarantees from 57b23c0f612d, the RCU Tasks Trace GP
> contains at least one synchronize_rcu(), which waits for alloc_bulk()
> invocations that started before that internal synchronize_rcu(). But a
> fresh alloc_bulk() entered after the GP expired is not waited for, and
> __free_rcu can run while that fresh invocation is in flight.
> 
> So what's actually true is narrower: a ttrace GP cannot expire while an
> alloc_bulk() read section that began before the GP's internal
> synchronize_rcu() is still in flight. A fresh alloc_bulk() after the GP
> expired can race with __free_rcu().
> 
> Could the explanation be more precise about which alloc_bulk() invocations
> are protected and which can race?
> 
>> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
>> index e9662db7198f..fb1e733bfb82 100644
>> --- a/kernel/bpf/memalloc.c
>> +++ b/kernel/bpf/memalloc.c
>> @@ -110,7 +110,9 @@ struct bpf_mem_cache {
>>   	struct llist_node *free_by_rcu_tail;
>>   	struct llist_head waiting_for_gp;
>>   	struct llist_node *waiting_for_gp_tail;
>> +	struct llist_node *waiting_for_reclaim_gp;
>>   	struct rcu_head rcu;
>> +	struct rcu_head rcu_reclaim;
>>   	atomic_t call_rcu_in_progress;
>>   	struct llist_head free_llist_extra_rcu;
> 
> [ ... ]
> 
>> @@ -276,12 +278,28 @@ static int free_all(struct bpf_mem_cache *c, struct llist_node *llnode, bool per
>>   	return cnt;
>>   }
>>
>> +static void __free_final_rcu(struct rcu_head *head)
>> +{
>> +	struct bpf_mem_cache *c = container_of(head, struct bpf_mem_cache, rcu);
>> +	struct llist_node *llnode = c->waiting_for_reclaim_gp;
>> +
>> +	c->waiting_for_reclaim_gp = NULL;
>> +	free_all(c, llnode, !!c->percpu_size);
>> +	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
>> +}
>> +
>>   static void __free_rcu(struct rcu_head *head)
>>   {
>>   	struct bpf_mem_cache *c = container_of(head, struct bpf_mem_cache, rcu_ttrace);
>> +	struct llist_node *llnode = llist_del_all(&c->waiting_for_gp_ttrace);
>>
>> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
>> -	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
>> +	if (!llnode) {
>> +		atomic_set(&c->call_rcu_ttrace_in_progress, 0);
>> +		return;
>> +	}
>> +
>> +	c->waiting_for_reclaim_gp = llnode;
>> +	call_rcu(&c->rcu_reclaim, __free_final_rcu);
>                            ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Does free_mem_alloc() wait for this new callback? Looking at the teardown
> path:
> 
> kernel/bpf/memalloc.c:free_mem_alloc() {
>      /*
>       * Trace grace period implies RCU grace period, so all __free_rcu
>       * don't need extra call_rcu() (and thus extra rcu_barrier() here).
>       */
>      rcu_barrier();               /* wait for __free_by_rcu */
>      rcu_barrier_tasks_trace();   /* wait for __free_rcu */
>      free_mem_alloc_no_barrier(ma);
>          -> free_percpu(ma->cache) / free_percpu(ma->caches)
> }
> 
> The barriers run in that order: normal RCU first, then tasks-trace. If
> __free_rcu() executes during rcu_barrier_tasks_trace() and queues
> call_rcu(&c->rcu_reclaim, __free_final_rcu), there's no rcu_barrier()
> left to wait for it. So free_percpu() runs while __free_final_rcu() is
> still queued on the now-freed per-cpu memory.
> 
> Concrete sequence:
> 1. do_call_rcu_ttrace() queues call_rcu_tasks_trace(&c->rcu_ttrace,
>     __free_rcu)
> 2. Map destroyed, destroy path computes rcu_in_progress >= 1 from
>     atomic_read(&c->call_rcu_ttrace_in_progress), takes deferred path
> 3. free_mem_alloc(): rcu_barrier() at 716 completes, rcu_barrier_tasks_trace()
>     at 717 waits for __free_rcu() which runs and queues call_rcu(&c->rcu_reclaim,
>     __free_final_rcu), barrier returns with normal-RCU callback newly queued
> 4. free_mem_alloc_no_barrier() frees the per-cpu memory holding c
> 5. When normal grace period ends, __free_final_rcu() runs on freed memory
> 
> Also, drain_mem_cache() drains seven lists but not waiting_for_reclaim_gp,
> and check_mem_cache() WARN_ON_ONCE()s on those seven but never inspects
> waiting_for_reclaim_gp, so the leak is silent.
> 
> Does this need an rcu_barrier() after rcu_barrier_tasks_trace() (or the
> two swapped plus an added one)? And should the comment at lines 706-715
> be updated since it now says the opposite of what the code does?
> 
> Also note check_mem_cache() should probably check waiting_for_reclaim_gp.
> 
> The same llist_del_first-vs-llist_del_all hazard that this patch fixes
> for waiting_for_gp_ttrace also exists on free_by_rcu_ttrace. Looking at
> kernel/bpf/memalloc.c:
> 
> alloc_bulk() does:
>      obj = llist_del_first(&c->free_by_rcu_ttrace);
> 
> while do_call_rcu_ttrace() does:
>      llnode = llist_del_all(&c->free_by_rcu_ttrace);
>      ...
>      free_all(c, llnode, percpu);  /* draining path, immediate free */
> 
> and drain_mem_cache() does:
>      free_all(llist_del_all(&c->free_by_rcu_ttrace), ...);
> 
> include/linux/llist.h's concurrency table marks the del_first x del_all
> cell 'L' (lock is needed) and the kerneldoc on llist_del_first says "If
> multiple consumers are needed, please use llist_del_all or use lock
> between consumers."
> 
> The non-draining path in do_call_rcu_ttrace() is incidentally covered by
> the new extra GP because nodes end up in waiting_for_gp_ttrace, but the
> draining direct-free and drain_mem_cache()'s free_all() still free without
> any RCU deferral.
> 
> Does free_by_rcu_ttrace need the same treatment?
> 
> While the nodes sit in c->waiting_for_reclaim_gp they are invisible to
> alloc_bulk()'s reuse loop, so for one extra normal RCU GP the allocator
> must kmalloc() instead of reusing. This partially undoes the optimization
> being fixed.
> 
> Is this expected? Should the commit message mention it?
> 
> The commit message says "Syzkaller repeatedly triggered" but carries no
> Reported-by:, Closes: or Link: tag, and the splat is from a private 6.6.0+
> tree rather than upstream syzbot.
> 
> Also, Hou Tao reported this exact hazard during review of 04fabf00b4d3 in
> July 2023 (lore.kernel.org/rcu/957dd5cd-0855-1197-7045-4cb1590bd753@huaweicloud.com).
> Alexei's reply was that the probability was "lower than a bit flip due to
> cosmic rays."
> 
> Could citing that thread make the analysis more credible and give proper
> credit to Hou Tao?
> 
> 
> ---
> 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/32959494022