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

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

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

Syzkaller repeatedly triggered UAF splats related to nodes in
waiting_for_gp_ttrace within the bpf memalloc:

BUG: KASAN: slab-use-after-free in llist_del_first+0x85/0x110 lib/llist.c:61
Read of size 8 at addr ffff8881572cd080 by task syz.4.470/5112
 ...
 llist_del_first+0x85/0x110 lib/llist.c:61
 alloc_bulk+0x193/0x460 kernel/bpf/memalloc.c:229
 bpf_mem_refill+0x386/0x560 kernel/bpf/memalloc.c:436

Freed by task 14:
 ...
 __free_rcu kernel/bpf/memalloc.c:281 [inline]
 __free_rcu_tasks_trace+0x48/0xd0 kernel/bpf/memalloc.c:291
 rcu_tasks_invoke_cbs+0x1ec/0x3e0 kernel/rcu/tasks.h:571
 rcu_tasks_one_gp+0x13d/0x220 kernel/rcu/tasks.h:621
 rcu_tasks_kthread+0xf3/0x120 kernel/rcu/tasks.h:651

Initially, we suspected that alloc_bulk() lacked RCU Tasks Trace
protection when accessing waiting_for_gp_ttrace. However, explicitly
adding rcu_read_lock_trace() did not help.

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. Thus, the callback
__free_rcu cannot run concurrently, ruling out missing RCU read-side
locks as the cause. And same for free_by_rcu_ttrace list.

Further investigation revealed that the UAF does not occur before the
RCU Tasks Trace grace period expires, but rather during the execution of
its callback. When the callback invokes llist_del_all to reclaim
waiting_for_gp_ttrace nodes, there is no synchronization protecting
against concurrent alloc_bulk() calls. If alloc_bulk() operates on
waiting_for_gp_ttrace simultaneously, a race condition ensues, as
illustrated below:

CPU0                                           CPU1
                                               __free_rcu (RCU Tasks Trace callback)
alloc_bulk (RCU read-side CS)
  llist_del_first(&c->waiting_for_gp_ttrace)
    entry = smp_load_acquire(&head->first);
    do {
      if (entry == NULL)
        return NULL;
                                               free_all(llist_del_all(&c->waiting_for_gp_ttrace))
                                                 llist_for_each_safe(pos, t, llnode)
                                                   free_one(pos);
      next = READ_ONCE(entry->next); <-- trigger UAF
    } while (!try_cmpxchg(&head->first, &entry, next));

Since alloc_bulk() operates on waiting_for_gp_ttrace under RCU read-side
CS, Fix this by introducing a raw spinlock to synchronize the concurrent
consumption (llist_del_first vs llist_del_all) on waiting_for_gp_ttrace.

Note that free_by_rcu_ttrace does not suffer from this issue as it only
has a single active consumer during normal operation.

