From: Joe Burton <[email protected]>
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 <[email protected]>
---
tools/lib/bpf/bpf.c | 10 ++++++++++
tools/lib/bpf/bpf.h | 9 +++++++++
tools/lib/bpf/libbpf.map | 1 +
3 files changed, 20 insertions(+)
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);
+ 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;
+};
+#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
On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <[email protected]> wrote:
>
> From: Joe Burton <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Stanislav Fomichev <[email protected]>
For context:
We've found this out while we were trying to add support for unpriv
processes to open pinned r-x maps.
Maybe this deserves a test as well? Not sure.
> ---
> tools/lib/bpf/bpf.c | 10 ++++++++++
> tools/lib/bpf/bpf.h | 9 +++++++++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 20 insertions(+)
>
> 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);
> + 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;
> +};
> +#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
>
On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu <[email protected]> wrote:
>
> > From: Stanislav Fomichev [mailto:[email protected]]
> > Sent: Tuesday, July 19, 2022 10:40 PM
> > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <[email protected]>
> > wrote:
> > >
> > > From: Joe Burton <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > Reviewed-by: Stanislav Fomichev <[email protected]>
> >
> > For context:
> > We've found this out while we were trying to add support for unpriv
> > processes to open pinned r-x maps.
> > Maybe this deserves a test as well? Not sure.
>
> Hi Stanislav, Joe
>
> I noticed now this patch. I'm doing a broader work to add opts
> to bpf_*_get_fd_by_id(). I also adjusted permissions of bpftool
> depending on the operation type (e.g. show, dump: BPF_F_RDONLY).
>
> Will send it soon (I'm trying to solve an issue with the CI, where
> libbfd is not available in the VM doing actual tests).
Is something like this patch included in your series as well? Can you
use this new interface or do you need something different?
> Roberto
>
> > > ---
> > > tools/lib/bpf/bpf.c | 10 ++++++++++
> > > tools/lib/bpf/bpf.h | 9 +++++++++
> > > tools/lib/bpf/libbpf.map | 1 +
> > > 3 files changed, 20 insertions(+)
> > >
> > > 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);
> > > + 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;
> > > +};
> > > +#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
> > >
On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu <[email protected]> wrote:
>
> > From: Stanislav Fomichev [mailto:[email protected]]
> > Sent: Thursday, July 21, 2022 12:38 AM
> > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu <[email protected]>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > <[email protected]>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > <[email protected]>
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Joe Burton <[email protected]>
> > > > > > >
> > > > > > > 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 <[email protected]>
> > > > > >
> > > > > > Reviewed-by: Stanislav Fomichev <[email protected]>
> > > > > >
> > > > > > For context:
> > > > > > We've found this out while we were trying to add support for unpriv
> > > > > > processes to open pinned r-x maps.
> > > > > > Maybe this deserves a test as well? Not sure.
> > > > >
> > > > > Hi Stanislav, Joe
> > > > >
> > > > > I noticed now this patch. I'm doing a broader work to add opts
> > > > > to bpf_*_get_fd_by_id(). I also adjusted permissions of bpftool
> > > > > depending on the operation type (e.g. show, dump: BPF_F_RDONLY).
> > > > >
> > > > > Will send it soon (I'm trying to solve an issue with the CI, where
> > > > > libbfd is not available in the VM doing actual tests).
> > > >
> > > > Is something like this patch included in your series as well? Can you
> > > > use this new interface or do you need something different?
> > >
> > > It is very similar. Except that I called it bpf_get_fd_opts, as it
> > > is shared with the bpf_*_get_fd_by_id() functions. The member
> > > name is just flags, plus an extra u32 for alignment.
> >
> > We can bikeshed the naming, but we've been using existing conventions
> > where opts fields match syscall fields, that seems like a sensible
> > thing to do?
>
> The only problem is that bpf_*_get_fd_by_id() functions would
> set the open_flags member of bpf_attr.
>
> Flags would be good for both, even if not exact. Believe me,
> duplicating the opts would just create more confusion.
Wait, that's completely different, right? We are talking here about
BPF_OBJ_GET (which has related BPF_OBJ_PIN).
Your GET_XXX_BY_ID are different so you'll still have to have another
wrapper with opts?
> > > It needs to be shared, as there are functions in bpftool calling
> > > both. Since the meaning of flags is the same, seems ok sharing.
> >
> > So I guess there are no objections to the current patch? If it gets
> > accepted, you should be able to drop some of your code and use this
> > new bpf_obj_get_opts..
>
> If you use a name good also for bpf_*_get_fd_by_id() and flags
> as structure member name, that would be ok.
>
> Roberto
>
> > > Roberto
> > >
> > > > > Roberto
> > > > >
> > > > > > > ---
> > > > > > > tools/lib/bpf/bpf.c | 10 ++++++++++
> > > > > > > tools/lib/bpf/bpf.h | 9 +++++++++
> > > > > > > tools/lib/bpf/libbpf.map | 1 +
> > > > > > > 3 files changed, 20 insertions(+)
> > > > > > >
> > > > > > > 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);
> > > > > > > + 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;
> > > > > > > +};
> > > > > > > +#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
> > > > > > >
On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu <[email protected]> wrote:
>
> > From: Stanislav Fomichev [mailto:[email protected]]
> > Sent: Wednesday, July 20, 2022 5:57 PM
> > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu <[email protected]>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <[email protected]>
> > > > wrote:
> > > > >
> > > > > From: Joe Burton <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > >
> > > > Reviewed-by: Stanislav Fomichev <[email protected]>
> > > >
> > > > For context:
> > > > We've found this out while we were trying to add support for unpriv
> > > > processes to open pinned r-x maps.
> > > > Maybe this deserves a test as well? Not sure.
> > >
> > > Hi Stanislav, Joe
> > >
> > > I noticed now this patch. I'm doing a broader work to add opts
> > > to bpf_*_get_fd_by_id(). I also adjusted permissions of bpftool
> > > depending on the operation type (e.g. show, dump: BPF_F_RDONLY).
> > >
> > > Will send it soon (I'm trying to solve an issue with the CI, where
> > > libbfd is not available in the VM doing actual tests).
> >
> > Is something like this patch included in your series as well? Can you
> > use this new interface or do you need something different?
>
> It is very similar. Except that I called it bpf_get_fd_opts, as it
> is shared with the bpf_*_get_fd_by_id() functions. The member
> name is just flags, plus an extra u32 for alignment.
We can bikeshed the naming, but we've been using existing conventions
where opts fields match syscall fields, that seems like a sensible
thing to do?
> It needs to be shared, as there are functions in bpftool calling
> both. Since the meaning of flags is the same, seems ok sharing.
So I guess there are no objections to the current patch? If it gets
accepted, you should be able to drop some of your code and use this
new bpf_obj_get_opts..
> Roberto
>
> > > Roberto
> > >
> > > > > ---
> > > > > tools/lib/bpf/bpf.c | 10 ++++++++++
> > > > > tools/lib/bpf/bpf.h | 9 +++++++++
> > > > > tools/lib/bpf/libbpf.map | 1 +
> > > > > 3 files changed, 20 insertions(+)
> > > > >
> > > > > 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);
> > > > > + 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;
> > > > > +};
> > > > > +#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
> > > > >
On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu <[email protected]> wrote:
>
> > From: Stanislav Fomichev [mailto:[email protected]]
> > Sent: Thursday, July 21, 2022 12:48 AM
> > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu <[email protected]>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > <[email protected]>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > <[email protected]>
> > > > > > wrote:
> > > > > > >
> > > > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > <[email protected]>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > From: Joe Burton <[email protected]>
> > > > > > > > >
> > > > > > > > > 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 <[email protected]>
> > > > > > > >
> > > > > > > > Reviewed-by: Stanislav Fomichev <[email protected]>
> > > > > > > >
> > > > > > > > For context:
> > > > > > > > We've found this out while we were trying to add support for unpriv
> > > > > > > > processes to open pinned r-x maps.
> > > > > > > > Maybe this deserves a test as well? Not sure.
> > > > > > >
> > > > > > > Hi Stanislav, Joe
> > > > > > >
> > > > > > > I noticed now this patch. I'm doing a broader work to add opts
> > > > > > > to bpf_*_get_fd_by_id(). I also adjusted permissions of bpftool
> > > > > > > depending on the operation type (e.g. show, dump: BPF_F_RDONLY).
> > > > > > >
> > > > > > > Will send it soon (I'm trying to solve an issue with the CI, where
> > > > > > > libbfd is not available in the VM doing actual tests).
> > > > > >
> > > > > > Is something like this patch included in your series as well? Can you
> > > > > > use this new interface or do you need something different?
> > > > >
> > > > > It is very similar. Except that I called it bpf_get_fd_opts, as it
> > > > > is shared with the bpf_*_get_fd_by_id() functions. The member
> > > > > name is just flags, plus an extra u32 for alignment.
> > > >
> > > > We can bikeshed the naming, but we've been using existing conventions
> > > > where opts fields match syscall fields, that seems like a sensible
> > > > thing to do?
> > >
> > > The only problem is that bpf_*_get_fd_by_id() functions would
> > > set the open_flags member of bpf_attr.
> > >
> > > Flags would be good for both, even if not exact. Believe me,
> > > duplicating the opts would just create more confusion.
> >
> > Wait, that's completely different, right? We are talking here about
> > BPF_OBJ_GET (which has related BPF_OBJ_PIN).
> > Your GET_XXX_BY_ID are different so you'll still have to have another
> > wrapper with opts?
>
> Yes, they have different wrappers, just accept the same opts as
> obj_get(). From bpftool subcommands you want to set the correct
> permission, and propagate it uniformly to bpf_*_get_fd_by_id()
> or obj_get(). See map_parse_fds().
I don't think they are accepting the same opts.
For our case, we care about:
struct { /* anonymous struct used by BPF_OBJ_* commands */
__aligned_u64 pathname;
__u32 bpf_fd;
__u32 file_flags;
};
For your case, you care about:
struct { /* anonymous struct used by BPF_*_GET_*_ID */
union {
__u32 start_id;
__u32 prog_id;
__u32 map_id;
__u32 btf_id;
__u32 link_id;
};
__u32 next_id;
__u32 open_flags;
};
So your new _opts libbpf routine should be independent of what Joe is
doing here.
> Roberto
>
> > > > > It needs to be shared, as there are functions in bpftool calling
> > > > > both. Since the meaning of flags is the same, seems ok sharing.
> > > >
> > > > So I guess there are no objections to the current patch? If it gets
> > > > accepted, you should be able to drop some of your code and use this
> > > > new bpf_obj_get_opts..
> > >
> > > If you use a name good also for bpf_*_get_fd_by_id() and flags
> > > as structure member name, that would be ok.
> > >
> > > Roberto
> > >
> > > > > Roberto
> > > > >
> > > > > > > Roberto
> > > > > > >
> > > > > > > > > ---
> > > > > > > > > tools/lib/bpf/bpf.c | 10 ++++++++++
> > > > > > > > > tools/lib/bpf/bpf.h | 9 +++++++++
> > > > > > > > > tools/lib/bpf/libbpf.map | 1 +
> > > > > > > > > 3 files changed, 20 insertions(+)
> > > > > > > > >
> > > > > > > > > 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);
> > > > > > > > > + 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;
> > > > > > > > > +};
> > > > > > > > > +#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
> > > > > > > > >
On Wed, Jul 20, 2022 at 4:12 PM Roberto Sassu <[email protected]> wrote:
>
> > From: Stanislav Fomichev [mailto:[email protected]]
> > Sent: Thursday, July 21, 2022 1:09 AM
> > On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu <[email protected]>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > Sent: Thursday, July 21, 2022 12:48 AM
> > > > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu
> > <[email protected]>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > > > <[email protected]>
> > > > > > wrote:
> > > > > > >
> > > > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > > > <[email protected]>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > > > <[email protected]>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Joe Burton <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > 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 <[email protected]>
> > > > > > > > > >
> > > > > > > > > > Reviewed-by: Stanislav Fomichev <[email protected]>
> > > > > > > > > >
> > > > > > > > > > For context:
> > > > > > > > > > We've found this out while we were trying to add support for
> > unpriv
> > > > > > > > > > processes to open pinned r-x maps.
> > > > > > > > > > Maybe this deserves a test as well? Not sure.
> > > > > > > > >
> > > > > > > > > Hi Stanislav, Joe
> > > > > > > > >
> > > > > > > > > I noticed now this patch. I'm doing a broader work to add opts
> > > > > > > > > to bpf_*_get_fd_by_id(). I also adjusted permissions of bpftool
> > > > > > > > > depending on the operation type (e.g. show, dump:
> > BPF_F_RDONLY).
> > > > > > > > >
> > > > > > > > > Will send it soon (I'm trying to solve an issue with the CI, where
> > > > > > > > > libbfd is not available in the VM doing actual tests).
> > > > > > > >
> > > > > > > > Is something like this patch included in your series as well? Can you
> > > > > > > > use this new interface or do you need something different?
> > > > > > >
> > > > > > > It is very similar. Except that I called it bpf_get_fd_opts, as it
> > > > > > > is shared with the bpf_*_get_fd_by_id() functions. The member
> > > > > > > name is just flags, plus an extra u32 for alignment.
> > > > > >
> > > > > > We can bikeshed the naming, but we've been using existing conventions
> > > > > > where opts fields match syscall fields, that seems like a sensible
> > > > > > thing to do?
> > > > >
> > > > > The only problem is that bpf_*_get_fd_by_id() functions would
> > > > > set the open_flags member of bpf_attr.
> > > > >
> > > > > Flags would be good for both, even if not exact. Believe me,
> > > > > duplicating the opts would just create more confusion.
> > > >
> > > > Wait, that's completely different, right? We are talking here about
> > > > BPF_OBJ_GET (which has related BPF_OBJ_PIN).
> > > > Your GET_XXX_BY_ID are different so you'll still have to have another
> > > > wrapper with opts?
> > >
> > > Yes, they have different wrappers, just accept the same opts as
> > > obj_get(). From bpftool subcommands you want to set the correct
> > > permission, and propagate it uniformly to bpf_*_get_fd_by_id()
> > > or obj_get(). See map_parse_fds().
> >
> > I don't think they are accepting the same opts.
> >
> > For our case, we care about:
> >
> > struct { /* anonymous struct used by BPF_OBJ_* commands */
> > __aligned_u64 pathname;
> > __u32 bpf_fd;
> > __u32 file_flags;
> > };
> >
> > For your case, you care about:
> >
> > struct { /* anonymous struct used by BPF_*_GET_*_ID */
> > union {
> > __u32 start_id;
> > __u32 prog_id;
> > __u32 map_id;
> > __u32 btf_id;
> > __u32 link_id;
> > };
> > __u32 next_id;
> > __u32 open_flags;
> > };
> >
> > So your new _opts libbpf routine should be independent of what Joe is
> > doing here.
>
> It is. Just I use the same opts to set file_flags or open_flags.
That seems confusing. Let's have separate calls for separate syscall
commands as we do already?
> Roberto
>
> > > Roberto
> > >
> > > > > > > It needs to be shared, as there are functions in bpftool calling
> > > > > > > both. Since the meaning of flags is the same, seems ok sharing.
> > > > > >
> > > > > > So I guess there are no objections to the current patch? If it gets
> > > > > > accepted, you should be able to drop some of your code and use this
> > > > > > new bpf_obj_get_opts..
> > > > >
> > > > > If you use a name good also for bpf_*_get_fd_by_id() and flags
> > > > > as structure member name, that would be ok.
> > > > >
> > > > > Roberto
> > > > >
> > > > > > > Roberto
> > > > > > >
> > > > > > > > > Roberto
> > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > tools/lib/bpf/bpf.c | 10 ++++++++++
> > > > > > > > > > > tools/lib/bpf/bpf.h | 9 +++++++++
> > > > > > > > > > > tools/lib/bpf/libbpf.map | 1 +
> > > > > > > > > > > 3 files changed, 20 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > 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);
> > > > > > > > > > > + 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;
> > > > > > > > > > > +};
> > > > > > > > > > > +#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
> > > > > > > > > > >
On Wed, Jul 20, 2022 at 4:17 PM Roberto Sassu <[email protected]> wrote:
>
> > From: Stanislav Fomichev [mailto:[email protected]]
> > Sent: Thursday, July 21, 2022 1:15 AM
> > On Wed, Jul 20, 2022 at 4:12 PM Roberto Sassu <[email protected]>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > Sent: Thursday, July 21, 2022 1:09 AM
> > > > On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu
> > <[email protected]>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > Sent: Thursday, July 21, 2022 12:48 AM
> > > > > > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu
> > > > <[email protected]>
> > > > > > wrote:
> > > > > > >
> > > > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > > > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > > > > > <[email protected]>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > > > > > <[email protected]>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > From: Stanislav Fomichev [mailto:[email protected]]
> > > > > > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > > > > > <[email protected]>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > From: Joe Burton <[email protected]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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 <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > Reviewed-by: Stanislav Fomichev <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > For context:
> > > > > > > > > > > > We've found this out while we were trying to add support for
> > > > unpriv
> > > > > > > > > > > > processes to open pinned r-x maps.
> > > > > > > > > > > > Maybe this deserves a test as well? Not sure.
> > > > > > > > > > >
> > > > > > > > > > > Hi Stanislav, Joe
> > > > > > > > > > >
> > > > > > > > > > > I noticed now this patch. I'm doing a broader work to add opts
> > > > > > > > > > > to bpf_*_get_fd_by_id(). I also adjusted permissions of bpftool
> > > > > > > > > > > depending on the operation type (e.g. show, dump:
> > > > BPF_F_RDONLY).
> > > > > > > > > > >
> > > > > > > > > > > Will send it soon (I'm trying to solve an issue with the CI, where
> > > > > > > > > > > libbfd is not available in the VM doing actual tests).
> > > > > > > > > >
> > > > > > > > > > Is something like this patch included in your series as well? Can
> > you
> > > > > > > > > > use this new interface or do you need something different?
> > > > > > > > >
> > > > > > > > > It is very similar. Except that I called it bpf_get_fd_opts, as it
> > > > > > > > > is shared with the bpf_*_get_fd_by_id() functions. The member
> > > > > > > > > name is just flags, plus an extra u32 for alignment.
> > > > > > > >
> > > > > > > > We can bikeshed the naming, but we've been using existing
> > conventions
> > > > > > > > where opts fields match syscall fields, that seems like a sensible
> > > > > > > > thing to do?
> > > > > > >
> > > > > > > The only problem is that bpf_*_get_fd_by_id() functions would
> > > > > > > set the open_flags member of bpf_attr.
> > > > > > >
> > > > > > > Flags would be good for both, even if not exact. Believe me,
> > > > > > > duplicating the opts would just create more confusion.
> > > > > >
> > > > > > Wait, that's completely different, right? We are talking here about
> > > > > > BPF_OBJ_GET (which has related BPF_OBJ_PIN).
> > > > > > Your GET_XXX_BY_ID are different so you'll still have to have another
> > > > > > wrapper with opts?
> > > > >
> > > > > Yes, they have different wrappers, just accept the same opts as
> > > > > obj_get(). From bpftool subcommands you want to set the correct
> > > > > permission, and propagate it uniformly to bpf_*_get_fd_by_id()
> > > > > or obj_get(). See map_parse_fds().
> > > >
> > > > I don't think they are accepting the same opts.
> > > >
> > > > For our case, we care about:
> > > >
> > > > struct { /* anonymous struct used by BPF_OBJ_* commands */
> > > > __aligned_u64 pathname;
> > > > __u32 bpf_fd;
> > > > __u32 file_flags;
> > > > };
> > > >
> > > > For your case, you care about:
> > > >
> > > > struct { /* anonymous struct used by BPF_*_GET_*_ID */
> > > > union {
> > > > __u32 start_id;
> > > > __u32 prog_id;
> > > > __u32 map_id;
> > > > __u32 btf_id;
> > > > __u32 link_id;
> > > > };
> > > > __u32 next_id;
> > > > __u32 open_flags;
> > > > };
> > > >
> > > > So your new _opts libbpf routine should be independent of what Joe is
> > > > doing here.
> > >
> > > It is. Just I use the same opts to set file_flags or open_flags.
> >
> > That seems confusing. Let's have separate calls for separate syscall
> > commands as we do already?
>
> Can you wait one day, I send what I have, so that we see
> everything together?
Sure, CC us both on the patches.
On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <[email protected]> wrote:
>
> From: Joe Burton <[email protected]>
>
> 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 <[email protected]>
> ---
> 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!
> 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
>
On Thu, Jul 28, 2022 at 12:58 AM Roberto Sassu <[email protected]> wrote:
>
> > From: Andrii Nakryiko [mailto:[email protected]]
> > Sent: Thursday, July 28, 2022 1:03 AM
> > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <[email protected]>
> > wrote:
> > >
> > > From: Joe Burton <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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?
it's because map_parse_fds() is used with both get_fd_by_id() and
bpf_obj_get(), right? I'd pass flags and construct correct set of
options internally, based on which BPF command you need to use to get
FD
>
> 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
> > >
On Wed, Jul 27, 2022 at 04:02:43PM -0700, Andrii Nakryiko wrote:
Applied both of these in v3, which I'll send out in a moment. Thanks for
the feedback!
> > 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.
> > +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)
>
TIL about this trick. Very clever.