Re: [PATCH v2 16/23] libbpf: introduce bpf_map__attach_struct_ops_opts()

Andrii Nakryiko
2025-10-28T17:24:22Z

On Tue, Oct 28, 2025 at 10:07 AM Roman Gushchin
<roman.gushchin@linux.dev> wrote:
>
> bot+bpf-ci@kernel.org writes:
>
> > ```
> > commit b58d54ed56054e3a550e02c324982feb20a2671e
> > Author: Roman Gushchin <roman.gushchin@linux.dev>
> >
> > libbpf: introduce bpf_map__attach_struct_ops_opts()
> >
> > This commit introduces a new API function that extends the existing
> > bpf_map__attach_struct_ops() to accept additional options, specifically
> > allowing attachment of struct_ops maps to cgroups via a relative_fd.
> >
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index b90574f39..be56a5dee 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -13196,12 +13196,19 @@ static int bpf_link__detach_struct_ops(struct bpf_link *link)
> >> return close(link->fd);
> >> }
> >>
> >> -struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
> >> +struct bpf_link *bpf_map__attach_struct_ops_opts(const struct bpf_map *map,
> >> + const struct bpf_struct_ops_opts *opts)
> >> {
> >> + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_opts);
> >> struct bpf_link_struct_ops *link;
> >> __u32 zero = 0;
> >> int err, fd;
> >>
> >> + if (!OPTS_VALID(opts, bpf_struct_ops_opts)) {
> >> + pr_warn("map '%s': invalid opts\n", map->name);
> >> + return libbpf_err_ptr(-EINVAL);
> >> + }
> >> +
> >> if (!bpf_map__is_struct_ops(map)) {
> >> pr_warn("map '%s': can't attach non-struct_ops map\n", map->name);
> >> return libbpf_err_ptr(-EINVAL);
> >
> > [ ... ]
> >
> >> link = calloc(1, sizeof(*link));
> >> if (!link)
> >> return libbpf_err_ptr(-EINVAL);
> >
> > Should this return -ENOMEM instead of -EINVAL? All other calloc failures
> > in libbpf.c return -ENOMEM (see lines 11006, 11802, 12288, 12746, 12829,
> > 12891, 13089, 13129 in the same file).
>
> This code was introduced by e9fc3ce99b34 ("libbpf: Streamline error
> reporting for high-level APIs"), so it's new. I agree that ENOMEM is
> correct, however it might be too late to fix. I'll let libbpf
> maintainers to decide.

yeah, let's fix this to return -ENOMEM

>
> >
> > [ ... ]
> >
> >> - fd = bpf_link_create(map->fd, 0, BPF_STRUCT_OPS, NULL);
> >> + link_opts.cgroup.relative_fd = OPTS_GET(opts, relative_fd, 0);
> >> +
> >> + fd = bpf_link_create(map->fd, 0, BPF_STRUCT_OPS, &link_opts);
> >
> > Is the expected_revision field being propagated correctly? The new
> > bpf_struct_ops_opts structure includes an expected_revision field
> > (see libbpf.h), and bpf_link_create() with BPF_STRUCT_OPS reads
> > opts->cgroup.expected_revision (see bpf.c line 889), but this code only
> > sets relative_fd in link_opts. Should it also include:
> >
> > link_opts.cgroup.expected_revision = OPTS_GET(opts,
> > expected_revision, 0);
>
> Correct, fixed.

I haven't looked at the rest of patches, but this use of relative_fd
seems wrong. relative_fd/relative_id and expected_version are there
for ordering of programs within the same attach target (e.g., same
cgroup). If you just want to specify cgroup to attach to, I think you
should use attr.link_create.target_fd (which is already handled a bit
lower generically)

>
> Thanks!