Fixes: 04fabf00b4d3 ("bpf: Allow reuse from waiting_for_gp_ttrace list.")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Suggested-by: Hou Tao <houtao1@huawei.com>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/bpf/memalloc.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
index e9662db7198f..58296e92a4fe 100644
--- a/kernel/bpf/memalloc.c
+++ b/kernel/bpf/memalloc.c
@@ -119,6 +119,7 @@ struct bpf_mem_cache {
 	struct llist_head waiting_for_gp_ttrace;
 	struct rcu_head rcu_ttrace;
 	atomic_t call_rcu_ttrace_in_progress;
+	raw_spinlock_t lock;
 };
 
 struct bpf_mem_caches {
@@ -207,6 +208,7 @@ static void add_obj_to_free_list(struct bpf_mem_cache *c, void *obj)
 static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
 {
 	struct mem_cgroup *memcg = NULL, *old_memcg;
+	unsigned long flags;
 	gfp_t gfp;
 	void *obj;
 	int i;
@@ -228,12 +230,14 @@ static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
 	if (i >= cnt)
 		return;
 
+	raw_spin_lock_irqsave(&c->lock, flags);
 	for (; i < cnt; i++) {
-		obj = llist_del_first(&c->waiting_for_gp_ttrace);
+		obj = __llist_del_first(&c->waiting_for_gp_ttrace);
 		if (!obj)
 			break;
 		add_obj_to_free_list(c, obj);
 	}
+	raw_spin_unlock_irqrestore(&c->lock, flags);
 	if (i >= cnt)
 		return;
 
@@ -279,8 +283,14 @@ static int free_all(struct bpf_mem_cache *c, struct llist_node *llnode, bool per
 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;
+	unsigned long flags;
 
-	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
+	raw_spin_lock_irqsave(&c->lock, flags);
+	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
+	raw_spin_unlock_irqrestore(&c->lock, flags);
+
+	free_all(c, llnode, !!c->percpu_size);
 	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
 }
 
@@ -297,6 +307,7 @@ static void enque_to_free(struct bpf_mem_cache *c, void *obj)
 static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
 {
 	struct llist_node *llnode, *t;
+	unsigned long flags;
 
 	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
 		if (unlikely(READ_ONCE(c->draining))) {
@@ -307,8 +318,10 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
 	}
 
 	WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
+	raw_spin_lock_irqsave(&c->lock, flags);
 	llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
-		llist_add(llnode, &c->waiting_for_gp_ttrace);
+		__llist_add(llnode, &c->waiting_for_gp_ttrace);
+	raw_spin_unlock_irqrestore(&c->lock, flags);
 
 	if (unlikely(READ_ONCE(c->draining))) {
 		__free_rcu(&c->rcu_ttrace);
@@ -535,6 +548,7 @@ int bpf_mem_alloc_init(struct bpf_mem_alloc *ma, int size, bool percpu)
 			c->objcg = objcg;
 			c->percpu_size = percpu_size;
 			c->tgt = c;
+			raw_spin_lock_init(&c->lock);
 			init_refill_work(c);
 			prefill_mem_cache(c, cpu);
 		}
@@ -557,7 +571,7 @@ int bpf_mem_alloc_init(struct bpf_mem_alloc *ma, int size, bool percpu)
 			c->objcg = objcg;
 			c->percpu_size = percpu_size;
 			c->tgt = c;
-
+			raw_spin_lock_init(&c->lock);
 			init_refill_work(c);
 			prefill_mem_cache(c, cpu);
 		}
@@ -609,7 +623,7 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
 		c->objcg = objcg;
 		c->percpu_size = percpu_size;
 		c->tgt = c;
-
+		raw_spin_lock_init(&c->lock);
 		init_refill_work(c);
 		prefill_mem_cache(c, cpu);
 	}
@@ -620,6 +634,8 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
 static void drain_mem_cache(struct bpf_mem_cache *c)
 {
 	bool percpu = !!c->percpu_size;
+	struct llist_node *llnode;
+	unsigned long flags;
 
 	/* No progs are using this bpf_mem_cache, but htab_map_free() called
 	 * bpf_mem_cache_free() for all remaining elements and they can be in
@@ -629,7 +645,10 @@ static void drain_mem_cache(struct bpf_mem_cache *c)
 	 * on these lists, so it is safe to use __llist_del_all().
 	 */
 	free_all(c, llist_del_all(&c->free_by_rcu_ttrace), percpu);
-	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), percpu);
+	raw_spin_lock_irqsave(&c->lock, flags);
+	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
+	raw_spin_unlock_irqrestore(&c->lock, flags);
+	free_all(c, llnode, percpu);
 	free_all(c, __llist_del_all(&c->free_llist), percpu);
 	free_all(c, __llist_del_all(&c->free_llist_extra), percpu);
 	free_all(c, __llist_del_all(&c->free_by_rcu), percpu);
-- 
2.34.1

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

[Leon Hwang]

