[PATCH bpf-next v3 0/6] bpf: Fix async callback use-after-free on map deletion

[PATCH bpf-next v3 0/6] bpf: Fix async callback use-after-free on map deletion

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

This series fixes a use-after-free when a BPF program schedules task
work, workqueue work or a timer through a value pointer retained across
map deletion. Review of the earlier task-work series [1] pointed out
that cancellation can leave the deleted value open to reinitialization.

The original reader keeps the element alive while it queues new work,
but that does not make the new callback part of the grace period freeing
the element. The final destructor cannot stop a callback that has already
started outside that waiting set from accessing the freed key or value.
Patch 2 includes the interleaving and a historical KASAN report from the
unmodified kernel.

Following Alexei's suggestions [2, 3], check task-work membership after
claiming PENDING, and check wq/timer membership only in the shared setter.
Leave async init unchanged. An unconfigured object cannot start a BPF
callback, so a newly initialized object rejected by the setter stays idle
until normal cleanup.

Keep the cancellation-order fixes separate: mark task work FREED or clear
cb->map before detaching the object. Only the first invalidator clears
the slot and disposes of its map-owned context reference. No hashtab or
allocator changes are needed.

Patches 1-3 fix task work and add its regression test. Patches 4-6 fix the
shared wq/timer paths, adjust rhash_timer and add the wq regression test.

Testing on x86-64 QEMU/KVM with KASAN, PROVE_LOCKING, PROVE_RCU and
DEBUG_ATOMIC_SLEEP:

- task_work_stress, wq and rhash_timer pass with the full series.
- The same test binary on the unfixed base fails exactly the three
  deleted-value -ENOENT checks: task-work scheduling and wq/timer
  set_callback return 0. These are expected negative-control failures.
- Directed two-canceller checks for task work and wq also pass.

Changes since v2:
- Combine wq/timer membership checking in the shared setter; leave init
  unchanged (Alexei).
- Check membership through the map's existing lookup operation and
  remove the extra RCU guard around the lookup (Alexei).
- Let only the first invalidator detach task work or an async callback
  object and dispose of its map-owned reference.
- Reuse task_work_stress, wq and rhash_timer to reduce the added test
  code; retain the recycled timer-field check.

v2: https://lore.kernel.org/all/20261106095747.471718-1-luyun_611@163.com/

Changes since v1:
- Replace slot poisoning and allocator hooks with lookup checks (Alexei).
- Separate FREED-before-detach from task-work membership (Alexei).
- Fix the same publication problem for wq and timers (Alexei, Mykyta).
- Expand task-work coverage and add wq/timer regression tests.

v1: https://lore.kernel.org/all/20261028073525.1035605-1-luyun_611@163.com/

[1] https://lore.kernel.org/all/20261012061915.231254-1-luyun_611@163.com/
[2] https://lore.kernel.org/all/DMGN8VM2BAOR.28NRNN51MCI8I@gmail.com/
[3] https://lore.kernel.org/all/DMOAQI1S0JBS.3N3WETHD2L6KQ@gmail.com/

Yun Lu (6):
  bpf: Mark task work FREED before detaching it
  bpf: Check task work value membership before scheduling
  selftests/bpf: Test task work scheduling on a deleted value
  bpf: Invalidate async callbacks before detaching them
  bpf: Check async callback value membership in the setter
  selftests/bpf: Test wq set_callback on a deleted value

 include/uapi/linux/bpf.h                      |   2 +
 kernel/bpf/helpers.c                          |  90 ++++++++++++++++++++++---
 tools/include/uapi/linux/bpf.h                |   2 +
 ...ing/selftests/bpf/prog_tests/rhash_timer.c |  24 +++----
 ...elftests/bpf/prog_tests/task_work_stress.c |  18 +++++
 .../testing/selftests/bpf/progs/rhash_timer.c |  26 ++-----
 ...ing/selftests/bpf/progs/task_work_stress.c |  17 +++++
 tools/testing/selftests/bpf/progs/wq.c        |  20 ++++++
 8 files changed, 156 insertions(+), 43 deletions(-)

base-commit: bd75869299f1d4ff6e9f45117cce28f8badee1cd

-- 
2.43.0

[PATCH bpf-next v3 1/6] bpf: Mark task work FREED before detaching it

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

bpf_task_work_cancel_and_free() clears the value's context slot before
marking the context FREED. A concurrent deletion can observe the empty
slot and return while the detached context can still enter RUNNING:

  uref cleanup                 delete                  target task
  xchg(slot, NULL) gets C
                               unlink E
                               cancel sees NULL
                               return
                                                       C -> RUNNING
  xchg(C->state, FREED)

A preallocated HASH uref walk racing with deletion allows this missed
cancellation. This deletion does not itself free the element.

Mark FREED before clearing the slot. Only the first FREED transition
clears the slot and drops or transfers its map-owned context reference.
Until that canceller clears the slot, no replacement can be published,
so a plain slot store cannot overwrite a new context.

Reading the slot no longer acquires exclusive ownership of C. Protect
that access with regular RCU while another canceller can release C, and
preserve unlink-to-slot ordering with a full barrier, including when the
slot is NULL.

