Re: [PATCH RFC v3 01/10] bpf: Refactor __bpf_async_set_callback()

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 1/9/26 22:18, Andrii Nakryiko wrote:
> On Wed, Jan 7, 2026 at 10:22 AM <bot+bpf-ci@kernel.org> wrote:
>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>> index 9eaa4185e0a7..954bd61310a6 100644
>>> --- a/kernel/bpf/helpers.c
>>> +++ b/kernel/bpf/helpers.c
>>> @@ -1355,55 +1355,36 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
>>>   };
>>>
>>>   static int __bpf_async_set_callback(struct bpf_async_kern *async, void *callback_fn,
>>> -                                 struct bpf_prog_aux *aux, unsigned int flags,
>>> -                                 enum bpf_async_type type)
>>> +                                 struct bpf_prog *prog)
>>>   {
>>> -     struct bpf_prog *prev, *prog = aux->prog;
>>> -     struct bpf_async_cb *cb;
>>> -     int ret = 0;
>>> +     struct bpf_prog *prev;
>>> +     struct bpf_async_cb *cb = async->cb;
>>>
>>> -     if (in_nmi())
>>> -             return -EOPNOTSUPP;
>>> -     __bpf_spin_lock_irqsave(&async->lock);
>>> -     cb = async->cb;
>>> -     if (!cb) {
>>> -             ret = -EINVAL;
>>> -             goto out;
>>> -     }
>>> +     if (!cb)
>>> +             return -EPERM;
>> A small concern: the error code changed from -EINVAL to -EPERM. The verifier
>> comment at kernel/bpf/verifier.c mentions that non-initialized timers
>> "will return -EINVAL". Is the error code change intentional, and should
>> the verifier comment be updated?
>>
> good AI, I asked the same
>
>>> -     if (!atomic64_read(&cb->map->usercnt)) {
>>> -             /* maps with timers must be either held by user space
>>> -              * or pinned in bpffs. Otherwise timer might still be
>>> -              * running even when bpf prog is detached and user space
>>> -              * is gone, since map_release_uref won't ever be called.
>>> -              */
>>> -             ret = -EPERM;
>>> -             goto out;
>>> -     }
>> The usercnt check is removed here. The check still exists in
>> __bpf_async_init(), so a timer cannot be initialized when usercnt is zero.
>> But can there be issues if set_callback is called after usercnt drops to
>> zero on an already-initialized timer? The old code would fail with -EPERM,
>> while the new code proceeds.
>>
> Wasn't this discussed earlier, Mykyta? Do you remember the conclusion?
I think what I missed is to put rcu lock in this function, because the
last prog reference is put in the rcu callback (the change this patchset 
introduces).
This way we make sure that whatever is set here will be put if map's 
usercount goes to 0.
>
>>> -     prev = cb->prog;
>>> -     if (prev != prog) {
>>> -             /* Bump prog refcnt once. Every bpf_timer_set_callback()
>>> -              * can pick different callback_fn-s within the same prog.
>>> -              */
>>> -             prog = bpf_prog_inc_not_zero(prog);
>>> -             if (IS_ERR(prog)) {
>>> -                     ret = PTR_ERR(prog);
>>> -                     goto out;
>>> +     do {
>>> +             if (prog) {
>>> +                     prog = bpf_prog_inc_not_zero(prog);
>>> +                     if (IS_ERR(prog))
>>> +                             return PTR_ERR(prog);
>>>                }
>>> +
>>> +             prev = xchg(&cb->prog, prog);
>>> +             rcu_assign_pointer(cb->callback_fn, callback_fn);
>> Is there a potential race between these two operations? Consider two
>> concurrent callers (with programs P and Q) racing:
>>
>> 1. Thread A: xchg stores P into cb->prog
>> 2. Thread B: xchg stores Q into cb->prog, gets P back
>> 3. Thread B: rcu_assign_pointer stores FB (Q's callback)
>> 4. Thread B: bpf_prog_put(P), B's check passes, B exits
>> 5. Thread A: rcu_assign_pointer stores FA (P's callback), overwrites FB
>> 6. Thread A: check fails (cb->prog is Q, not P), retries
>>
>> After step 5, cb->prog holds Q (with Q's reference) but cb->callback_fn
>> points to FA (from P). If P's refcount drops to zero elsewhere and P is
>> freed, would the timer callback FA become a dangling pointer?
> AI is not completely wrong here, IMO. No, there is no use-after-free
> just yet because program is RCU protected, so we don't have dangling
> pointer just yet.
>
> But. That bpf_prog_inc_not_zero() on retry will fail if P's program
> refcount dropped to zero already. And then once we exit, we'll have
> Q+FA combo, which is not good.
>
> So I think we need to do one bpf_prog_inc_not_zero() outside of the
> loop once and keep that extra ref until we exit. And then instead of
> bpf_prog_inc_not_zero(prog) inside the loop, we do unconditional
> bpf_prog_inc(), which we *know* has to succeed. Less error handling
> inside the loop as well.
>
> Thoughts?
>
>
>> The bpf_timer_cb() function reads cb->callback_fn and calls it directly
>> without verifying it corresponds to cb->prog. The cb->prog field holds
>> the reference keeping the program alive, but if the callback pointer is
>> from a different program that may have been freed, this could be a problem.
>>
>>> +
>>>                if (prev)
>>> -                     /* Drop prev prog refcnt when swapping with new prog */
>>>                        bpf_prog_put(prev);
>>> -             cb->prog = prog;
>>> -     }
>>> -     rcu_assign_pointer(cb->callback_fn, callback_fn);
>>> -out:
>>> -     __bpf_spin_unlock_irqrestore(&async->lock);
>>> -     return ret;
>>> +
>>> +     } while (READ_ONCE(cb->prog) != prog || READ_ONCE(cb->callback_fn) != callback_fn);
>>> +
>>> +     return 0;
>>>   }
>> [ ... ]
>>
>>
>> ---
>> 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/20791345842