On 27/8/26 16:40, Pu Lehui wrote:
> From: Pu Lehui <pulehui@huawei.com>
[...]
>  kernel/bpf/memalloc.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
> index e9662db7198f..58296e92a4fe 100644
> --- a/kernel/bpf/memalloc.c
> +++ b/kernel/bpf/memalloc.c
> @@ -119,6 +119,7 @@ struct bpf_mem_cache {
>  	struct llist_head waiting_for_gp_ttrace;
>  	struct rcu_head rcu_ttrace;
>  	atomic_t call_rcu_ttrace_in_progress;
> +	raw_spinlock_t lock;
>  };
>  
>  struct bpf_mem_caches {
> @@ -207,6 +208,7 @@ static void add_obj_to_free_list(struct bpf_mem_cache *c, void *obj)
>  static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
>  {
>  	struct mem_cgroup *memcg = NULL, *old_memcg;
> +	unsigned long flags;
>  	gfp_t gfp;
>  	void *obj;
>  	int i;
> @@ -228,12 +230,14 @@ static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
>  	if (i >= cnt)
>  		return;
>  
> +	raw_spin_lock_irqsave(&c->lock, flags);
>  	for (; i < cnt; i++) {
> -		obj = llist_del_first(&c->waiting_for_gp_ttrace);
> +		obj = __llist_del_first(&c->waiting_for_gp_ttrace);
>  		if (!obj)
>  			break;
>  		add_obj_to_free_list(c, obj);
>  	}
> +	raw_spin_unlock_irqrestore(&c->lock, flags);

Looks like scoped_guard() could get rid of the 'flags':

scoped_guard(raw_spinlock_irqsave, &c->lock) {
	...
}

>  	if (i >= cnt)
>  		return;
>  
> @@ -279,8 +283,14 @@ static int free_all(struct bpf_mem_cache *c, struct llist_node *llnode, bool per
>  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;
> +	unsigned long flags;
>  
> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
> +	raw_spin_lock_irqsave(&c->lock, flags);
> +	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
> +	raw_spin_unlock_irqrestore(&c->lock, flags);

Ditto
> +
> +	free_all(c, llnode, !!c->percpu_size);
>  	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
>  }
>  
> @@ -297,6 +307,7 @@ static void enque_to_free(struct bpf_mem_cache *c, void *obj)
>  static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>  {
>  	struct llist_node *llnode, *t;
> +	unsigned long flags;
>  
>  	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
>  		if (unlikely(READ_ONCE(c->draining))) {
> @@ -307,8 +318,10 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>  	}
>  
>  	WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
> +	raw_spin_lock_irqsave(&c->lock, flags);
>  	llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
> -		llist_add(llnode, &c->waiting_for_gp_ttrace);
> +		__llist_add(llnode, &c->waiting_for_gp_ttrace);
> +	raw_spin_unlock_irqrestore(&c->lock, flags);

Ditto

>  
>  	if (unlikely(READ_ONCE(c->draining))) {
>  		__free_rcu(&c->rcu_ttrace);
> @@ -535,6 +548,7 @@ int bpf_mem_alloc_init(struct bpf_mem_alloc *ma, int size, bool percpu)
>  			c->objcg = objcg;
>  			c->percpu_size = percpu_size;
>  			c->tgt = c;
> +			raw_spin_lock_init(&c->lock);
>  			init_refill_work(c);
>  			prefill_mem_cache(c, cpu);
>  		}
> @@ -557,7 +571,7 @@ int bpf_mem_alloc_init(struct bpf_mem_alloc *ma, int size, bool percpu)
>  			c->objcg = objcg;
>  			c->percpu_size = percpu_size;
>  			c->tgt = c;
> -
> +			raw_spin_lock_init(&c->lock);
>  			init_refill_work(c);
>  			prefill_mem_cache(c, cpu);
>  		}
> @@ -609,7 +623,7 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
>  		c->objcg = objcg;
>  		c->percpu_size = percpu_size;
>  		c->tgt = c;
> -
> +		raw_spin_lock_init(&c->lock);
>  		init_refill_work(c);
>  		prefill_mem_cache(c, cpu);
>  	}
> @@ -620,6 +634,8 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
>  static void drain_mem_cache(struct bpf_mem_cache *c)
>  {
>  	bool percpu = !!c->percpu_size;
> +	struct llist_node *llnode;
> +	unsigned long flags;
>  
>  	/* No progs are using this bpf_mem_cache, but htab_map_free() called
>  	 * bpf_mem_cache_free() for all remaining elements and they can be in
> @@ -629,7 +645,10 @@ static void drain_mem_cache(struct bpf_mem_cache *c)
>  	 * on these lists, so it is safe to use __llist_del_all().
>  	 */
>  	free_all(c, llist_del_all(&c->free_by_rcu_ttrace), percpu);
> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), percpu);
> +	raw_spin_lock_irqsave(&c->lock, flags);
> +	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
> +	raw_spin_unlock_irqrestore(&c->lock, flags);

Ditto

Thanks,
Leon

> +	free_all(c, llnode, percpu);
>  	free_all(c, __llist_del_all(&c->free_llist), percpu);
>  	free_all(c, __llist_del_all(&c->free_llist_extra), percpu);
>  	free_all(c, __llist_del_all(&c->free_by_rcu), percpu);

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

[bot+bpf-ci]

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

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

[ ... ]

