* [PATCH bpf 0/2] Fix pinning devmaps
@ 2022-09-27 18:23 Pramukh Naduthota
2022-09-27 18:23 ` [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
2022-09-27 18:23 ` [PATCH bpf 2/2] Add selftests for devmap pinning Pramukh Naduthota
0 siblings, 2 replies; 8+ messages in thread
From: Pramukh Naduthota @ 2022-09-27 18:23 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, Pramukh Naduthota
Fix devmap pinning and reloading. The kernel adds BPF_F_RDONLY_PROG to all
devmaps when created, but libbpf checks that user flags match pinned map
flags when using LIBBPF_PIN_BY_NAME, so reusing pinned devmaps doesn't
work, failing with an error like:
libbpf: couldn't reuse pinned map at '/sys/fs/bpf/dev_map': parameter mismatch
Work around this by ignoring RDONLY_PROG in the compat check in libbpf.
Pramukh Naduthota (2):
Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned
devmaps
Add selftests for devmap pinning
tools/lib/bpf/libbpf.c | 8 +++++++-
.../testing/selftests/bpf/prog_tests/devmap.c | 21 +++++++++++++++++++
.../selftests/bpf/progs/test_pinned_devmap.c | 17 ++++++++++++++++
3 files changed, 45 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/devmap.c
create mode 100644 tools/testing/selftests/bpf/progs/test_pinned_devmap.c
--
2.30.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps
2022-09-27 18:23 [PATCH bpf 0/2] Fix pinning devmaps Pramukh Naduthota
@ 2022-09-27 18:23 ` Pramukh Naduthota
2022-09-28 0:39 ` Martin KaFai Lau
2022-09-27 18:23 ` [PATCH bpf 2/2] Add selftests for devmap pinning Pramukh Naduthota
1 sibling, 1 reply; 8+ messages in thread
From: Pramukh Naduthota @ 2022-09-27 18:23 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, Pramukh Naduthota
Ignore BPF_F_RDONLY_PROG when checking for compatibility for devmaps. The
kernel adds the flag to all devmap creates, and this breaks pinning
behavior, as libbpf will then check the actual vs user supplied flags and
see this difference. Work around this by adding RDONLY_PROG to the
users's flags when testing against the pinned map
Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
---
tools/lib/bpf/libbpf.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 50d41815f4..a3dae26d82 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4818,6 +4818,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
char msg[STRERR_BUFSIZE];
__u32 map_info_len;
int err;
+ unsigned int effective_flags = map->def.map_flags;
map_info_len = sizeof(map_info);
@@ -4830,11 +4831,16 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
return false;
}
+ /* The kernel adds RDONLY_PROG to devmaps */
+ if (map->def.type == BPF_MAP_TYPE_DEVMAP ||
+ map->def.type == BPF_MAP_TYPE_DEVMAP_HASH)
+ effective_flags |= BPF_F_RDONLY_PROG;
+
return (map_info.type == map->def.type &&
map_info.key_size == map->def.key_size &&
map_info.value_size == map->def.value_size &&
map_info.max_entries == map->def.max_entries &&
- map_info.map_flags == map->def.map_flags &&
+ map_info.map_flags == effective_flags &&
map_info.map_extra == map->map_extra);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf 2/2] Add selftests for devmap pinning
2022-09-27 18:23 [PATCH bpf 0/2] Fix pinning devmaps Pramukh Naduthota
2022-09-27 18:23 ` [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
@ 2022-09-27 18:23 ` Pramukh Naduthota
2022-09-27 20:30 ` Pramukh Naduthota
2022-09-30 21:05 ` Andrii Nakryiko
1 sibling, 2 replies; 8+ messages in thread
From: Pramukh Naduthota @ 2022-09-27 18:23 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, Pramukh Naduthota
Add a test for devmap pinning.
Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
---
.../testing/selftests/bpf/prog_tests/devmap.c | 21 +++++++++++++++++++
.../selftests/bpf/progs/test_pinned_devmap.c | 17 +++++++++++++++
2 files changed, 38 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/devmap.c
create mode 100644 tools/testing/selftests/bpf/progs/test_pinned_devmap.c
diff --git a/tools/testing/selftests/bpf/prog_tests/devmap.c b/tools/testing/selftests/bpf/prog_tests/devmap.c
new file mode 100644
index 000000000000..735333d3ac07
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/devmap.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2022 Google
+#include "testing_helpers.h"
+#include "test_progs.h"
+#include "test_pinned_devmap.skel.h"
+#include "test_pinned_devmap_rdonly_prog.skel.h"
+
+void test_devmap_pinning(void)
+{
+ struct test_pinned_devmap *ptr;
+
+ ASSERT_OK_PTR(ptr = test_pinned_devmap__open_and_load(), "first load");
+ test_pinned_devmap__destroy(ptr);
+ ASSERT_OK_PTR(test_pinned_devmap__open_and_load(), "re-load");
+}
+
+void test_devmap(void)
+{
+ if (test__start_subtest("pinned_devmap"))
+ test_devmap_pinning();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_pinned_devmap.c b/tools/testing/selftests/bpf/progs/test_pinned_devmap.c
new file mode 100644
index 000000000000..63879de18ad3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_pinned_devmap.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Google */
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <linux/types.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+ __uint(type, BPF_MAP_TYPE_DEVMAP_HASH);
+ __uint(max_entries, 32);
+ __type(key, int);
+ __type(value, int);
+ __uint(pinning, LIBBPF_PIN_BY_NAME);
+} dev_map SEC(".maps");
+
+
+char _license[] SEC("license") = "GPL";
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 2/2] Add selftests for devmap pinning
2022-09-27 18:23 ` [PATCH bpf 2/2] Add selftests for devmap pinning Pramukh Naduthota
@ 2022-09-27 20:30 ` Pramukh Naduthota
2022-09-30 21:05 ` Andrii Nakryiko
1 sibling, 0 replies; 8+ messages in thread
From: Pramukh Naduthota @ 2022-09-27 20:30 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, Stanislav Fomichev, haoluo, jolsa
> +#include "test_pinned_devmap_rdonly_prog.skel.h"
Oops, it looks like I forgot to clean up from an earlier idea. I'll send out
a v2 with that removed, but I'd still appreciated feedback on whether
this is the right fix for devmap pinning.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps
2022-09-27 18:23 ` [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
@ 2022-09-28 0:39 ` Martin KaFai Lau
2022-09-30 21:03 ` Andrii Nakryiko
0 siblings, 1 reply; 8+ messages in thread
From: Martin KaFai Lau @ 2022-09-28 0:39 UTC (permalink / raw)
To: Pramukh Naduthota
Cc: ast, daniel, andrii, song, yhs, john.fastabend, kpsingh, sdf,
haoluo, jolsa, bpf
On 9/27/22 11:23 AM, Pramukh Naduthota wrote:
> Ignore BPF_F_RDONLY_PROG when checking for compatibility for devmaps. The
> kernel adds the flag to all devmap creates, and this breaks pinning
> behavior, as libbpf will then check the actual vs user supplied flags and
> see this difference. Work around this by adding RDONLY_PROG to the
> users's flags when testing against the pinned map
>
> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
> ---
> tools/lib/bpf/libbpf.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 50d41815f4..a3dae26d82 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4818,6 +4818,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
> char msg[STRERR_BUFSIZE];
> __u32 map_info_len;
> int err;
> + unsigned int effective_flags = map->def.map_flags;
>
> map_info_len = sizeof(map_info);
>
> @@ -4830,11 +4831,16 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
> return false;
> }
>
> + /* The kernel adds RDONLY_PROG to devmaps */
> + if (map->def.type == BPF_MAP_TYPE_DEVMAP ||
> + map->def.type == BPF_MAP_TYPE_DEVMAP_HASH)
> + effective_flags |= BPF_F_RDONLY_PROG;
May be set BPF_F_RDONLY_PROG in effective_flags only when map->def.map_flags
does not have both BPF_F_RDONLY_PROG and BPF_F_WRONLY_PROG set? Just in case
the devmap may support setting them during map creation in the future.
> +
> return (map_info.type == map->def.type &&
> map_info.key_size == map->def.key_size &&
> map_info.value_size == map->def.value_size &&
> map_info.max_entries == map->def.max_entries &&
> - map_info.map_flags == map->def.map_flags &&
> + map_info.map_flags == effective_flags &&
> map_info.map_extra == map->map_extra);
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps
2022-09-28 0:39 ` Martin KaFai Lau
@ 2022-09-30 21:03 ` Andrii Nakryiko
2022-10-14 20:32 ` Pramukh Naduthota
0 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2022-09-30 21:03 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Pramukh Naduthota, ast, daniel, andrii, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf
On Tue, Sep 27, 2022 at 5:40 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 9/27/22 11:23 AM, Pramukh Naduthota wrote:
> > Ignore BPF_F_RDONLY_PROG when checking for compatibility for devmaps. The
> > kernel adds the flag to all devmap creates, and this breaks pinning
> > behavior, as libbpf will then check the actual vs user supplied flags and
> > see this difference. Work around this by adding RDONLY_PROG to the
> > users's flags when testing against the pinned map
> >
> > Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> > Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
> > ---
> > tools/lib/bpf/libbpf.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 50d41815f4..a3dae26d82 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4818,6 +4818,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
> > char msg[STRERR_BUFSIZE];
> > __u32 map_info_len;
> > int err;
> > + unsigned int effective_flags = map->def.map_flags;
> >
> > map_info_len = sizeof(map_info);
> >
> > @@ -4830,11 +4831,16 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
> > return false;
> > }
> >
> > + /* The kernel adds RDONLY_PROG to devmaps */
> > + if (map->def.type == BPF_MAP_TYPE_DEVMAP ||
> > + map->def.type == BPF_MAP_TYPE_DEVMAP_HASH)
> > + effective_flags |= BPF_F_RDONLY_PROG;
>
> May be set BPF_F_RDONLY_PROG in effective_flags only when map->def.map_flags
> does not have both BPF_F_RDONLY_PROG and BPF_F_WRONLY_PROG set? Just in case
> the devmap may support setting them during map creation in the future.
>
Would it be sane to just ignore
BPF_F_RDONLY|BPF_F_WRONLY|BPF_F_RDONLY_PROG|BPF_F_WRONLY_PROG flags
during this comparison? Those flags don't really change compatibility
of two maps in terms of their definition, right? Just that for some
combination of those flags BPF program load might fail (e.g., if it is
trying to modify BPF_F_RDONLY_PROG map). But that will be pretty
obvious from BPF the verifier log?
> > +
> > return (map_info.type == map->def.type &&
> > map_info.key_size == map->def.key_size &&
> > map_info.value_size == map->def.value_size &&
> > map_info.max_entries == map->def.max_entries &&
> > - map_info.map_flags == map->def.map_flags &&
> > + map_info.map_flags == effective_flags &&
> > map_info.map_extra == map->map_extra);
> > }
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 2/2] Add selftests for devmap pinning
2022-09-27 18:23 ` [PATCH bpf 2/2] Add selftests for devmap pinning Pramukh Naduthota
2022-09-27 20:30 ` Pramukh Naduthota
@ 2022-09-30 21:05 ` Andrii Nakryiko
1 sibling, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2022-09-30 21:05 UTC (permalink / raw)
To: Pramukh Naduthota
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
On Tue, Sep 27, 2022 at 11:24 AM Pramukh Naduthota
<pnaduthota@google.com> wrote:
>
> Add a test for devmap pinning.
>
> Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
> ---
> .../testing/selftests/bpf/prog_tests/devmap.c | 21 +++++++++++++++++++
> .../selftests/bpf/progs/test_pinned_devmap.c | 17 +++++++++++++++
> 2 files changed, 38 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/devmap.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_pinned_devmap.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/devmap.c b/tools/testing/selftests/bpf/prog_tests/devmap.c
> new file mode 100644
> index 000000000000..735333d3ac07
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/devmap.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2022 Google
nit: use /* */ for copyright line
> +#include "testing_helpers.h"
> +#include "test_progs.h"
> +#include "test_pinned_devmap.skel.h"
> +#include "test_pinned_devmap_rdonly_prog.skel.h"
> +
> +void test_devmap_pinning(void)
static
> +{
> + struct test_pinned_devmap *ptr;
> +
> + ASSERT_OK_PTR(ptr = test_pinned_devmap__open_and_load(), "first load");
nit: don't be too clever inside ASSERT_OK_PTR(), do assignment outside
and then check ptr
> + test_pinned_devmap__destroy(ptr);
> + ASSERT_OK_PTR(test_pinned_devmap__open_and_load(), "re-load");
> +}
> +
> +void test_devmap(void)
> +{
> + if (test__start_subtest("pinned_devmap"))
> + test_devmap_pinning();
if it's just one subtest then there isn't much point in making it a subtest
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_pinned_devmap.c b/tools/testing/selftests/bpf/progs/test_pinned_devmap.c
> new file mode 100644
> index 000000000000..63879de18ad3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_pinned_devmap.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Google */
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <linux/types.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_DEVMAP_HASH);
> + __uint(max_entries, 32);
> + __type(key, int);
> + __type(value, int);
> + __uint(pinning, LIBBPF_PIN_BY_NAME);
> +} dev_map SEC(".maps");
please use a bit more specific name to minimize potential interference
with other parallel tests
> +
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps
2022-09-30 21:03 ` Andrii Nakryiko
@ 2022-10-14 20:32 ` Pramukh Naduthota
0 siblings, 0 replies; 8+ messages in thread
From: Pramukh Naduthota @ 2022-10-14 20:32 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Martin KaFai Lau, ast, daniel, andrii, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, bpf
Do you have any more thoughts on this?
For my 2c:
- It looks like the kernel is making the map BPF_F_RDONLY_PROG because
"Lookup returns a pointer straight to dev->ifindex", so I don't know
if we have to
worry about the kernel allowing write access to devmaps
- I like having the guardrails in libbpf, but I'm not sure what the
exact contract
is for what libbpf will let me reload
I'll spin out a V2 similar to what I had if nobody has any strong opinions on
the exact behavior of this check
- Pramukh
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-14 20:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 18:23 [PATCH bpf 0/2] Fix pinning devmaps Pramukh Naduthota
2022-09-27 18:23 ` [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
2022-09-28 0:39 ` Martin KaFai Lau
2022-09-30 21:03 ` Andrii Nakryiko
2022-10-14 20:32 ` Pramukh Naduthota
2022-09-27 18:23 ` [PATCH bpf 2/2] Add selftests for devmap pinning Pramukh Naduthota
2022-09-27 20:30 ` Pramukh Naduthota
2022-09-30 21:05 ` Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.