Fixes: 38aa7003e369 ("bpf: task work scheduling kfuncs")
Signed-off-by: Yun Lu <luyun@kylinos.cn>

---
 kernel/bpf/helpers.c |  12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 937a0d7d6839..4b252ca61166 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -5174,11 +5174,21 @@ void bpf_task_work_cancel_and_free(void *val)
 	struct bpf_task_work_ctx *ctx;
 	enum bpf_task_work_state state;
 
-	ctx = xchg(&twk->ctx, NULL);
+	/* Another canceller can release the map-owned context reference. */
+	guard(rcu)();
+	/* Order a preceding unlink before observing an empty slot. */
+	smp_mb();
+	ctx = READ_ONCE(twk->ctx);
 	if (!ctx)
 		return;
 
+	/* Stop callbacks before making the context invisible to deletion. */
 	state = xchg(&ctx->state, BPF_TW_FREED);
+	/* Only the first FREED transition detaches ctx and owns the map's ref. */
+	if (state == BPF_TW_FREED)
+		return;
+	WRITE_ONCE(twk->ctx, NULL);
+
 	if (state == BPF_TW_SCHEDULED) {
 		/* run in irq_work to avoid locks in NMI */
 		init_irq_work(&ctx->irq_work, bpf_task_work_cancel_scheduled);
-- 
2.43.0

Re: [PATCH bpf-next v3 1/6] bpf: Mark task work FREED before detaching it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: race in bpf_task_work_cancel_and_free() on concurrent
  cancellation
--

commit ba93530a5f1bdfb22fe067f66a5dec0d9c8f3f5a
Author: Yun Lu <luyun@kylinos.cn>

bpf: Mark task work FREED before detaching it

This commit marks a BPF task work context as BPF_TW_FREED before clearing
the map value's context slot in bpf_task_work_cancel_and_free() so a
concurrent map deletion does not observe an empty slot while a detached
context can still transition to BPF_TW_RUNNING.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 937a0d7d68390..4b252ca61166b 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -5174,11 +5174,21 @@ void bpf_task_work_cancel_and_free(void *val)
>  	struct bpf_task_work_ctx *ctx;
>  	enum bpf_task_work_state state;
>  
> -	ctx = xchg(&twk->ctx, NULL);
> +	/* Another canceller can release the map-owned context reference. */
> +	guard(rcu)();
> +	/* Order a preceding unlink before observing an empty slot. */
> +	smp_mb();
> +	ctx = READ_ONCE(twk->ctx);
>  	if (!ctx)
>  		return;
>  
> +	/* Stop callbacks before making the context invisible to deletion. */
>  	state = xchg(&ctx->state, BPF_TW_FREED);
> +	/* Only the first FREED transition detaches ctx and owns the map's ref. */
> +	if (state == BPF_TW_FREED)
> +		return;
> +	WRITE_ONCE(twk->ctx, NULL);

[Severity: Medium]
Can a second concurrent bpf_task_work_cancel_and_free() return with the
BPF_TW_FREED context still attached to twk->ctx, or miss a newly published
context?

array_map_update_elem() updates an element in place and calls
bpf_obj_cancel_fields() without a lock:

  array_map_update_elem()
    bpf_obj_cancel_fields()
      bpf_task_work_cancel_and_free()
        ctx = READ_ONCE(twk->ctx)
        xchg(&ctx->state, BPF_TW_FREED)
        <irq>
          array_map_update_elem()
            bpf_obj_cancel_fields()
              bpf_task_work_cancel_and_free()
                ctx = READ_ONCE(twk->ctx)
                state = xchg(&ctx->state, BPF_TW_FREED)
                if (state == BPF_TW_FREED)
                  return;
            bpf_task_work_schedule_signal()
              bpf_task_work_acquire_ctx()
                bpf_task_work_fetch_ctx()  <-- sees old BPF_TW_FREED ctx
        WRITE_ONCE(twk->ctx, NULL)

Because twk->ctx has not been cleared when the second canceller returns,
bpf_task_work_fetch_ctx() sees the non-NULL pointer and returns the dead
BPF_TW_FREED context instead of allocating a new one, causing
bpf_task_work_acquire_ctx() to fail with -EBUSY.

Conversely, if the second canceller reads the old context C1 before the
first canceller clears twk->ctx, and a concurrent bpf_task_work_fetch_ctx()
installs a new context C2 once twk->ctx becomes NULL, the second canceller
sees C1->state == BPF_TW_FREED and returns without detaching or cancelling
C2.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111024333.228633-1-luyun_611@163.com?part=1

[PATCH bpf-next v3 2/6] bpf: Check task work value membership before scheduling

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

Cancellation clears the task-work slot, but a BPF program can retain the
deleted element's value pointer and publish a new context through it.
The context reference keeps C alive, not the element at C->map_val:

  1. A BPF program looks up E and retains its value pointer.
  2. Another task deletes E, cancels its task work and clears the slot.
  3. The original reader publishes C, queues task work and leaves its
     trace read-side section.
  4. C enters RUNNING outside the grace period's waiting set for E,
     before the final destructor marks C FREED.
  5. The destructor frees E while the callback can still access it.

A controlled reproducer on the unmodified kernel reported:

  BUG: KASAN: slab-use-after-free in array_map_update_elem+0x4b8/0x7f0
  Read of size 8 at addr ffff88800e61be98 by task uaf_repro/91
  Call Trace:
   __asan_memcpy+0x23/0x60
   array_map_update_elem+0x4b8/0x7f0
   bpf_prog_976f3fa6d36fbef6_cb_probe_step+0xaf/0xe5
   bpf_prog_ac7215eae4609044_process_work_cb+0x12c/0x234
   bpf_task_work_callback+0x351/0x970
   task_work_run+0x116/0x200

  Freed by task 9:
   kfree+0x1b8/0x550
   free_all.isra.0+0x5b/0x110
   __free_rcu+0x87/0x1b0
   srcu_invoke_callbacks+0x360/0x5c0

After publishing C and claiming PENDING, look up its key and require the
returned pointer to match the retained value. Before irq_work is queued,
either lookup rejects the removed value or deletion finds C and marks it
FREED.

Return -ENOENT on mismatch. Conditionally roll PENDING back to STANDBY to
preserve a concurrent FREED, and drop this attempt's callback reference.
Leave the idle context for normal cancellation and reclamation.

Fixes: 38aa7003e369 ("bpf: task work scheduling kfuncs")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Yun Lu <luyun@kylinos.cn>

---
 kernel/bpf/helpers.c |  38 ++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 4b252ca61166..a08b2af56ee6 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1151,6 +1151,14 @@ static void *map_key_from_value(struct bpf_map *map, void *value, u32 *arr_idx)
 	return (void *)value - round_up(map->key_size, 8);
 }
 