> @@ -297,6 +307,7 @@ static void enque_to_free(struct bpf_mem_cache *c, void *obj)
>  static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>  {
>  	struct llist_node *llnode, *t;
> +	unsigned long flags;
>
>  	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
>  		if (unlikely(READ_ONCE(c->draining))) {
> @@ -307,8 +318,10 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>  	}
>
>  	WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
> +	raw_spin_lock_irqsave(&c->lock, flags);
>  	llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
> -		llist_add(llnode, &c->waiting_for_gp_ttrace);
> +		__llist_add(llnode, &c->waiting_for_gp_ttrace);
> +	raw_spin_unlock_irqrestore(&c->lock, flags);

Can this loop iteration count be bounded? The critical section covers
the whole free_by_rcu_ttrace -> waiting_for_gp_ttrace transfer, so
c->lock is held with IRQs disabled for a number of iterations that has
no upper bound.

free_by_rcu_ttrace is filled from enque_to_free() by free_bulk() and,
more significantly, by the batch append in __free_by_rcu():

kernel/bpf/memalloc.c:__free_by_rcu() {
    llist_add_batch(llnode, c->waiting_for_gp_tail,
                    &tgt->free_by_rcu_ttrace);
}

That batch is the whole c->waiting_for_gp chain, which builds from
every bpf_mem_free_rcu()/unit_free_rcu() that happened while
call_rcu_in_progress was set, proportional to the object-free rate
without any watermark. While call_rcu_ttrace_in_progress is set,
successive __free_by_rcu() invocations keep appending without anything
draining it, so batches accumulate across rounds.

Compare the other new critical sections in the patch, which are all
O(1): __llist_del_all in __free_rcu and drain_mem_cache, or bounded by
c->batch in alloc_bulk (roughly 48 iterations max). Only this one is
unbounded, and raw_spinlock_t + irqsave means it is non-preemptible
even on PREEMPT_RT, where do_call_rcu_ttrace() is reached from the
rcuc/rcuop kthread via __free_by_rcu().

The lock is only needed for the destination list. llist_del_all() on
free_by_rcu_ttrace is an xchg and is already safe against concurrent
llist_add() producers without a lock. Could the chain be re-linked (or
its tail found) outside the lock and spliced into waiting_for_gp_ttrace
with a single __llist_add_batch() under the lock, keeping the IRQ-off
window O(1)?

[ ... ]

> @@ -620,6 +634,8 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
>  static void drain_mem_cache(struct bpf_mem_cache *c)
>  {
>  	bool percpu = !!c->percpu_size;
> +	struct llist_node *llnode;
> +	unsigned long flags;
>
>  	/* No progs are using this bpf_mem_cache, but htab_map_free() called
>  	 * bpf_mem_cache_free() for all remaining elements and they can be in
> @@ -629,7 +645,10 @@ static void drain_mem_cache(struct bpf_mem_cache *c)
>  	 * on these lists, so it is safe to use __llist_del_all().
>  	 */
>  	free_all(c, llist_del_all(&c->free_by_rcu_ttrace), percpu);
> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), percpu);
> +	raw_spin_lock_irqsave(&c->lock, flags);
> +	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
> +	raw_spin_unlock_irqrestore(&c->lock, flags);
> +	free_all(c, llnode, percpu);

Does the comment above need updating? It says "Except for
waiting_for_gp_ttrace list, there are no concurrent operations on these
lists, so it is safe to use __llist_del_all()."

Before the patch the comment matched the code: waiting_for_gp_ttrace
was the one list drained with the atomic llist_del_all(), and the
comment named it as the exception to "safe to use __llist_del_all()".

After the patch waiting_for_gp_ttrace is the list that does use
__llist_del_all() (now serialised by c->lock), while free_by_rcu_ttrace
and waiting_for_gp still use the atomic llist_del_all(). The sentence
now points at the opposite list from the code, and no longer explains
why the new lock is needed here.

A reader trying to work out which lists still need atomic accessors
after this change gets the wrong answer from the comment. Should the
comment say that concurrent consumption of waiting_for_gp_ttrace is now
excluded by c->lock, so plain __llist_del_all() is safe, while
free_by_rcu_ttrace / waiting_for_gp still need the atomic variants?

[ ... ]

commit 03e0a12c3be3d11fe9a3b87582070b2158a7bb74
Author: Pu Lehui <pulehui@huawei.com>

>     bpf: Fix UAF due to concurrent consumption of waiting_for_gp_ttrace
>
>     Syzkaller repeatedly triggered UAF splats related to nodes in
>     waiting_for_gp_ttrace within the bpf memalloc:

[ ... ]

>     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. Thus, the callback
>     __free_rcu cannot run concurrently, ruling out missing RCU read-side
>     locks as the cause. And same for free_by_rcu_ttrace list.

Is the statement "Thus, the callback __free_rcu cannot run concurrently"
accurate as written?

Looking at the race diagram three paragraphs later, it shows exactly
that scenario: __free_rcu on CPU1 executing in the middle of
alloc_bulk's llist_del_first on CPU0. What the RCU argument actually
establishes is narrower: the GP cannot complete during the particular
read-side CS it is waiting on, so RCU read-side protection is not what
is missing.

Nothing prevents a new alloc_bulk() from starting after the GP has
completed and running concurrently with the callback. __free_rcu runs
from the rcu_tasks_trace kthread (process context, no irqs disabled, any
CPU) while alloc_bulk runs from irq_work on the cache's own CPU; on
PREEMPT_RT alloc_bulk is even preemptible, so same-CPU interleaving is
possible too.

