> From: Andrii Nakryiko [mailto:andrii.nakryiko@xxxxxxxxx] > Sent: Thursday, July 28, 2022 1:03 AM > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@xxxxxxxxx> > wrote: > > > > From: Joe Burton <jevburton@xxxxxxxxxx> > > > > Add an extensible variant of bpf_obj_get() capable of setting the > > `file_flags` parameter. > > > > This parameter is needed to enable unprivileged access to BPF maps. > > Without a method like this, users must manually make the syscall. > > > > Signed-off-by: Joe Burton <jevburton@xxxxxxxxxx> > > --- > > tools/lib/bpf/bpf.c | 10 ++++++++++ > > tools/lib/bpf/bpf.h | 9 +++++++++ > > tools/lib/bpf/libbpf.map | 1 + > > 3 files changed, 20 insertions(+) > > > > I agree that bpf_obj_get_opts should be separate from bpf_get_fd_opts. > Just because both currently have file_flags in them doesn't mean that > they should/will always stay in sync. So two separate opts for two > separate APIs makes sense to me. > > So I'd accept this patch, but please see a few small things below and > send v3. Thanks! Should map_parse_fds() accept two opts, or just the flags to be set on locally-defined variables? Thanks Roberto > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > > index 5eb0df90eb2b..5acb0e8bd13c 100644 > > --- a/tools/lib/bpf/bpf.c > > +++ b/tools/lib/bpf/bpf.c > > @@ -578,12 +578,22 @@ int bpf_obj_pin(int fd, const char *pathname) > > } > > > > int bpf_obj_get(const char *pathname) > > +{ > > + LIBBPF_OPTS(bpf_obj_get_opts, opts); > > if you were doing it this way, here should be an empty line. But > really you can/should just pass NULL instead of opts in this case. > > > + return bpf_obj_get_opts(pathname, &opts); > > +} > > + > > +int bpf_obj_get_opts(const char *pathname, const struct bpf_obj_get_opts > *opts) > > { > > union bpf_attr attr; > > int fd; > > > > + if (!OPTS_VALID(opts, bpf_obj_get_opts)) > > + return libbpf_err(-EINVAL); > > + > > memset(&attr, 0, sizeof(attr)); > > attr.pathname = ptr_to_u64((void *)pathname); > > + attr.file_flags = OPTS_GET(opts, file_flags, 0); > > > > fd = sys_bpf_fd(BPF_OBJ_GET, &attr, sizeof(attr)); > > return libbpf_err_errno(fd); > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h > > index 88a7cc4bd76f..f31b493b5f9a 100644 > > --- a/tools/lib/bpf/bpf.h > > +++ b/tools/lib/bpf/bpf.h > > @@ -270,8 +270,17 @@ LIBBPF_API int bpf_map_update_batch(int fd, const > void *keys, const void *values > > __u32 *count, > > const struct bpf_map_batch_opts *opts); > > > > +struct bpf_obj_get_opts { > > + size_t sz; /* size of this struct for forward/backward compatibility */ > > + > > + __u32 file_flags; > > please add size_t :0; to avoid non-zero-initialized padding (we do it > in a lot of other opts structs) > > > > +}; > > +#define bpf_obj_get_opts__last_field file_flags > > + > > LIBBPF_API int bpf_obj_pin(int fd, const char *pathname); > > LIBBPF_API int bpf_obj_get(const char *pathname); > > +LIBBPF_API int bpf_obj_get_opts(const char *pathname, > > + const struct bpf_obj_get_opts *opts); > > > > struct bpf_prog_attach_opts { > > size_t sz; /* size of this struct for forward/backward compatibility */ > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > index 0625adb9e888..119e6e1ea7f1 100644 > > --- a/tools/lib/bpf/libbpf.map > > +++ b/tools/lib/bpf/libbpf.map > > @@ -355,6 +355,7 @@ LIBBPF_0.8.0 { > > > > LIBBPF_1.0.0 { > > global: > > + bpf_obj_get_opts; > > bpf_prog_query_opts; > > bpf_program__attach_ksyscall; > > btf__add_enum64; > > -- > > 2.37.0.170.g444d1eabd0-goog > >
