On Fri, Jul 22, 2022 at 10:19 AM Roberto Sassu <roberto.sassu@xxxxxxxxxx> wrote:
>
> Define a new data structure called bpf_get_fd_opts, with the member flags,
> to be used by callers of the _opts variants of bpf_*_get_fd_by_id() and
> bpf_obj_get() to specify the permissions needed for the operations on the
> obtained file descriptor.
>
> Define only one data structure for all variants, as the open_flags field in
> bpf_attr for bpf_*_get_fd_by_id_opts() and file_flags for
> bpf_obj_get_opts() have the same meaning. Also, it would avoid the
as I replied elsewhere, bpf_obj_get_opts() is a different bpf()
command, so it makes sense to use a separate opts struct for it
> confusion when the same bpftool function calls both
> bpf_*_get_fd_by_id_opts() and bpf_obj_get_opts() (e.g. map_parse_fds()).
>
> Then, introduce the new feature FEAT_GET_FD_BY_ID_OPEN_FLAGS to determine
> whether or not the kernel supports setting open_flags for
> bpf_*_get_fd_by_id() functions (except for bpf_map_get_fd_by_id(), which
> already can get it). If the feature is not detected, the _opts variants
> ignore flags in the bpf_get_fd_opts structure and leave open_flags to zero.
>
> Finally, introduce bpf_prog_get_fd_by_id_opts(), to let the caller pass the
> newly introduced data structure. Keep the existing bpf_prog_get_fd_by_id(),
> and call bpf_prog_get_fd_by_id_opts() with NULL as opts argument.
>
> Currently, setting permissions in the data structure has no effect, as the
> kernel does not evaluate them. The new variant has been introduced anyway
> for symmetry with bpf_map_get_fd_by_id_opts(). Evaluating permissions could
> be done in future kernel versions.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
> tools/lib/bpf/bpf.c | 37 ++++++++++++++++++++++++++++++++-
> tools/lib/bpf/bpf.h | 11 ++++++++++
> tools/lib/bpf/libbpf.c | 4 ++++
> tools/lib/bpf/libbpf.map | 1 +
> tools/lib/bpf/libbpf_internal.h | 3 +++
> 5 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 5eb0df90eb2b..9014a61bca83 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -910,18 +910,53 @@ int bpf_link_get_next_id(__u32 start_id, __u32 *next_id)
> return bpf_obj_get_next_id(start_id, next_id, BPF_LINK_GET_NEXT_ID);
> }
>
> -int bpf_prog_get_fd_by_id(__u32 id)
> +static int
> +bpf_prog_get_fd_by_id_opts_check(__u32 id, const struct bpf_get_fd_opts *opts,
> + bool check_support)
> {
> union bpf_attr attr;
> int fd;
>
> + if (!OPTS_VALID(opts, bpf_get_fd_opts))
> + return libbpf_err(-EINVAL);
> +
> memset(&attr, 0, sizeof(attr));
> attr.prog_id = id;
> + if (!check_support ||
> + kernel_supports(NULL, FEAT_GET_FD_BY_ID_OPEN_FLAGS))
> + attr.open_flags = OPTS_GET(opts, flags, 0);
low-level APIs from libbpf/bpf.h, as a general rule, don't do such
feature detection and don't ignore user-provided flags just because
kernel doesn't support them. So I think this is wrong approach, just
pass through user flags and user will have to make sure to pass
correct value (potentially just zero, or just pass NULL as opts).
>
> fd = sys_bpf_fd(BPF_PROG_GET_FD_BY_ID, &attr, sizeof(attr));
> return libbpf_err_errno(fd);
> }
>
[...]
