Re: [PATCH bpf v2] bpf: Fix UAF due to concurrent consumption of waiting_for_gp_ttrace
From: Pu Lehui
Date: Sat Aug 29 2026 - 05:52:13 EST
++to Alexei
On 2026/8/27 22:40, Hou Tao wrote:
On 8/27/2026 5:24 PM, bot+bpf-ci@xxxxxxxxxx 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)Can this loop iteration count be bounded? The critical section covers
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);
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)Does the comment above need updating? It says "Except for
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);
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@xxxxxxxxxx>
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: RetireIs the statement "Thus, the callback __free_rcu cannot run concurrently"
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.
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 onlyIs this premise correct? free_by_rcu_ttrace has two distinct consumers,
has a single active consumer during normal operation.
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()
Hi Hou Tao,
Thanks for pointing out that.
Indeed, free_by_rcu_ttrace also requires spinlock protection. However, converting it entirely to spinlock + non-atomic __llist_* ops would introduce unnecessary locking overhead into the fast-freeing path (particularly in enque_to_free(), which runs in a tight loop inside free_bulk()).
Therefore, I propose adding the spinlock only at the actual consumer race points while keeping the atomic llist_* ops against concurrent lockless producers. This keeps the logic simple, avoids penalizing the hot free path, and completely eliminates the UAF.
How about the following?
diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c
index e9662db7198f..942480d399ec 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 {
@@ -214,25 +215,27 @@ 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;
-
- for (; i < cnt; i++) {
- obj = llist_del_first(&c->waiting_for_gp_ttrace);
- if (!obj)
- break;
- add_obj_to_free_list(c, obj);
+ 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);
+ }
+ if (i >= cnt)
+ return;
+
+ 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;
@@ -279,8 +282,12 @@ 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;
+
+ scoped_guard(raw_spinlock_irqsave, &c->lock)
+ llnode = llist_del_all(&c->waiting_for_gp_ttrace);
- free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
+ free_all(c, llnode, !!c->percpu_size);
atomic_set(&c->call_rcu_ttrace_in_progress, 0);
}
@@ -300,7 +307,8 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
if (unlikely(READ_ONCE(c->draining))) {
- llnode = llist_del_all(&c->free_by_rcu_ttrace);
+ scoped_guard(raw_spinlock_irqsave, &c->lock)
+ llnode = llist_del_all(&c->free_by_rcu_ttrace);
free_all(c, llnode, !!c->percpu_size);
}
return;
@@ -535,6 +543,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 +566,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 +618,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);
}
--
2.34.1
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