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

[sashiko-bot@kernel.org]

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