Message-ID: <CAMB2axMsX6g2ByUtM31yqVvsTO9gGgDUKsMstpZtJLcv_90qVw@mail.gmail.com>
Date: Tue, 4 Nov 2025 10:03:47 -0800
From: Amery Hung <ameryhung@...il.com>
To: bot+bpf-ci@...nel.org
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com, 
	andrii@...nel.org, daniel@...earbox.net, tj@...nel.org, martin.lau@...nel.org, 
	kernel-team@...a.com, ast@...nel.org, eddyz87@...il.com, 
	yonghong.song@...ux.dev, clm@...a.com, ihor.solodrai@...ux.dev
Subject: Re: [PATCH bpf-next v5 2/7] bpf: Support associating BPF program with struct_ops

On Tue, Nov 4, 2025 at 9:54 AM <bot+bpf-ci@...nel.org> wrote:
>
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index a47d67db3..0f71030c0 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
>
> [ ... ]
>
> > @@ -2026,6 +2028,9 @@ static inline void bpf_module_put(const void *data, struct module *owner)
> >               module_put(owner);
> >  }
> >  int bpf_struct_ops_link_create(union bpf_attr *attr);
> > +int bpf_prog_assoc_struct_ops(struct bpf_prog *prog, struct bpf_map *map);
> > +void bpf_prog_disassoc_struct_ops(struct bpf_prog *prog);
> > +void *bpf_prog_get_assoc_struct_ops(const struct bpf_prog_aux *aux);
> >  u32 bpf_struct_ops_id(const void *kdata);
>
> [ ... ]
>
> > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> > index a41e6730e..0a19842da 100644
> > --- a/kernel/bpf/bpf_struct_ops.c
> > +++ b/kernel/bpf/bpf_struct_ops.c
>
> [ ... ]
>
> > @@ -1394,6 +1413,77 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
> >       return err;
> >  }
> >
> > +int bpf_prog_assoc_struct_ops(struct bpf_prog *prog, struct bpf_map *map)
> > +{
> > +     struct bpf_map *st_ops_assoc;
> > +
> > +     guard(mutex)(&prog->aux->st_ops_assoc_mutex);
> > +
> > +     st_ops_assoc = prog->aux->st_ops_assoc;
> > +
> > +     if (st_ops_assoc && st_ops_assoc == map)
> > +             return 0;
> > +
> > +     if (st_ops_assoc) {
> > +             if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> > +                     return -EBUSY;
> > +
> > +             WRITE_ONCE(prog->aux->st_ops_assoc, BPF_PTR_POISON);
> > +     } else {
> > +             if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> > +                     bpf_map_inc(map);
> > +
> > +             WRITE_ONCE(prog->aux->st_ops_assoc, map);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +void bpf_prog_disassoc_struct_ops(struct bpf_prog *prog)
> > +{
> > +     struct bpf_map *st_ops_assoc;
> > +
> > +     guard(mutex)(&prog->aux->st_ops_assoc_mutex);
> > +
> > +     st_ops_assoc = prog->aux->st_ops_assoc;
> > +
> > +     if (!st_ops_assoc || st_ops_assoc == BPF_PTR_POISON)
> > +             return;
> > +
> > +     if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> > +             bpf_map_put(st_ops_assoc);
> > +
> > +     WRITE_ONCE(prog->aux->st_ops_assoc, NULL);
> > +}
> > +
> > +/*
> > + * Get a reference to the struct_ops struct (i.e., kdata) associated with a
> > + * program.
> > + *
> > + * If the returned pointer is not NULL, it must points to a valid and
> > + * initialized struct_ops. The struct_ops may or may not be attached.
> > + * Kernel struct_ops implementers are responsible for tracking and checking
> > + * the state of the struct_ops if the use case requires an attached struct_ops.
> > + */
> > +void *bpf_prog_get_assoc_struct_ops(const struct bpf_prog_aux *aux)
> > +{
> > +     struct bpf_map *st_ops_assoc = READ_ONCE(aux->st_ops_assoc);
> > +     struct bpf_struct_ops_map *st_map;
> > +
> > +     if (!st_ops_assoc || st_ops_assoc == BPF_PTR_POISON)
> > +             return NULL;
> > +
> > +     st_map = (struct bpf_struct_ops_map *)st_ops_assoc;
> > +
> > +     if (smp_load_acquire(&st_map->kvalue.common.state) == BPF_STRUCT_OPS_STATE_INIT) {
> > +             bpf_map_put(st_ops_assoc);
>                 ^^^^
>
> Does bpf_prog_get_assoc_struct_ops() need to call bpf_map_put() here?
>

Right. bpf_map_put() is not needed here. It was some artifact in v4.
Will respin and fix it.

> The function comment says "Get a reference to the struct_ops struct"
> but the function never calls bpf_map_get/inc() to acquire a reference.
> It only reads aux->st_ops_assoc via READ_ONCE().
>
> When the state check fails (INIT state), the function calls bpf_map_put()
> which drops the reference that was acquired in bpf_prog_assoc_struct_ops().
> But on the success path below, it returns kdata without any refcount
> operation.
>
> This creates an imbalance:
> - INIT state path: drops a reference, returns NULL
> - Non-INIT path: no refcount change, returns kdata
>
> The caller has no way to know whether the reference count was modified.
> Also, the function is EXPORTED but doesn't document the reference counting
> semantics clearly.
>
> > +             return NULL;
> > +     }
> > +
> > +     return &st_map->kvalue.data;
> > +}
> > +EXPORT_SYMBOL_GPL(bpf_prog_get_assoc_struct_ops);
>
> Can this function race with bpf_prog_disassoc_struct_ops()?
>
> Since bpf_prog_get_assoc_struct_ops() reads aux->st_ops_assoc via
> READ_ONCE() without holding the mutex or acquiring a reference, there's
> a window where:
>
> Thread A: reads st_ops_assoc pointer
> Thread B: calls bpf_prog_disassoc_struct_ops()->bpf_map_put() (drops last ref)
> Thread A: dereferences st_map->kvalue.common.state (use-after-free)
>
> For non-struct_ops programs, bpf_prog_assoc_struct_ops() holds a
> reference via bpf_map_inc(). This reference is dropped in
> bpf_prog_disassoc_struct_ops() when the program is freed. Without
> acquiring a new reference in bpf_prog_get_assoc_struct_ops(), the map
> can be freed during the race window above.

This is fine. bpf_prog_get_assoc_struct_ops() should only be called by
kfunc (i.e., when a BPF program is still alive). Refcount has been
bumped during the BPF_PROG_ASSOC_STRUCT_OPS command or during async
callback registration.

>
> [ ... ]
>
>
> ---
> 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

Powered by blists - more mailing lists