All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/2] Fix pinning devmaps
@ 2022-12-01  1:11 Pramukh Naduthota
  2022-12-01  1:11 ` [PATCH net-next v2 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
  2022-12-01  1:11 ` [PATCH net-next v2 2/2] Add a selftest for devmap pinning Pramukh Naduthota
  0 siblings, 2 replies; 6+ messages in thread
From: Pramukh Naduthota @ 2022-12-01  1:11 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.

Changes since v1:
- Fixed a broken import
- Fixed style issues


Pramukh Naduthota (2):
  Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned
    devmaps
  Add a selftest for devmap pinning.

 tools/lib/bpf/libbpf.c                        |  8 +++++++-
 .../testing/selftests/bpf/prog_tests/devmap.c | 20 +++++++++++++++++++
 .../selftests/bpf/progs/test_pinned_devmap.c  | 17 ++++++++++++++++
 3 files changed, 44 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] 6+ messages in thread
* [PATCH net-next v2 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps
  2022-12-01  1:11 [PATCH net-next v2 0/2] Fix pinning devmaps Pramukh Naduthota
@ 2022-12-01  1:11 ` Pramukh Naduthota
  2022-12-01  1:11 ` [PATCH net-next v2 2/2] Add a selftest for devmap pinning Pramukh Naduthota
  1 sibling, 0 replies; 6+ messages in thread
From: Pramukh Naduthota @ 2022-12-01  1:11 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 50d41815f431..a3dae26d82d6 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] 6+ messages in thread
* [PATCH net-next v2 2/2] Add a selftest for devmap pinning.
  2022-12-01  1:11 [PATCH net-next v2 0/2] Fix pinning devmaps Pramukh Naduthota
  2022-12-01  1:11 ` [PATCH net-next v2 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
@ 2022-12-01  1:11 ` Pramukh Naduthota
  2022-12-05 22:31   ` Daniel Borkmann
  1 sibling, 1 reply; 6+ messages in thread
From: Pramukh Naduthota @ 2022-12-01  1:11 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, Pramukh Naduthota

Add a selftest

Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
---
 .../testing/selftests/bpf/prog_tests/devmap.c | 20 +++++++++++++++++++
 .../selftests/bpf/progs/test_pinned_devmap.c  | 17 ++++++++++++++++
 2 files changed, 37 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..50c5006c1416
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/devmap.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Google */
+#include "testing_helpers.h"
+#include "test_progs.h"
+#include "test_pinned_devmap.skel.h"
+
+void test_devmap_pinning(void)
+{
+	struct test_pinned_devmap *ptr;
+
+	ptr = test_pinned_devmap__open_and_load()
+	ASSERT_OK_PTR(ptr, "first load");
+	test_pinned_devmap__destroy(ptr);
+	ASSERT_OK_PTR(test_pinned_devmap__open_and_load(), "re-load");
+}
+
+void test_devmap(void)
+{
+	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..2e9b25fe657c
--- /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);
+} repinned_dev_map SEC(".maps");
+
+
+char _license[] SEC("license") = "GPL";
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] Add a selftest for devmap pinning.
  2022-12-01  1:11 ` [PATCH net-next v2 2/2] Add a selftest for devmap pinning Pramukh Naduthota
@ 2022-12-05 22:31   ` Daniel Borkmann
  2022-12-07  0:42     ` Pramukh Naduthota
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2022-12-05 22:31 UTC (permalink / raw)
  To: Pramukh Naduthota, bpf
  Cc: ast, andrii, martin.lau, song, yhs, john.fastabend, kpsingh, sdf,
	haoluo, jolsa