A reviewer reading this sentence literally would conclude the race in
the diagram is impossible. Could the paragraph say "RCU read-side
protection is not what is missing; what is missing is mutual exclusion
between the two llist consumers"?

>     Note that free_by_rcu_ttrace does not suffer from this issue as it only
>     has a single active consumer during normal operation.

Is this premise correct? free_by_rcu_ttrace has two distinct consumers,
and they are not confined to one CPU:

  - alloc_bulk() memalloc.c:225 (still outside c->lock):

    obj = llist_del_first(&c->free_by_rcu_ttrace);

  - do_call_rcu_ttrace() memalloc.c:322, and drain_mem_cache()
    memalloc.c:647:

    llist_del_all(&c->free_by_rcu_ttrace)

do_call_rcu_ttrace() is called as do_call_rcu_ttrace(tgt) with
tgt = c->tgt from free_bulk() (memalloc.c:341, 364) and __free_by_rcu()
(memalloc.c:370, 383). c->tgt is set to the cache that allocated the
object, potentially another CPU's cache:

kernel/bpf/memalloc.c:unit_free() {
    c->tgt = *(struct bpf_mem_cache **)llnode;
}

So llist_del_all(&tgt->free_by_rcu_ttrace) on CPU1 can run concurrently
with llist_del_first(&tgt->free_by_rcu_ttrace) in alloc_bulk(tgt) on
tgt's own CPU. The file's own comment at memalloc.c:220-224 says as
much: "Other CPUs might llist_add() and llist_del_all() in parallel."

What actually keeps that pair harmless in the common case is the RCU
deferral the changelog describes earlier, not single-consumer-ness: the
nodes taken by llist_del_all() are moved to waiting_for_gp_ttrace and
only freed by __free_rcu() after an RCU tasks trace GP, which cannot
elapse while alloc_bulk() is inside its RCU read-side section. That
argument does not cover the two draining paths, which free immediately
while still only being ordered against alloc_bulk() by
WRITE_ONCE(c->draining, true) / irq_work_sync().

Since the changelog is what a reviewer will use to decide the fix is
complete, should the sentence be corrected to state the real reason (RCU
deferral) or should the residual draining-path window be acknowledged?

Leon Hwang suggested in the v2 review that scoped_guard() could be used
instead of explicit raw_spin_lock_irqsave/raw_spin_unlock_irqrestore to
simplify the code and avoid needing the flags variable. This is a code
style improvement that would make the locking more idiomatic. The
suggestion applies to multiple locations: alloc_bulk(), __free_rcu(),
do_call_rcu_ttrace(), and drain_mem_cache(). Is this something worth
addressing before merge?


---
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/33055527430

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

[Pu Lehui]

