Re: [PATCH bpf-next v5 3/7] bpf: Pin associated struct_ops when registering async callback

Amery Hung
2025-11-04T18:10:44Z

On Tue, Nov 4, 2025 at 10:03 AM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 865b0dae3..557570479 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
>
> [ ... ]
>
> > +static int bpf_async_res_get(struct bpf_async_res *res, struct bpf_prog *prog)
> > +{
> > + struct bpf_map *st_ops_assoc = NULL;
> > + int err;
> > +
> > + prog = bpf_prog_inc_not_zero(prog);
> > + if (IS_ERR(prog))
> > + return PTR_ERR(prog);
> > +
> > + st_ops_assoc = READ_ONCE(prog->aux->st_ops_assoc);
> > + if (prog->type == BPF_PROG_TYPE_STRUCT_OPS &&
> > + st_ops_assoc && st_ops_assoc != BPF_PTR_POISON) {
> > + st_ops_assoc = bpf_map_inc_not_zero(st_ops_assoc);
> > + if (IS_ERR(st_ops_assoc)) {
> > + err = PTR_ERR(st_ops_assoc);
> > + goto put_prog;
> > + }
> > + }
>
> Can this race with bpf_prog_disassoc_struct_ops()? Between reading
> st_ops_assoc and incrementing it, another thread could dissociate the
> map:

This is fine. struct_ops map will wait for struct_ops BPF programs to
finish running (an RCU gp) before freeing itself. Therefore, when
another thread is freeing the map and this thread is calling
bpf_async_res_get(), if this thread still sees
prog->aux->st_ops_assoc, bpf_map_inc_not_zero() will fail and stop the
async callback from being registered.

>
> bpf_async_res_get():
> READ_ONCE(prog->aux->st_ops_assoc) // reads valid map pointer
>
> bpf_prog_disassoc_struct_ops():
> guard(mutex)(&prog->aux->st_ops_assoc_mutex)
> WRITE_ONCE(prog->aux->st_ops_assoc, NULL)
> bpf_map_put(st_ops_assoc) // potentially frees map
>
> bpf_async_res_get():
> bpf_map_inc_not_zero(st_ops_assoc) // use-after-free
>
> The map could be freed via RCU and memory reused before
> bpf_map_inc_not_zero() executes. Other functions that access
> st_ops_assoc (bpf_prog_assoc_struct_ops and bpf_prog_disassoc_struct_ops)
> hold prog->aux->st_ops_assoc_mutex. Additionally, bpf_map_inc_not_zero's
> documentation states "map_idr_lock should have been held or the map
> should have been protected by rcu read lock."
>
> Should bpf_async_res_get() hold the st_ops_assoc_mutex or an RCU read
> lock around the st_ops_assoc access?
>
> > +
> > + res->prog = prog;
> > + res->st_ops_assoc = st_ops_assoc;
> > + return 0;
> > +put_prog:
> > + bpf_prog_put(prog);
> > + return err;
> > +}
>
> [ ... ]
>
>
> ---
> 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/19077679684