* [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
@ 2022-07-19 19:40 Joe Burton
2022-07-19 20:40 ` Stanislav Fomichev
2022-07-27 23:02 ` Andrii Nakryiko
0 siblings, 2 replies; 18+ messages in thread
From: Joe Burton @ 2022-07-19 19:40 UTC (permalink / raw)
To: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel
Cc: Joe Burton
From: Joe Burton <jevburton@google.com>
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@google.com>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-19 19:40 [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts() Joe Burton
@ 2022-07-19 20:40 ` Stanislav Fomichev
2022-07-20 8:02 ` Roberto Sassu
2022-07-27 23:02 ` Andrii Nakryiko
1 sibling, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-19 20:40 UTC (permalink / raw)
To: Joe Burton
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com> wrote:
>
> From: Joe Burton <jevburton@google.com>
>
> 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@google.com>
Reviewed-by: Stanislav Fomichev <sdf@google.com>
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-19 20:40 ` Stanislav Fomichev
@ 2022-07-20 8:02 ` Roberto Sassu
2022-07-20 15:56 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-20 8:02 UTC (permalink / raw)
To: Stanislav Fomichev, Joe Burton
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
> From: Stanislav Fomichev [mailto:sdf@google.com]
> Sent: Tuesday, July 19, 2022 10:40 PM
> On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com>
> wrote:
> >
> > From: Joe Burton <jevburton@google.com>
> >
> > 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@google.com>
>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
>
> 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).
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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 8:02 ` Roberto Sassu
@ 2022-07-20 15:56 ` Stanislav Fomichev
2022-07-20 22:30 ` Roberto Sassu
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-20 15:56 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Stanislav Fomichev [mailto:sdf@google.com]
> > Sent: Tuesday, July 19, 2022 10:40 PM
> > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com>
> > wrote:
> > >
> > > From: Joe Burton <jevburton@google.com>
> > >
> > > 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@google.com>
> >
> > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> >
> > 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
> > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 15:56 ` Stanislav Fomichev
@ 2022-07-20 22:30 ` Roberto Sassu
2022-07-20 22:38 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-20 22:30 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
> From: Stanislav Fomichev [mailto:sdf@google.com]
> Sent: Wednesday, July 20, 2022 5:57 PM
> On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com>
> > > wrote:
> > > >
> > > > From: Joe Burton <jevburton@google.com>
> > > >
> > > > 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@google.com>
> > >
> > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > >
> > > 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.
It needs to be shared, as there are functions in bpftool calling
both. Since the meaning of flags is the same, seems ok sharing.
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
> > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 22:30 ` Roberto Sassu
@ 2022-07-20 22:38 ` Stanislav Fomichev
2022-07-20 22:44 ` Roberto Sassu
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-20 22:38 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Stanislav Fomichev [mailto:sdf@google.com]
> > Sent: Wednesday, July 20, 2022 5:57 PM
> > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com>
> > > > wrote:
> > > > >
> > > > > From: Joe Burton <jevburton@google.com>
> > > > >
> > > > > 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@google.com>
> > > >
> > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > >
> > > > 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
> > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 22:38 ` Stanislav Fomichev
@ 2022-07-20 22:44 ` Roberto Sassu
2022-07-20 22:48 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-20 22:44 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
> From: Stanislav Fomichev [mailto:sdf@google.com]
> Sent: Thursday, July 21, 2022 12:38 AM
> On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> <roberto.sassu@huawei.com>
> > > wrote:
> > > >
> > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> <jevburton.kernel@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > From: Joe Burton <jevburton@google.com>
> > > > > >
> > > > > > 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@google.com>
> > > > >
> > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > >
> > > > > 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.
> > 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
> > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 22:44 ` Roberto Sassu
@ 2022-07-20 22:48 ` Stanislav Fomichev
2022-07-20 23:02 ` Roberto Sassu
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-20 22:48 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Stanislav Fomichev [mailto:sdf@google.com]
> > Sent: Thursday, July 21, 2022 12:38 AM
> > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > <roberto.sassu@huawei.com>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > <jevburton.kernel@gmail.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > >
> > > > > > > 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@google.com>
> > > > > >
> > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > >
> > > > > > 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
> > > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 22:48 ` Stanislav Fomichev
@ 2022-07-20 23:02 ` Roberto Sassu
2022-07-20 23:08 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-20 23:02 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
> From: Stanislav Fomichev [mailto:sdf@google.com]
> Sent: Thursday, July 21, 2022 12:48 AM
> On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > Sent: Thursday, July 21, 2022 12:38 AM
> > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> <roberto.sassu@huawei.com>
> > > wrote:
> > > >
> > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > <roberto.sassu@huawei.com>
> > > > > wrote:
> > > > > >
> > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > <jevburton.kernel@gmail.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > > >
> > > > > > > > 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@google.com>
> > > > > > >
> > > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > > >
> > > > > > > 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().
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
> > > > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 23:02 ` Roberto Sassu
@ 2022-07-20 23:08 ` Stanislav Fomichev
2022-07-20 23:12 ` Roberto Sassu
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-20 23:08 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Stanislav Fomichev [mailto:sdf@google.com]
> > Sent: Thursday, July 21, 2022 12:48 AM
> > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > <roberto.sassu@huawei.com>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > <roberto.sassu@huawei.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > <jevburton.kernel@gmail.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > > > >
> > > > > > > > > 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@google.com>
> > > > > > > >
> > > > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > > > >
> > > > > > > > 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
> > > > > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 23:08 ` Stanislav Fomichev
@ 2022-07-20 23:12 ` Roberto Sassu
2022-07-20 23:14 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-20 23:12 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
> From: Stanislav Fomichev [mailto:sdf@google.com]
> Sent: Thursday, July 21, 2022 1:09 AM
> On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > Sent: Thursday, July 21, 2022 12:48 AM
> > > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu
> <roberto.sassu@huawei.com>
> > > wrote:
> > > >
> > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > > <roberto.sassu@huawei.com>
> > > > > wrote:
> > > > > >
> > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > > <roberto.sassu@huawei.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > > <jevburton.kernel@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > > > > >
> > > > > > > > > > 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@google.com>
> > > > > > > > >
> > > > > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > > > > >
> > > > > > > > > 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.
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
> > > > > > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 23:12 ` Roberto Sassu
@ 2022-07-20 23:14 ` Stanislav Fomichev
2022-07-20 23:17 ` Roberto Sassu
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-20 23:14 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Wed, Jul 20, 2022 at 4:12 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Stanislav Fomichev [mailto:sdf@google.com]
> > Sent: Thursday, July 21, 2022 1:09 AM
> > On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > Sent: Thursday, July 21, 2022 12:48 AM
> > > > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu
> > <roberto.sassu@huawei.com>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > > > <roberto.sassu@huawei.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > > > <roberto.sassu@huawei.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > > > <jevburton.kernel@gmail.com>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > > > > > >
> > > > > > > > > > > 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@google.com>
> > > > > > > > > >
> > > > > > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 23:14 ` Stanislav Fomichev
@ 2022-07-20 23:17 ` Roberto Sassu
2022-07-20 23:26 ` Stanislav Fomichev
0 siblings, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-20 23:17 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
> From: Stanislav Fomichev [mailto:sdf@google.com]
> Sent: Thursday, July 21, 2022 1:15 AM
> On Wed, Jul 20, 2022 at 4:12 PM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > Sent: Thursday, July 21, 2022 1:09 AM
> > > On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu
> <roberto.sassu@huawei.com>
> > > wrote:
> > > >
> > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > Sent: Thursday, July 21, 2022 12:48 AM
> > > > > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu
> > > <roberto.sassu@huawei.com>
> > > > > wrote:
> > > > > >
> > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > > > > <roberto.sassu@huawei.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > > > > <roberto.sassu@huawei.com>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > > > > <jevburton.kernel@gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > > > > > > >
> > > > > > > > > > > > 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@google.com>
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > > > > > > >
> > > > > > > > > > > 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?
Thanks
Roberto
> > 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
> > > > > > > > > > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-20 23:17 ` Roberto Sassu
@ 2022-07-20 23:26 ` Stanislav Fomichev
0 siblings, 0 replies; 18+ messages in thread
From: Stanislav Fomichev @ 2022-07-20 23:26 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, Joe Burton
On Wed, Jul 20, 2022 at 4:17 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Stanislav Fomichev [mailto:sdf@google.com]
> > Sent: Thursday, July 21, 2022 1:15 AM
> > On Wed, Jul 20, 2022 at 4:12 PM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > Sent: Thursday, July 21, 2022 1:09 AM
> > > > On Wed, Jul 20, 2022 at 4:02 PM Roberto Sassu
> > <roberto.sassu@huawei.com>
> > > > wrote:
> > > > >
> > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > Sent: Thursday, July 21, 2022 12:48 AM
> > > > > > On Wed, Jul 20, 2022 at 3:44 PM Roberto Sassu
> > > > <roberto.sassu@huawei.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > Sent: Thursday, July 21, 2022 12:38 AM
> > > > > > > > On Wed, Jul 20, 2022 at 3:30 PM Roberto Sassu
> > > > > > <roberto.sassu@huawei.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > > > Sent: Wednesday, July 20, 2022 5:57 PM
> > > > > > > > > > On Wed, Jul 20, 2022 at 1:02 AM Roberto Sassu
> > > > > > > > <roberto.sassu@huawei.com>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > From: Stanislav Fomichev [mailto:sdf@google.com]
> > > > > > > > > > > > Sent: Tuesday, July 19, 2022 10:40 PM
> > > > > > > > > > > > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton
> > > > > > > > <jevburton.kernel@gmail.com>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > From: Joe Burton <jevburton@google.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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@google.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > > > > > > > > > > >
> > > > > > > > > > > > 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-19 19:40 [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts() Joe Burton
2022-07-19 20:40 ` Stanislav Fomichev
@ 2022-07-27 23:02 ` Andrii Nakryiko
2022-07-28 7:58 ` Roberto Sassu
2022-07-29 20:20 ` Joe Burton
1 sibling, 2 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-27 23:02 UTC (permalink / raw)
To: Joe Burton
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, open list,
Joe Burton
On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com> wrote:
>
> From: Joe Burton <jevburton@google.com>
>
> 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@google.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-27 23:02 ` Andrii Nakryiko
@ 2022-07-28 7:58 ` Roberto Sassu
2022-07-29 18:55 ` Andrii Nakryiko
2022-07-29 20:20 ` Joe Burton
1 sibling, 1 reply; 18+ messages in thread
From: Roberto Sassu @ 2022-07-28 7:58 UTC (permalink / raw)
To: Andrii Nakryiko, Joe Burton
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, open list,
Joe Burton
> From: Andrii Nakryiko [mailto:andrii.nakryiko@gmail.com]
> Sent: Thursday, July 28, 2022 1:03 AM
> On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com>
> wrote:
> >
> > From: Joe Burton <jevburton@google.com>
> >
> > 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@google.com>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-28 7:58 ` Roberto Sassu
@ 2022-07-29 18:55 ` Andrii Nakryiko
0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-07-29 18:55 UTC (permalink / raw)
To: Roberto Sassu
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, open list,
Joe Burton
On Thu, Jul 28, 2022 at 12:58 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Andrii Nakryiko [mailto:andrii.nakryiko@gmail.com]
> > Sent: Thursday, July 28, 2022 1:03 AM
> > On Tue, Jul 19, 2022 at 12:40 PM Joe Burton <jevburton.kernel@gmail.com>
> > wrote:
> > >
> > > From: Joe Burton <jevburton@google.com>
> > >
> > > 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@google.com>
> > > ---
> > > 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
> > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts()
2022-07-27 23:02 ` Andrii Nakryiko
2022-07-28 7:58 ` Roberto Sassu
@ 2022-07-29 20:20 ` Joe Burton
1 sibling, 0 replies; 18+ messages in thread
From: Joe Burton @ 2022-07-29 20:20 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Joe Burton, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, open list
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-07-29 20:20 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 19:40 [PATCH v2 bpf-next] libbpf: Add bpf_obj_get_opts() Joe Burton
2022-07-19 20:40 ` Stanislav Fomichev
2022-07-20 8:02 ` Roberto Sassu
2022-07-20 15:56 ` Stanislav Fomichev
2022-07-20 22:30 ` Roberto Sassu
2022-07-20 22:38 ` Stanislav Fomichev
2022-07-20 22:44 ` Roberto Sassu
2022-07-20 22:48 ` Stanislav Fomichev
2022-07-20 23:02 ` Roberto Sassu
2022-07-20 23:08 ` Stanislav Fomichev
2022-07-20 23:12 ` Roberto Sassu
2022-07-20 23:14 ` Stanislav Fomichev
2022-07-20 23:17 ` Roberto Sassu
2022-07-20 23:26 ` Stanislav Fomichev
2022-07-27 23:02 ` Andrii Nakryiko
2022-07-28 7:58 ` Roberto Sassu
2022-07-29 18:55 ` Andrii Nakryiko
2022-07-29 20:20 ` Joe Burton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).