On 2026/8/27 16:51, Leon Hwang wrote:
> On 27/8/26 16:40, Pu Lehui wrote:
>> From: Pu Lehui <pulehui@huawei.com>
> [...]
>>   kernel/bpf/memalloc.c | 31 +++++++++++++++++++++++++------
>>   1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
>> index e9662db7198f..58296e92a4fe 100644
>> --- a/kernel/bpf/memalloc.c
>> +++ b/kernel/bpf/memalloc.c
>> @@ -119,6 +119,7 @@ struct bpf_mem_cache {
>>   	struct llist_head waiting_for_gp_ttrace;
>>   	struct rcu_head rcu_ttrace;
>>   	atomic_t call_rcu_ttrace_in_progress;
>> +	raw_spinlock_t lock;
>>   };
>>   
>>   struct bpf_mem_caches {
>> @@ -207,6 +208,7 @@ static void add_obj_to_free_list(struct bpf_mem_cache *c, void *obj)
>>   static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
>>   {
>>   	struct mem_cgroup *memcg = NULL, *old_memcg;
>> +	unsigned long flags;
>>   	gfp_t gfp;
>>   	void *obj;
>>   	int i;
>> @@ -228,12 +230,14 @@ static void alloc_bulk(struct bpf_mem_cache *c, int cnt, int node, bool atomic)
>>   	if (i >= cnt)
>>   		return;
>>   
>> +	raw_spin_lock_irqsave(&c->lock, flags);
>>   	for (; i < cnt; i++) {
>> -		obj = llist_del_first(&c->waiting_for_gp_ttrace);
>> +		obj = __llist_del_first(&c->waiting_for_gp_ttrace);
>>   		if (!obj)
>>   			break;
>>   		add_obj_to_free_list(c, obj);
>>   	}
>> +	raw_spin_unlock_irqrestore(&c->lock, flags);
> 
> Looks like scoped_guard() could get rid of the 'flags':
> 
> scoped_guard(raw_spinlock_irqsave, &c->lock) {
> 	...
> }
> 
>>   	if (i >= cnt)
>>   		return;
>>   
>> @@ -279,8 +283,14 @@ static int free_all(struct bpf_mem_cache *c, struct llist_node *llnode, bool per
>>   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;
>> +	unsigned long flags;
>>   
>> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
>> +	raw_spin_lock_irqsave(&c->lock, flags);
>> +	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
>> +	raw_spin_unlock_irqrestore(&c->lock, flags);
> 
> Ditto
>> +
>> +	free_all(c, llnode, !!c->percpu_size);
>>   	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
>>   }
>>   
>> @@ -297,6 +307,7 @@ static void enque_to_free(struct bpf_mem_cache *c, void *obj)
>>   static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>>   {
>>   	struct llist_node *llnode, *t;
>> +	unsigned long flags;
>>   
>>   	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
>>   		if (unlikely(READ_ONCE(c->draining))) {
>> @@ -307,8 +318,10 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>>   	}
>>   
>>   	WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
>> +	raw_spin_lock_irqsave(&c->lock, flags);
>>   	llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
>> -		llist_add(llnode, &c->waiting_for_gp_ttrace);
>> +		__llist_add(llnode, &c->waiting_for_gp_ttrace);
>> +	raw_spin_unlock_irqrestore(&c->lock, flags);
> 
> Ditto
> 
>>   
>>   	if (unlikely(READ_ONCE(c->draining))) {
>>   		__free_rcu(&c->rcu_ttrace);
>> @@ -535,6 +548,7 @@ int bpf_mem_alloc_init(struct bpf_mem_alloc *ma, int size, bool percpu)
>>   			c->objcg = objcg;
>>   			c->percpu_size = percpu_size;
>>   			c->tgt = c;
>> +			raw_spin_lock_init(&c->lock);
>>   			init_refill_work(c);
>>   			prefill_mem_cache(c, cpu);
>>   		}
>> @@ -557,7 +571,7 @@ int bpf_mem_alloc_init(struct bpf_mem_alloc *ma, int size, bool percpu)
>>   			c->objcg = objcg;
>>   			c->percpu_size = percpu_size;
>>   			c->tgt = c;
>> -
>> +			raw_spin_lock_init(&c->lock);
>>   			init_refill_work(c);
>>   			prefill_mem_cache(c, cpu);
>>   		}
>> @@ -609,7 +623,7 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
>>   		c->objcg = objcg;
>>   		c->percpu_size = percpu_size;
>>   		c->tgt = c;
>> -
>> +		raw_spin_lock_init(&c->lock);
>>   		init_refill_work(c);
>>   		prefill_mem_cache(c, cpu);
>>   	}
>> @@ -620,6 +634,8 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
>>   static void drain_mem_cache(struct bpf_mem_cache *c)
>>   {
>>   	bool percpu = !!c->percpu_size;
>> +	struct llist_node *llnode;
>> +	unsigned long flags;
>>   
>>   	/* No progs are using this bpf_mem_cache, but htab_map_free() called
>>   	 * bpf_mem_cache_free() for all remaining elements and they can be in
>> @@ -629,7 +645,10 @@ static void drain_mem_cache(struct bpf_mem_cache *c)
>>   	 * on these lists, so it is safe to use __llist_del_all().
>>   	 */
>>   	free_all(c, llist_del_all(&c->free_by_rcu_ttrace), percpu);
>> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), percpu);
>> +	raw_spin_lock_irqsave(&c->lock, flags);
>> +	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
>> +	raw_spin_unlock_irqrestore(&c->lock, flags);
> 
> Ditto
> 
> Thanks,
> Leon
> 
>> +	free_all(c, llnode, percpu);
>>   	free_all(c, __llist_del_all(&c->free_llist), percpu);
>>   	free_all(c, __llist_del_all(&c->free_llist_extra), percpu);
>>   	free_all(c, __llist_del_all(&c->free_by_rcu), percpu);

ok, will do

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

[Hou Tao]