On 12/1/22 2:11 AM, Pramukh Naduthota wrote:
> Add a selftest
> 
> Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
> ---
>   .../testing/selftests/bpf/prog_tests/devmap.c | 20 +++++++++++++++++++
>   .../selftests/bpf/progs/test_pinned_devmap.c  | 17 ++++++++++++++++
>   2 files changed, 37 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..50c5006c1416
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/devmap.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Google */
> +#include "testing_helpers.h"
> +#include "test_progs.h"
> +#include "test_pinned_devmap.skel.h"
> +
> +void test_devmap_pinning(void)
> +{
> +	struct test_pinned_devmap *ptr;
> +
> +	ptr = test_pinned_devmap__open_and_load()
> +	ASSERT_OK_PTR(ptr, "first load");

Looks like you never actually compiled your selftest? :(

     [...]
     TEST-OBJ [test_progs] rcu_read_lock.test.o
     TEST-OBJ [test_progs] btf_dump.test.o
   In file included from /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c:4:
   /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c: In function ‘test_devmap_pinning’:
   ./test_progs.h:352:35: error: expected expression before ‘{’ token
     352 | #define ASSERT_OK_PTR(ptr, name) ({     \
         |                                   ^
   /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c:12:2: note: in expansion of macro ‘ASSERT_OK_PTR’
      12 |  ASSERT_OK_PTR(ptr, "first load");
         |  ^~~~~~~~~~~~~
   /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c:11:8: error: called object is not a function or function pointer
      11 |  ptr = test_pinned_devmap__open_and_load()
         |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   make: *** [Makefile:539: /tmp/work/bpf/bpf/tools/testing/selftests/bpf/devmap.test.o] Error 1
   make: *** Waiting for unfinished jobs....
   make: Leaving directory '/tmp/work/bpf/bpf/tools/testing/selftests/bpf'
   Error: Process completed with exit code 2.

> +	test_pinned_devmap__destroy(ptr);
> +	ASSERT_OK_PTR(test_pinned_devmap__open_and_load(), "re-load");
> +}
> +
> +void test_devmap(void)
> +{
> +	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..2e9b25fe657c
> --- /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);
> +} repinned_dev_map SEC(".maps");
> +
> +
> +char _license[] SEC("license") = "GPL";
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] Add a selftest for devmap pinning.
  2022-12-05 22:31   ` Daniel Borkmann
@ 2022-12-07  0:42     ` Pramukh Naduthota
  2022-12-07  6:43       ` John Fastabend
  0 siblings, 1 reply; 6+ messages in thread
From: Pramukh Naduthota @ 2022-12-07  0:42 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: bpf, ast, andrii, martin.lau, song, yhs, john.fastabend, kpsingh,
	sdf, haoluo, jolsa

Sorry, looks like I didn't run the tests again after fixing my
checkpatch errors. Still new to this, and am quite mortified.

Is there a better way to fix this than sending out a v3 of my patch?

On Mon, Dec 5, 2022 at 2:31 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 12/1/22 2:11 AM, Pramukh Naduthota wrote:
> > Add a selftest
> >
> > Signed-off-by: Pramukh Naduthota <pnaduthota@google.com>
> > ---
> >   .../testing/selftests/bpf/prog_tests/devmap.c | 20 +++++++++++++++++++
> >   .../selftests/bpf/progs/test_pinned_devmap.c  | 17 ++++++++++++++++
> >   2 files changed, 37 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..50c5006c1416
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/devmap.c
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2022 Google */
> > +#include "testing_helpers.h"
> > +#include "test_progs.h"
> > +#include "test_pinned_devmap.skel.h"
> > +
> > +void test_devmap_pinning(void)
> > +{
> > +     struct test_pinned_devmap *ptr;
> > +
> > +     ptr = test_pinned_devmap__open_and_load()
> > +     ASSERT_OK_PTR(ptr, "first load");
>
> Looks like you never actually compiled your selftest? :(
>
>      [...]
>      TEST-OBJ [test_progs] rcu_read_lock.test.o
>      TEST-OBJ [test_progs] btf_dump.test.o
>    In file included from /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c:4:
>    /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c: In function ‘test_devmap_pinning’:
>    ./test_progs.h:352:35: error: expected expression before ‘{’ token
>      352 | #define ASSERT_OK_PTR(ptr, name) ({     \
>          |                                   ^
>    /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c:12:2: note: in expansion of macro ‘ASSERT_OK_PTR’
>       12 |  ASSERT_OK_PTR(ptr, "first load");
>          |  ^~~~~~~~~~~~~
>    /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/devmap.c:11:8: error: called object is not a function or function pointer
>       11 |  ptr = test_pinned_devmap__open_and_load()
>          |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    make: *** [Makefile:539: /tmp/work/bpf/bpf/tools/testing/selftests/bpf/devmap.test.o] Error 1
>    make: *** Waiting for unfinished jobs....
>    make: Leaving directory '/tmp/work/bpf/bpf/tools/testing/selftests/bpf'
>    Error: Process completed with exit code 2.
>
> > +     test_pinned_devmap__destroy(ptr);
> > +     ASSERT_OK_PTR(test_pinned_devmap__open_and_load(), "re-load");
> > +}
> > +
> > +void test_devmap(void)
> > +{
> > +     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..2e9b25fe657c
> > --- /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);
> > +} repinned_dev_map SEC(".maps");
> > +
> > +
> > +char _license[] SEC("license") = "GPL";
> >
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] Add a selftest for devmap pinning.
  2022-12-07  0:42     ` Pramukh Naduthota
@ 2022-12-07  6:43       ` John Fastabend
  0 siblings, 0 replies; 6+ messages in thread
From: John Fastabend @ 2022-12-07  6:43 UTC (permalink / raw)
  To: Pramukh Naduthota, Daniel Borkmann
  Cc: bpf, ast, andrii, martin.lau, song, yhs, john.fastabend, kpsingh,
	sdf, haoluo, jolsa

Pramukh Naduthota wrote:
> Sorry, looks like I didn't run the tests again after fixing my
> checkpatch errors. Still new to this, and am quite mortified.
> 
> Is there a better way to fix this than sending out a v3 of my patch?

You'll need to send a v3 and be sure to build/run the selftests. Also for
future reference please don't top post it. And thanks for working
on the fix.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-07  6:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-01  1:11 [PATCH net-next v2 0/2] Fix pinning devmaps Pramukh Naduthota
2022-12-01  1:11 ` [PATCH net-next v2 1/2] Ignore RDONLY_PROG for devmaps in libbpf to allow re-loading of pinned devmaps Pramukh Naduthota
2022-12-01  1:11 ` [PATCH net-next v2 2/2] Add a selftest for devmap pinning Pramukh Naduthota
2022-12-05 22:31   ` Daniel Borkmann
2022-12-07  0:42     ` Pramukh Naduthota
2022-12-07  6:43       ` John Fastabend

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.