| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzbpVkpCyQX4C0gn13t63LETAfGpbf27rSJKyMcp=ELKDw@mail.gmail.com>
Date: Thu, 16 Oct 2025 11:41:06 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Amery Hung <ameryhung@...il.com>
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
Subject: Re: [RFC PATCH v1 bpf-next 2/4] bpf: Support associating BPF program
with struct_ops
On Wed, Oct 15, 2025 at 3:35 PM Amery Hung <ameryhung@...il.com> wrote:
>
> On Mon, Oct 13, 2025 at 5:10 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Fri, Oct 10, 2025 at 10:49 AM Amery Hung <ameryhung@...il.com> wrote:
> > >
> > > Add a new BPF command BPF_STRUCT_OPS_ASSOCIATE_PROG to allow associating
> > > a BPF program with a struct_ops. This command takes a file descriptor of
> > > a struct_ops map and a BPF program and set prog->aux->st_ops_assoc to
> > > the kdata of the struct_ops map.
> > >
> > > The command does not accept a struct_ops program or a non-struct_ops
> > > map. Programs of a struct_ops map is automatically associated with the
> > > map during map update. If a program is shared between two struct_ops
> > > maps, the first one will be the map associated with the program. The
> > > associated struct_ops map, once set cannot be changed later. This
> > > restriction may be lifted in the future if there is a use case.
> > >
> > > Each associated programs except struct_ops programs of the map will take
> > > a refcount on the map to pin it so that prog->aux->st_ops_assoc, if set,
> > > is always valid. However, it is not guaranteed whether the map members
> > > are fully updated nor is it attached or not. For example, a BPF program
> > > can be associated with a struct_ops map before map_update. The
> > > struct_ops implementer will be responsible for maintaining and checking
> > > the state of the associated struct_ops map before accessing it.
> > >
> > > Signed-off-by: Amery Hung <ameryhung@...il.com>
> > > ---
> > > include/linux/bpf.h | 11 ++++++++++
> > > include/uapi/linux/bpf.h | 16 ++++++++++++++
> > > kernel/bpf/bpf_struct_ops.c | 32 ++++++++++++++++++++++++++++
> > > kernel/bpf/core.c | 6 ++++++
> > > kernel/bpf/syscall.c | 38 ++++++++++++++++++++++++++++++++++
> > > tools/include/uapi/linux/bpf.h | 16 ++++++++++++++
> > > 6 files changed, 119 insertions(+)
> > >
[...]
> > > + } struct_ops_assoc_prog;
> > > +
> > > } __attribute__((aligned(8)));
> > >
> > > /* The description below is an attempt at providing documentation to eBPF
> > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> > > index a41e6730edcf..e57428e1653b 100644
> > > --- a/kernel/bpf/bpf_struct_ops.c
> > > +++ b/kernel/bpf/bpf_struct_ops.c
> > > @@ -528,6 +528,7 @@ static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
> > > for (i = 0; i < st_map->funcs_cnt; i++) {
> > > if (!st_map->links[i])
> > > break;
> > > + bpf_struct_ops_disassoc_prog(st_map->links[i]->prog);
> > > bpf_link_put(st_map->links[i]);
> > > st_map->links[i] = NULL;
> > > }
> > > @@ -801,6 +802,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> > > goto reset_unlock;
> > > }
> > >
> > > + /* Don't stop a program from being reused. prog->aux->st_ops_assoc
> >
> > nit: comment style, we are converging onto /* on separate line
>
> Got it, so I assume it applies to kerne/bpf/* even existing comments
> in the file are netdev style. Is it also the case for
> net/core/filter.c?
yeah
>
>
> >
> > > + * will point to the first struct_ops kdata.
> > > + */
> > > + bpf_struct_ops_assoc_prog(&st_map->map, prog);
> >
> > ignoring error? we should do something better here... poisoning this
> > association altogether if program is used in multiple struct_ops seems
> > like the only thing we can reasonable do, no?
> >
> > > +
> > > link = kzalloc(sizeof(*link), GFP_USER);
> > > if (!link) {
> > > bpf_prog_put(prog);
> >
> > [...]
> >
> > >
> > > +#define BPF_STRUCT_OPS_ASSOCIATE_PROG_LAST_FIELD struct_ops_assoc_prog.prog_fd
> > > +
> >
> > looking at libbpf side, it's quite a mouthful to write out
> > bpf_struct_ops_associate_prog()... maybe let's shorten this to
> > BPF_STRUCT_OPS_ASSOC or BPF_ASSOC_STRUCT_OPS (with the idea that we
> > associate struct_ops with a program). The latter is actually a bit
> > more preferable, because then we can have a meaningful high-level
> > bpf_program__assoc_struct_ops(struct bpf_program *prog, struct bpf_map
> > *map), where map has to be struct_ops. Having bpf_map__assoc_prog() is
> > a bit too generic, as this works only for struct_ops maps.
> >
> > It's all not major, but I think that lends for a bit better naming and
> > more logical usage throughout.
>
> Will change the naming.
thanks
>
> >
> > > +static int struct_ops_assoc_prog(union bpf_attr *attr)
> > > +{
> > > + struct bpf_prog *prog;
> > > + struct bpf_map *map;
> > > + int ret;
> > > +
> > > + if (CHECK_ATTR(BPF_STRUCT_OPS_ASSOCIATE_PROG))
> > > + return -EINVAL;
> > > +
> > > + prog = bpf_prog_get(attr->struct_ops_assoc_prog.prog_fd);
> > > + if (IS_ERR(prog))
> > > + return PTR_ERR(prog);
> > > +
> > > + map = bpf_map_get(attr->struct_ops_assoc_prog.map_fd);
> > > + if (IS_ERR(map)) {
> > > + ret = PTR_ERR(map);
> > > + goto out;
> > > + }
> > > +
> > > + if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
> > > + prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> >
> > you can check prog->type earlier, before getting map itself
>
> Got it. I will make it a separate check right after getting prog.
>
> >
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + ret = bpf_struct_ops_assoc_prog(map, prog);
> > > +out:
> > > + if (ret && !IS_ERR(map))
> >
> > nit: purely stylistic preference, but I'd rather have a clear
> > error-only clean up path, and success with explicit return 0, instead
> > of checking ret or IS_ERR(map)
> >
> > ...
> >
> > /* goto to put_{map,prog}, depending on how far we've got */
> >
> > err = bpf_struct_ops_assoc_prog(map, prog);
> > if (err)
> > goto put_map;
> >
> > return 0;
> >
> > put_map:
> > bpf_map_put(map);
> > put_prog:
> > bpf_prog_put(prog);
> > return err;
>
> I will separate error path out.
great, thanks
>
> >
> >
> > > + bpf_map_put(map);
> > > + bpf_prog_put(prog);
> > > + return ret;
> > > +}
> > > +
> >
> > [...]
Powered by blists - more mailing lists