On 8/27/2026 5:24 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
>> index e9662db7198f..58296e92a4fe 100644
>> --- a/kernel/bpf/memalloc.c
>> +++ b/kernel/bpf/memalloc.c
> [ ... ]
>
>> @@ -297,6 +307,7 @@ static void enque_to_free(struct bpf_mem_cache *c, void *obj)
>>  static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>>  {
>>  	struct llist_node *llnode, *t;
>> +	unsigned long flags;
>>
>>  	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
>>  		if (unlikely(READ_ONCE(c->draining))) {
>> @@ -307,8 +318,10 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
>>  	}
>>
>>  	WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
>> +	raw_spin_lock_irqsave(&c->lock, flags);
>>  	llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
>> -		llist_add(llnode, &c->waiting_for_gp_ttrace);
>> +		__llist_add(llnode, &c->waiting_for_gp_ttrace);
>> +	raw_spin_unlock_irqrestore(&c->lock, flags);
> Can this loop iteration count be bounded? The critical section covers
> the whole free_by_rcu_ttrace -> waiting_for_gp_ttrace transfer, so
> c->lock is held with IRQs disabled for a number of iterations that has
> no upper bound.
>
> free_by_rcu_ttrace is filled from enque_to_free() by free_bulk() and,
> more significantly, by the batch append in __free_by_rcu():
>
> kernel/bpf/memalloc.c:__free_by_rcu() {
>     llist_add_batch(llnode, c->waiting_for_gp_tail,
>                     &tgt->free_by_rcu_ttrace);
> }
>
> That batch is the whole c->waiting_for_gp chain, which builds from
> every bpf_mem_free_rcu()/unit_free_rcu() that happened while
> call_rcu_in_progress was set, proportional to the object-free rate
> without any watermark. While call_rcu_ttrace_in_progress is set,
> successive __free_by_rcu() invocations keep appending without anything
> draining it, so batches accumulate across rounds.
>
> Compare the other new critical sections in the patch, which are all
> O(1): __llist_del_all in __free_rcu and drain_mem_cache, or bounded by
> c->batch in alloc_bulk (roughly 48 iterations max). Only this one is
> unbounded, and raw_spinlock_t + irqsave means it is non-preemptible
> even on PREEMPT_RT, where do_call_rcu_ttrace() is reached from the
> rcuc/rcuop kthread via __free_by_rcu().
>
> The lock is only needed for the destination list. llist_del_all() on
> free_by_rcu_ttrace is an xchg and is already safe against concurrent
> llist_add() producers without a lock. Could the chain be re-linked (or
> its tail found) outside the lock and spliced into waiting_for_gp_ttrace
> with a single __llist_add_batch() under the lock, keeping the IRQ-off
> window O(1)?

I think it is a good suggestion to reduce the lock scope of the
irq-disabled raw-spinlock.
>
> [ ... ]
>
>> @@ -620,6 +634,8 @@ int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size)
>>  static void drain_mem_cache(struct bpf_mem_cache *c)
>>  {
>>  	bool percpu = !!c->percpu_size;
>> +	struct llist_node *llnode;
>> +	unsigned long flags;
>>
>>  	/* No progs are using this bpf_mem_cache, but htab_map_free() called
>>  	 * bpf_mem_cache_free() for all remaining elements and they can be in
>> @@ -629,7 +645,10 @@ static void drain_mem_cache(struct bpf_mem_cache *c)
>>  	 * on these lists, so it is safe to use __llist_del_all().
>>  	 */
>>  	free_all(c, llist_del_all(&c->free_by_rcu_ttrace), percpu);
>> -	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), percpu);
>> +	raw_spin_lock_irqsave(&c->lock, flags);
>> +	llnode = __llist_del_all(&c->waiting_for_gp_ttrace);
>> +	raw_spin_unlock_irqrestore(&c->lock, flags);
>> +	free_all(c, llnode, percpu);
> Does the comment above need updating? It says "Except for
> waiting_for_gp_ttrace list, there are no concurrent operations on these
> lists, so it is safe to use __llist_del_all()."
>
> Before the patch the comment matched the code: waiting_for_gp_ttrace
> was the one list drained with the atomic llist_del_all(), and the
> comment named it as the exception to "safe to use __llist_del_all()".
>
> After the patch waiting_for_gp_ttrace is the list that does use
> __llist_del_all() (now serialised by c->lock), while free_by_rcu_ttrace
> and waiting_for_gp still use the atomic llist_del_all(). The sentence
> now points at the opposite list from the code, and no longer explains
> why the new lock is needed here.
>
> A reader trying to work out which lists still need atomic accessors
> after this change gets the wrong answer from the comment. Should the
> comment say that concurrent consumption of waiting_for_gp_ttrace is now
> excluded by c->lock, so plain __llist_del_all() is safe, while
> free_by_rcu_ttrace / waiting_for_gp still need the atomic variants?

Good point.
>
> [ ... ]
>
> commit 03e0a12c3be3d11fe9a3b87582070b2158a7bb74
> Author: Pu Lehui <pulehui@huawei.com>
>
>>     bpf: Fix UAF due to concurrent consumption of waiting_for_gp_ttrace
>>
>>     Syzkaller repeatedly triggered UAF splats related to nodes in
>>     waiting_for_gp_ttrace within the bpf memalloc:
> [ ... ]
>
>>     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. Thus, the callback
>>     __free_rcu cannot run concurrently, ruling out missing RCU read-side
>>     locks as the cause. And same for free_by_rcu_ttrace list.
> Is the statement "Thus, the callback __free_rcu cannot run concurrently"
> accurate as written?

