[PATCH bpf 0/2] Fix pinning devmaps

[PATCH bpf 0/2] Fix pinning devmaps

[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

[PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps

[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

Re: [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps

[Martin KaFai Lau]

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);
>   }
>   

Re: [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps

[Andrii Nakryiko]

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);
> >   }
> >
>

Re: [PATCH bpf 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps

[Pramukh Naduthota]

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

[PATCH bpf 2/2] Add selftests for devmap pinning

[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

Re: [PATCH bpf 2/2] Add selftests for devmap pinning

[Pramukh Naduthota]

> +#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.

Re: [PATCH bpf 2/2] Add selftests for devmap pinning

[Andrii Nakryiko]

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
>