Re: [RFC PATCH bpf-next v7 04/11] libbpf: introduce bpf_map__attach_struct_ops_opts()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `expected_revision` field in `struct bpf_struct_ops_opts` is silently ignored.
- [High] Silent fallback to global struct_ops attachment when `target_fd` is provided but the map lacks `BPF_F_LINK`.
--

commit 38a993fa39aa42f01d7ad014d6fc85b69daffad3
Author: Roman Gushchin <roman.gushchin@linux.dev>

libbpf: introduce bpf_map__attach_struct_ops_opts()

This commit introduces an extended version of bpf_map__attach_struct_ops()
which takes an additional struct bpf_struct_ops_opts argument. This enables
passing a target_fd and the BPF_F_CGROUP_FD flag for cgroup attachment.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1e8688975d16d..a1b54da1ded25 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
[ ... ]
> @@ -13724,7 +13731,9 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>  		return &link->link;
>  	}
>  

[Severity: High]
If a user provides a cgroup FD via opts->target_fd to restrict the attachment
to a specific cgroup, but the map was not created with BPF_F_LINK, does this
silently fall back to global registration?

Looking slightly above this block:

	if (!(map->def.map_flags & BPF_F_LINK)) {
		/* w/o a real link */
		link->link.fd = map->fd;
		link->map_fd = -1;
		return &link->link;
	}

If BPF_F_LINK is missing, it returns early before extracting and applying
opts->target_fd. Should we explicitly reject the operation if opts->target_fd
or opts->flags are set but the map lacks BPF_F_LINK, to prevent subverting
the user's intent to isolate the struct_ops?

> -	fd = bpf_link_create(map->fd, 0, BPF_STRUCT_OPS, NULL);
> +	link_opts.flags = OPTS_GET(opts, flags, 0);
> +	target_fd = OPTS_GET(opts, target_fd, 0);

[Severity: High]
Is the expected_revision field intended to be extracted here?

It is defined in struct bpf_struct_ops_opts but never extracted via
OPTS_GET() or checked. If users attempt to use expected_revision to prevent
TOCTOU races during atomic replacements, will they experience silent
failures because the field is completely ignored?

Should the library either remove this field from the struct, or explicitly
check for OPTS_GET(opts, expected_revision, 0) and return an error?

> +	fd = bpf_link_create(map->fd, target_fd, BPF_STRUCT_OPS, &link_opts);
>  	if (fd < 0) {
>  		free(link);
>  		return libbpf_err_ptr(fd);
[ ... ]
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index bba4e84643967..18af178547ad5 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
[ ... ]
> @@ -945,6 +945,20 @@ bpf_program__attach_cgroup_opts(const struct bpf_program *prog, int cgroup_fd,
>  struct bpf_map;
>  
>  LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map);
> +
> +struct bpf_struct_ops_opts {
> +	/* size of this struct, for forward/backward compatibility */
> +	size_t sz;
> +	__u32 flags;
> +	__u32 target_fd;
> +	__u64 expected_revision;
> +	size_t :0;
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779760876.git.zhuhui@kylinos.cn?part=4