I think it just means multiple __free_rcu() can not run concurrently.
>
> Looking at the race diagram three paragraphs later, it shows exactly
> that scenario: __free_rcu on CPU1 executing in the middle of
> alloc_bulk's llist_del_first on CPU0. What the RCU argument actually
> establishes is narrower: the GP cannot complete during the particular
> read-side CS it is waiting on, so RCU read-side protection is not what
> is missing.
>
> Nothing prevents a new alloc_bulk() from starting after the GP has
> completed and running concurrently with the callback. __free_rcu runs
> from the rcu_tasks_trace kthread (process context, no irqs disabled, any
> CPU) while alloc_bulk runs from irq_work on the cache's own CPU; on
> PREEMPT_RT alloc_bulk is even preemptible, so same-CPU interleaving is
> possible too.
>
> A reviewer reading this sentence literally would conclude the race in
> the diagram is impossible. Could the paragraph say "RCU read-side
> protection is not what is missing; what is missing is mutual exclusion
> between the two llist consumers"?
>
>>     Note that free_by_rcu_ttrace does not suffer from this issue as it only
>>     has a single active consumer during normal operation.
> Is this premise correct? free_by_rcu_ttrace has two distinct consumers,
> and they are not confined to one CPU:
>
>   - alloc_bulk() memalloc.c:225 (still outside c->lock):
>
>     obj = llist_del_first(&c->free_by_rcu_ttrace);
>
>   - do_call_rcu_ttrace() memalloc.c:322, and drain_mem_cache()
>     memalloc.c:647:
>
>     llist_del_all(&c->free_by_rcu_ttrace)
>
> do_call_rcu_ttrace() is called as do_call_rcu_ttrace(tgt) with
> tgt = c->tgt from free_bulk() (memalloc.c:341, 364) and __free_by_rcu()
> (memalloc.c:370, 383). c->tgt is set to the cache that allocated the
> object, potentially another CPU's cache:
>
> kernel/bpf/memalloc.c:unit_free() {
>     c->tgt = *(struct bpf_mem_cache **)llnode;
> }
>
> So llist_del_all(&tgt->free_by_rcu_ttrace) on CPU1 can run concurrently
> with llist_del_first(&tgt->free_by_rcu_ttrace) in alloc_bulk(tgt) on
> tgt's own CPU. The file's own comment at memalloc.c:220-224 says as
> much: "Other CPUs might llist_add() and llist_del_all() in parallel."
>
> What actually keeps that pair harmless in the common case is the RCU
> deferral the changelog describes earlier, not single-consumer-ness: the
> nodes taken by llist_del_all() are moved to waiting_for_gp_ttrace and
> only freed by __free_rcu() after an RCU tasks trace GP, which cannot
> elapse while alloc_bulk() is inside its RCU read-side section. That
> argument does not cover the two draining paths, which free immediately
> while still only being ordered against alloc_bulk() by
> WRITE_ONCE(c->draining, true) / irq_work_sync().

Er, It seems the UAF problem is still possible for free_by_rcu_ttrace
under PREEMPT_RT. The possible race is shown below:

// CPU=0
// irq work is still busy (e.g., on PREEMPT_RT)
alloc_bulk()
  llist_del_first(&c->free_by_rcu_ttrace)
                                           
            // CPU=1
            bpf_mem_alloc_destroy
                WRITE_ONCE(c->draining, true)
                // wait for CPU 0
                irq_work_sync()

                // CPU=2 (due to cross-cpu free)
                do_call_rcu_ttrace()
                    llist_del_all(&c->free_by_rcu_ttrace)
                    // due to c->draining is True
                    free_all()

The problem is that alloc_bulk may still be running in the irq work
kthread under PREEMPT_RT when unit_alloc() doesn't have any caller any more.
>
> Since the changelog is what a reviewer will use to decide the fix is
> complete, should the sentence be corrected to state the real reason (RCU
> deferral) or should the residual draining-path window be acknowledged?
>
> Leon Hwang suggested in the v2 review that scoped_guard() could be used
> instead of explicit raw_spin_lock_irqsave/raw_spin_unlock_irqrestore to
> simplify the code and avoid needing the flags variable. This is a code
> style improvement that would make the locking more idiomatic. The
> suggestion applies to multiple locations: alloc_bulk(), __free_rcu(),
> do_call_rcu_ttrace(), and drain_mem_cache(). Is this something worth
> addressing before merge?
>
>
> ---
> 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/33055527430