+/* Is the element that holds this value still in the map? */
+static bool bpf_map_value_is_linked(struct bpf_map *map, void *value)
+{
+	u32 idx;
+
+	return map->ops->map_lookup_elem(map, map_key_from_value(map, value, &idx)) == value;
+}
+
 enum bpf_async_type {
 	BPF_ASYNC_TYPE_TIMER = 0,
 	BPF_ASYNC_TYPE_WQ,
@@ -4859,6 +4867,19 @@ static struct bpf_task_work_ctx *bpf_task_work_acquire_ctx(struct bpf_task_work
 		return ERR_PTR(-EBUSY);
 	}
 
+	/*
+	 * The prog can still hold a value of an element that was deleted from the map.
+	 * bpf_task_work_cancel_and_free() is done with it and nothing stops a new callback
+	 * when the element is freed. The ctx is published and PENDING at this point: either
+	 * the lookup doesn't find the element or bpf_task_work_cancel_and_free() finds the ctx.
+	 * An idle ctx stays in the element until the element is reused or freed.
+	 */
+	if (!bpf_map_value_is_linked(map, (void *)tw - map->record->task_work_off)) {
+		(void)cmpxchg(&ctx->state, BPF_TW_PENDING, BPF_TW_STANDBY);
+		bpf_task_work_ctx_put(ctx);
+		return ERR_PTR(-ENOENT);
+	}
+
 	/*
 	 * If no process or bpffs is holding a reference to the map, no new callbacks should be
 	 * scheduled. This does not address any race or correctness issue, but rather is a policy
@@ -4927,7 +4948,16 @@ release_prog:
  * @callback: pointer to BPF subprogram to call
  * @aux: pointer to bpf_prog_aux of the caller BPF program, implicitly set by the verifier
  *
- * Return: 0 if task work has been scheduled successfully, negative error code otherwise
+ * When the membership check is reached, scheduling is rejected with -ENOENT
+ * if the current map lookup does not return this value; a value inserted
+ * again at the same address is the current identity. Earlier errors keep
+ * their own codes. A live value with no user reference or a busy context is
+ * rejected with -EBUSY. Return 0 only means the scheduling irq_work was
+ * queued; it does not guarantee that the callback will run.
+ *
+ * Return: 0 if the scheduling irq_work was queued, -ENOENT if the value is
+ * no longer the current map value at the membership check, -EBUSY for a busy
+ * context or zero usercnt, or another negative error code on failure.
  */
 __bpf_kfunc int bpf_task_work_schedule_signal(struct task_struct *task, struct bpf_task_work *tw,
 					      void *map__const_map, bpf_task_work_callback_t callback,
@@ -4945,7 +4975,11 @@ __bpf_kfunc int bpf_task_work_schedule_signal(struct task_struct *task, struct b
  * @callback: pointer to BPF subprogram to call
  * @aux: pointer to bpf_prog_aux of the caller BPF program, implicitly set by the verifier
  *
- * Return: 0 if task work has been scheduled successfully, negative error code otherwise
+ * See bpf_task_work_schedule_signal() for return semantics.
+ *
+ * Return: 0 if the scheduling irq_work was queued, -ENOENT for a removed
+ * value, -EBUSY for a busy context or zero usercnt, or another negative
+ * error code on failure.
  */
 __bpf_kfunc int bpf_task_work_schedule_resume(struct task_struct *task, struct bpf_task_work *tw,
 					      void *map__const_map, bpf_task_work_callback_t callback,
-- 
2.43.0

Re: [PATCH bpf-next v3 2/6] bpf: Check task work value membership before scheduling

[bot+bpf-ci]

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

> bpf: Check task work value membership before scheduling

TL;DR: Nit only: the kernel-doc for bpf_task_work_schedule_signal() and
_resume() describes the return values twice, in the description and in
Return:. A single concise Return: line may suffice.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -4927,7 +4948,16 @@ static int bpf_task_work_schedule(struct task_struct *task, struct bpf_task_work
>   * @callback: pointer to BPF subprogram to call
>   * @aux: pointer to bpf_prog_aux of the caller BPF program, implicitly set by the verifier
>   *
> - * Return: 0 if task work has been scheduled successfully, negative error code otherwise
> + * When the membership check is reached, scheduling is rejected with -ENOENT
> + * if the current map lookup does not return this value; a value inserted
> + * again at the same address is the current identity. Earlier errors keep
> + * their own codes. A live value with no user reference or a busy context is
> + * rejected with -EBUSY. Return 0 only means the scheduling irq_work was
> + * queued; it does not guarantee that the callback will run.
> + *
> + * Return: 0 if the scheduling irq_work was queued, -ENOENT if the value is
> + * no longer the current map value at the membership check, -EBUSY for a busy
> + * context or zero usercnt, or another negative error code on failure.
>   */
>  __bpf_kfunc int bpf_task_work_schedule_signal(struct task_struct *task, struct bpf_task_work *tw,

This isn't a bug, but the kernel-doc for bpf_task_work_schedule_signal()
now describes the -ENOENT, -EBUSY and 0 results twice, once in the
description paragraph and again in the Return: section.

Would a single Return: line listing 0, -ENOENT (value no longer in the
map) and -EBUSY be enough here?

The same question applies to bpf_task_work_schedule_resume(), which says
"See bpf_task_work_schedule_signal() for return semantics." and then
repeats a Return: line anyway.  The other kfunc docs in
kernel/bpf/helpers.c use a single concise Return: line.


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

[PATCH bpf-next v3 3/6] selftests/bpf: Test task work scheduling on a deleted value

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

Add a deleted-value case to task_work_stress. A syscall program looks up
a HASH value, deletes it and schedules task work through the retained
pointer. Assert that scheduling returns -ENOENT; positive step codes
distinguish setup failures from the unfixed kernel's return value of 0.

Reuse process_work(), which only counts callbacks and does not dereference
the old key or value.

Signed-off-by: Yun Lu <luyun@kylinos.cn>

---
 ...elftests/bpf/prog_tests/task_work_stress.c |  18 ++++++++++++++++++
 ...ing/selftests/bpf/progs/task_work_stress.c |  17 +++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/task_work_stress.c b/tools/testing/selftests/bpf/prog_tests/task_work_stress.c
index 450d17d91a56..90d6d696537d 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_work_stress.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_work_stress.c
@@ -121,8 +121,26 @@ cleanup:
 	task_work_stress__destroy(skel);
 }
 
+static void task_work_deleted(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+	struct task_work_stress *skel;
+	int err;
+
+	skel = task_work_stress__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.schedule_deleted_task_work),
+				     &opts);
+	if (ASSERT_OK(err, "schedule_deleted_task_work"))
+		ASSERT_EQ((int)opts.retval, -ENOENT, "deleted schedule rejected");
+	task_work_stress__destroy(skel);
+}
+
 void test_task_work_stress(void)
 {
+	if (test__start_subtest("deleted"))
+		task_work_deleted();
 	if (test__start_subtest("no_delete"))
 		task_work_run(false);
 	if (test__start_subtest("with_delete"))
diff --git a/tools/testing/selftests/bpf/progs/task_work_stress.c b/tools/testing/selftests/bpf/progs/task_work_stress.c
index 1d4378f351ef..5f47133ad308 100644
--- a/tools/testing/selftests/bpf/progs/task_work_stress.c
+++ b/tools/testing/selftests/bpf/progs/task_work_stress.c
@@ -71,3 +71,20 @@ int delete_task_work(void *ctx)
 		__sync_fetch_and_add(&delete_success, 1);
 	return 0;
 }
+
+SEC("syscall")
+int schedule_deleted_task_work(void *ctx)
+{
+	struct elem empty = {}, *value;
+	int key = 0;
+
+	if (bpf_map_update_elem(&hmap, &key, &empty, BPF_NOEXIST))
+		return 1;
+	value = bpf_map_lookup_elem(&hmap, &key);
+	if (!value)
+		return 2;
+	if (bpf_map_delete_elem(&hmap, &key))
+		return 3;
+	return bpf_task_work_schedule_signal(bpf_get_current_task_btf(),
+					     &value->tw, &hmap, process_work);
+}
-- 
2.43.0

[PATCH bpf-next v3 4/6] bpf: Invalidate async callbacks before detaching them

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

__bpf_async_init() publishes a callback object C before checking usercnt.
On a non-preallocated HASH map, a syscall BPF program can pause after
publication while another CPU configures and queues C on the live value.
If usercnt then drops to zero, init's failure cleanup can race with uref
cleanup and deletion. The old code permits this interleaving:

  1. Init's cleanup detaches C and pauses before clearing C->map.
  2. Uref cleanup and deletion see an empty slot; deletion retires the
     element for reclamation.
  3. C's queued WQ callback enters outside the freeing GP's waiting set
     and reads a valid map, program and callback function.
  4. Init's cleanup resumes and invalidates C, but cancel_work() does not
     wait for the running callback.
  5. The element is freed while the callback can still access its value.

Invalidate C before detaching it. Use xchg() on cb->map to select the only
canceller that clears the slot and consumes the map-owned reference.
Until it clears the slot, init cannot publish a replacement, so a plain
slot store cannot overwrite a new callback object.

Protect the initial access to C with regular RCU. Order unlink before the
slot load, and acquire prior invalidation when the slot is already empty.

Fixes: 1bfbc267ec91 ("bpf: Enable bpf_timer and bpf_wq in any context")
Signed-off-by: Yun Lu <luyun@kylinos.cn>

---
 kernel/bpf/helpers.c |  15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index a08b2af56ee6..716be7a852b5 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1747,15 +1747,18 @@ static void bpf_async_cancel_and_free(struct bpf_async_kern *async)
 {
 	struct bpf_async_cb *cb;
 
-	if (!READ_ONCE(async->cb))
+	/* Another canceller can release the map-owned cb reference. */
+	guard(rcu)();
+	/* Order unlink before reading async->cb, including an empty slot. */
+	smp_mb();
+	/* Acquire any invalidation published before the slot was cleared. */
+	cb = smp_load_acquire(&async->cb);
+	/* Only the first invalidator detaches cb and drops the map's reference. */
+	if (!cb || !xchg(&cb->map, NULL))
 		return;
 
-	cb = xchg(&async->cb, NULL);
-	if (!cb)
-		return;
+	WRITE_ONCE(async->cb, NULL);
 
-	/* Callbacks that run after this see NULL and return. */
-	WRITE_ONCE(cb->map, NULL);
 	bpf_async_update_prog_callback(cb, NULL, NULL);
 	/*
 	 * No refcount_inc_not_zero(&cb->refcnt) here. Dropping the last
-- 
2.43.0

Re: [PATCH bpf-next v3 4/6] bpf: Invalidate async callbacks before detaching them

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: stale async->cb pointer when losing xchg() race in
  bpf_async_cancel_and_free()
--

commit 686504449c958e635d8414edba153676c8d63adc
Author: Yun Lu <luyun@kylinos.cn>

bpf: Invalidate async callbacks before detaching them

This commit changes bpf_async_cancel_and_free() to clear cb->map with xchg()
under RCU before clearing async->cb, so that a callback object is invalidated
before it is detached from the map value slot.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a08b2af56ee64..716be7a852b58 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1747,15 +1747,18 @@ static void bpf_async_cancel_and_free(struct bpf_async_kern *async)
>  {
>  	struct bpf_async_cb *cb;
>  
> -	if (!READ_ONCE(async->cb))
> +	/* Another canceller can release the map-owned cb reference. */
> +	guard(rcu)();
> +	/* Order unlink before reading async->cb, including an empty slot. */
> +	smp_mb();
> +	/* Acquire any invalidation published before the slot was cleared. */
> +	cb = smp_load_acquire(&async->cb);
> +	/* Only the first invalidator detaches cb and drops the map's reference. */
> +	if (!cb || !xchg(&cb->map, NULL))
>  		return;
>  
> -	cb = xchg(&async->cb, NULL);
> -	if (!cb)
> -		return;
> +	WRITE_ONCE(async->cb, NULL);

[Severity: Medium]
Can a losing canceller return from bpf_async_cancel_and_free() while
async->cb is still non-NULL?

Before this patch, xchg(&async->cb, NULL) guaranteed that async->cb was
NULL whenever bpf_async_cancel_and_free() returned.  With the split between
xchg(&cb->map, NULL) and WRITE_ONCE(async->cb, NULL), a second canceller
can observe the invalidated cb and return early before the slot is cleared:

CPU0                              CPU1
array_map_update_elem()           array_map_update_elem()
  bpf_obj_cancel_fields()           copy_map_value()
    bpf_async_cancel_and_free()     bpf_obj_cancel_fields()
      cb = smp_load_acquire(...)      bpf_async_cancel_and_free()
      xchg(&cb->map, NULL)              cb = smp_load_acquire(...)
                                        !xchg(&cb->map, NULL)
                                        return;
                                  bpf_timer_init()
                                    __bpf_async_init()
                                      READ_ONCE(async->cb) != NULL
                                      return -EBUSY;  <-- spurious failure
      WRITE_ONCE(async->cb, NULL)

The same race occurs on a single CPU if an interrupt or NMI BPF program
updates the element and calls bpf_timer_init() or bpf_wq_init() while the
interrupted task is between xchg(&cb->map, NULL) and
WRITE_ONCE(async->cb, NULL), or when check_and_cancel_fields() races with
map_release_uref() before recycling a hash map element.

Having the losing canceller also clear the slot (for example via
cmpxchg(&async->cb, cb, NULL)) before returning would preserve the
guarantee that async->cb is NULL on return without overwriting a newly
published callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111024333.228633-1-luyun_611@163.com?part=4

[PATCH bpf-next v3 5/6] bpf: Check async callback value membership in the setter

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

A BPF program can retain a value pointer across deletion, initialize a
new timer or workqueue object on it, and configure and start a callback.
That callback can run outside the grace period freeing the element and
access its freed key or value. Keeping the callback object alive does not
keep the element alive.

Only __bpf_async_set_callback() installs cb->prog and cb->callback_fn.
Check membership once there, ordering the slot load before the lookup
with a full barrier. Return -ENOENT if the object was invalidated or the
lookup no longer returns cb->value.

Leave __bpf_async_init() unchanged. The start helpers reject an object
without a program, so a rejected, newly initialized object stays idle
until normal cleanup. Document the setter error and adjust rhash_timer
to expect -ENOENT while retaining its recycled timer-field check.

Fixes: 1bfbc267ec91 ("bpf: Enable bpf_timer and bpf_wq in any context")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Yun Lu <luyun@kylinos.cn>

---
 include/uapi/linux/bpf.h                      |   2 ++
 kernel/bpf/helpers.c                          |  25 ++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h                |   2 ++
 ...ing/selftests/bpf/prog_tests/rhash_timer.c |  24 +++++++++--------------
 .../testing/selftests/bpf/progs/rhash_timer.c |  26 +++++++------------------
 5 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 62de0919db72..0d0d56f87e09 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5478,6 +5478,8 @@ union bpf_attr {
  *	Return
  *		0 on success.
  *		**-EINVAL** if *timer* was not initialized with bpf_timer_init() earlier.
+ *		**-ENOENT** if the containing map element is no longer in the map
+ *		or the timer was invalidated by map cleanup.
  *		**-EPERM** if *timer* is in a map that doesn't have any user references.
  *		The user space should either hold a file descriptor to a map with timers
  *		or pin such map in bpffs. When map is unpinned or file descriptor is
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 716be7a852b5..9fa75bcd5e77 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1552,11 +1552,25 @@ static int __bpf_async_set_callback(struct bpf_async_kern *async, void *callback
 				    struct bpf_prog *prog)
 {
 	struct bpf_async_cb *cb;
+	struct bpf_map *map;
 
 	cb = READ_ONCE(async->cb);
 	if (!cb)
 		return -EINVAL;
 
+	/*
+	 * The prog can still hold a value of an element that was deleted from the map.
+	 * bpf_async_cancel_and_free() is done with it and nothing stops a new callback
+	 * when the element is freed. cb is published at this point: either the lookup
+	 * doesn't find the element or bpf_async_cancel_and_free() finds cb.
+	 * Nothing runs without callback_fn. An idle cb stays in the element until the
+	 * element is reused or freed.
+	 */
+	smp_mb();
+	map = READ_ONCE(cb->map);
+	if (!map || !bpf_map_value_is_linked(map, cb->value))
+		return -ENOENT;
+
 	return bpf_async_update_prog_callback(cb, prog, callback_fn);
 }
 
@@ -3530,6 +3544,17 @@ __bpf_kfunc int bpf_wq_start(struct bpf_wq *wq, unsigned int flags)
 	}
 }
 
+/**
+ * bpf_wq_set_callback - Set the callback of a BPF workqueue
+ * @wq: pointer to the bpf_wq field
+ * @callback_fn: callback function
+ * @flags: must be zero
+ * @aux: implicit program metadata
+ *
+ * Return: 0 on success, -EINVAL for invalid flags or an uninitialized @wq,
+ * -ENOENT if the containing map element is no longer in the map or the
+ * workqueue was invalidated by map cleanup, or another negative error.
+ */
 __bpf_kfunc int bpf_wq_set_callback(struct bpf_wq *wq,
 				    int (callback_fn)(void *map, int *key, void *value),
 				    unsigned int flags,
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 62de0919db72..0d0d56f87e09 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5478,6 +5478,8 @@ union bpf_attr {
  *	Return
  *		0 on success.
  *		**-EINVAL** if *timer* was not initialized with bpf_timer_init() earlier.
+ *		**-ENOENT** if the containing map element is no longer in the map
+ *		or the timer was invalidated by map cleanup.
  *		**-EPERM** if *timer* is in a map that doesn't have any user references.
  *		The user space should either hold a file descriptor to a map with timers
  *		or pin such map in bpffs. When map is unpinned or file descriptor is
diff --git a/tools/testing/selftests/bpf/prog_tests/rhash_timer.c b/tools/testing/selftests/bpf/prog_tests/rhash_timer.c
index 3aad9fc02e06..87287e038a98 100644
--- a/tools/testing/selftests/bpf/prog_tests/rhash_timer.c
+++ b/tools/testing/selftests/bpf/prog_tests/rhash_timer.c
@@ -52,7 +52,7 @@ void test_rhash_timer(void)
 	LIBBPF_OPTS(bpf_test_run_opts, opts);
 	struct rhash_timer *skel = NULL;
 	cpu_set_t old_mask;
-	int map_fd = -1, arm_fd, cancel_fd;
+	int map_fd = -1, init_fd, cancel_fd;
 	bool affinity_set = false;
 	__u64 key = 1;
 	int attempt, err;
@@ -69,8 +69,8 @@ void test_rhash_timer(void)
 	map_fd = bpf_map__fd(skel->maps.timer_map);
 	if (!ASSERT_GE(map_fd, 0, "timer_map fd"))
 		goto out;
-	arm_fd = bpf_program__fd(skel->progs.arm_deleted_timer);
-	if (!ASSERT_GE(arm_fd, 0, "arm_deleted_timer fd"))
+	init_fd = bpf_program__fd(skel->progs.init_deleted_timer);
+	if (!ASSERT_GE(init_fd, 0, "init_deleted_timer fd"))
 		goto out;
 	cancel_fd = bpf_program__fd(skel->progs.cancel_recycled_timer);
 	if (!ASSERT_GE(cancel_fd, 0, "cancel_recycled_timer fd"))
@@ -81,28 +81,22 @@ void test_rhash_timer(void)
 		goto out;
 
 	for (attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
-		err = run_prog(arm_fd, &opts);
+		err = run_prog(init_fd, &opts);
 		if (err) {
-			ASSERT_OK(err, "arm_deleted_timer");
+			ASSERT_OK(err, "init_deleted_timer");
 			goto out;
 		}
-		if (skel->bss->armed != attempt + 1) {
-			ASSERT_EQ(skel->bss->armed, attempt + 1, "armed");
+		if (skel->bss->initialized != attempt + 1) {
+			ASSERT_EQ(skel->bss->initialized, attempt + 1, "initialized");
 			goto out;
 		}
 		if (skel->bss->timer_init_err) {
 			ASSERT_OK(skel->bss->timer_init_err, "timer_init_err");
 			goto out;
 		}
-		if (skel->bss->timer_set_callback_err) {
-			ASSERT_OK(skel->bss->timer_set_callback_err,
-				  "timer_set_callback_err");
+		if (!ASSERT_EQ(skel->bss->timer_set_callback_err, -ENOENT,
+			       "deleted setter rejected"))
 			goto out;
-		}
-		if (skel->bss->timer_start_err) {
-			ASSERT_OK(skel->bss->timer_start_err, "timer_start_err");
-			goto out;
-		}
 
 		if ((attempt + 1) % RCU_SYNC_INTERVAL == 0) {
 			err = kern_sync_rcu();
diff --git a/tools/testing/selftests/bpf/progs/rhash_timer.c b/tools/testing/selftests/bpf/progs/rhash_timer.c
index 2e06a463c605..49bc3602fa12 100644
--- a/tools/testing/selftests/bpf/progs/rhash_timer.c
+++ b/tools/testing/selftests/bpf/progs/rhash_timer.c
@@ -5,7 +5,6 @@
 #include <bpf/bpf_helpers.h>
 
 #define CLOCK_MONOTONIC 1
-#define TIMER_NSEC (60ULL * 1000 * 1000 * 1000)
 
 struct timer_value {
 	struct bpf_timer timer;
@@ -20,11 +19,10 @@ struct {
 	__type(value, struct timer_value);
 } timer_map SEC(".maps");
 
-u64 armed;
+u64 initialized;
 u64 cancelled;
 long timer_init_err;
 long timer_set_callback_err;
-long timer_start_err;
 long timer_cancel_err;
 
 static int timer_cb(void *map, u64 *key, struct timer_value *value)
@@ -32,8 +30,8 @@ static int timer_cb(void *map, u64 *key, struct timer_value *value)
 	return 0;
 }
 
-static long arm_timer_cb(struct bpf_map *map, u64 *key,
-			 struct timer_value *value, void *ctx)
+static long init_timer_cb(struct bpf_map *map, u64 *key,
+			  struct timer_value *value, void *ctx)
 {
 	u64 key_copy = *key;
 	long err;
@@ -48,19 +46,9 @@ static long arm_timer_cb(struct bpf_map *map, u64 *key,
 		return 1;
 	}
 
-	err = bpf_timer_set_callback(&value->timer, timer_cb);
-	if (err) {
-		timer_set_callback_err = err;
-		return 1;
-	}
-
-	err = bpf_timer_start(&value->timer, TIMER_NSEC, BPF_F_TIMER_CPU_PIN);
-	if (err) {
-		timer_start_err = err;
-		return 1;
-	}
+	timer_set_callback_err = bpf_timer_set_callback(&value->timer, timer_cb);
 
-	__sync_fetch_and_add(&armed, 1);
+	__sync_fetch_and_add(&initialized, 1);
 	return 1;
 }
 
@@ -82,9 +70,9 @@ static long cancel_timer_cb(struct bpf_map *map, u64 *key,
 }
 
 SEC("syscall")
-int arm_deleted_timer(void *ctx)
+int init_deleted_timer(void *ctx)
 {
-	bpf_for_each_map_elem(&timer_map, arm_timer_cb, NULL, 0);
+	bpf_for_each_map_elem(&timer_map, init_timer_cb, NULL, 0);
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH bpf-next v3 5/6] bpf: Check async callback value membership in the setter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: missing map usercnt check in __bpf_async_set_callback()
--

commit a30152ffd79aa0871058c367266aaec41f12d33b
Author: Yun Lu <luyun@kylinos.cn>

bpf: Check async callback value membership in the setter

This commit checks map element membership in __bpf_async_set_callback() so
that timers and workqueues initialized on deleted map elements cannot have a
callback installed and started.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 716be7a852b58..9fa75bcd5e77f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1552,11 +1552,25 @@ static int __bpf_async_set_callback(struct bpf_async_kern *async, void *callback
>  				    struct bpf_prog *prog)
>  {
>  	struct bpf_async_cb *cb;
> +	struct bpf_map *map;
>  
>  	cb = READ_ONCE(async->cb);
>  	if (!cb)
>  		return -EINVAL;
>  
> +	/*
> +	 * The prog can still hold a value of an element that was deleted from the map.
> +	 * bpf_async_cancel_and_free() is done with it and nothing stops a new callback
> +	 * when the element is freed. cb is published at this point: either the lookup
> +	 * doesn't find the element or bpf_async_cancel_and_free() finds cb.
> +	 * Nothing runs without callback_fn. An idle cb stays in the element until the
> +	 * element is reused or freed.
> +	 */
> +	smp_mb();
> +	map = READ_ONCE(cb->map);
> +	if (!map || !bpf_map_value_is_linked(map, cb->value))
> +		return -ENOENT;
> +
>  	return bpf_async_update_prog_callback(cb, prog, callback_fn);
>  }

[Severity: High]
Does __bpf_async_set_callback() also need to check atomic64_read(&map->usercnt)
and return -EPERM when it is zero?

Because __bpf_async_init() leaves an idle cb attached to an unlinked element
in a non-preallocated hash or rhash map's bpf_mem_alloc cache while
map->usercnt > 0, a subsequent bpf_map_put_uref() only walks linked elements
in htab_free_malloced_internal_structs() / rhtab_map_free_internal_structs()
and never clears that unlinked element's cb.

If a loaded BPF program then recycles that element back into the map via
bpf_map_update_elem() after map->usercnt has reached 0, copy_map_value()
preserves the timer/wq field and bpf_map_value_is_linked() succeeds here.
Installing cb->prog after map_release_uref() has already run pins the BPF
program and map in a permanent reference cycle, or allows a callback on an
inner map to race with non-synchronous cancellation in htab_map_free() /
rhtab_map_free() and use the freed map and element after they are kfreed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111024333.228633-1-luyun_611@163.com?part=5

[PATCH bpf-next v3 6/6] selftests/bpf: Test wq set_callback on a deleted value

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

Add a deleted-value case to wq.c using its existing NO_PREALLOC HASH map
and callback. Look up and delete a value, initialize its workqueue, and
assert that bpf_wq_set_callback() returns -ENOENT. Do not start work on
the deleted value.

Signed-off-by: Yun Lu <luyun@kylinos.cn>

---
 tools/testing/selftests/bpf/progs/wq.c |  20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/wq.c b/tools/testing/selftests/bpf/progs/wq.c
index 25be2cd9d42c..4bd663d427ab 100644
--- a/tools/testing/selftests/bpf/progs/wq.c
+++ b/tools/testing/selftests/bpf/progs/wq.c
@@ -3,6 +3,7 @@
  */
 
 #include "bpf_experimental.h"
+#include <errno.h>
 #include <bpf/bpf_helpers.h>
 #include "bpf_misc.h"
 #include "../test_kmods/bpf_testmod_kfunc.h"
@@ -138,6 +139,25 @@ static int wq_cb_sleepable(void *map, int *key, void *value)
 	return 0;
 }
 
+SEC("syscall")
+__retval(-ENOENT)
+long test_deleted_wq(void *ctx)
+{
+	struct hmap_elem empty = {}, *value;
+	int key = 0;
+
+	if (bpf_map_update_elem(&hmap_malloc, &key, &empty, BPF_NOEXIST))
+		return 1;
+	value = bpf_map_lookup_elem(&hmap_malloc, &key);
+	if (!value)
+		return 2;
+	if (bpf_map_delete_elem(&hmap_malloc, &key))
+		return 3;
+	if (bpf_wq_init(&value->work, &hmap_malloc, 0))
+		return 4;
+	return bpf_wq_set_callback(&value->work, wq_callback, 0);
+}
+
 SEC("tc")
 /* test that workqueues can be used from an array */
 __retval(0)
-- 
2.43.0