[PATCH bpf-next v2 0/4] Sockmap copying

[PATCH bpf-next v2 0/4] Sockmap copying

[Lorenz Bauer]

Changes in v2:
- Check sk_fullsock in map_update_elem (Martin)

Enable calling map_update_elem on sockmaps from bpf_iter context. This
in turn allows us to copy a sockmap by iterating its elements.

The change itself is tiny, all thanks to the ground work from Martin,
whose series [1] this patch is based on. I updated the tests to do some
copying, and also included two cleanups.

I'm sending this out now rather than when Martin's series has landed
because I hope this can get in before the merge window (potentially)
closes this weekend.

1: https://lore.kernel.org/bpf/20200925000337.3853598-1-kafai@fb.com/

Lorenz Bauer (4):
  bpf: sockmap: enable map_update_elem from bpf_iter
  selftests: bpf: Add helper to compare socket cookies
  selftests: bpf: remove shared header from sockmap iter test
  selftest: bpf: Test copying a sockmap and sockhash

 kernel/bpf/verifier.c                         |   2 +-
 net/core/sock_map.c                           |   3 +
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 100 +++++++++++-------
 .../selftests/bpf/progs/bpf_iter_sockmap.c    |  32 ++++--
 .../selftests/bpf/progs/bpf_iter_sockmap.h    |   3 -
 5 files changed, 90 insertions(+), 50 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h

-- 
2.25.1

[PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter

[Lorenz Bauer]

Allow passing a pointer to a BTF struct sock_common* when updating
a sockmap or sockhash. Since BTF pointers can fault and therefore be
NULL at runtime we need to add an additional !sk check to
sock_map_update_elem. Since we may be passed a request or timewait
socket we also need to check sk_fullsock. Doing this allows calling
map_update_elem on sockmap from bpf_iter context, which uses
BTF pointers.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 kernel/bpf/verifier.c | 2 +-
 net/core/sock_map.c   | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b25ba989c2dc..cc9c90d74dc1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3943,7 +3943,7 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env,
 	case BPF_MAP_TYPE_SOCKMAP:
 	case BPF_MAP_TYPE_SOCKHASH:
 		if (*arg_type == ARG_PTR_TO_MAP_VALUE) {
-			*arg_type = ARG_PTR_TO_SOCKET;
+			*arg_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON;
 		} else {
 			verbose(env, "invalid arg_type for sockmap/sockhash\n");
 			return -EINVAL;
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index e1f05e3fa1d0..08bc86f51593 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -610,6 +610,9 @@ static int sock_map_update_elem(struct bpf_map *map, void *key,
 	struct sock *sk = (struct sock *)value;
 	int ret;
 
+	if (unlikely(!sk || !sk_fullsock(sk)))
+		return -EINVAL;
+
 	if (!sock_map_sk_is_suitable(sk))
 		return -EOPNOTSUPP;
 
-- 
2.25.1

[PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Lorenz Bauer]

We compare socket cookies to ensure that insertion into a sockmap worked.
Pull this out into a helper function for use in other tests.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 50 +++++++++++++------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 4b7a527e7e82..67d3301bdf81 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -50,6 +50,37 @@ static int connected_socket_v4(void)
 	return -1;
 }
 
+static void compare_cookies(struct bpf_map *src, struct bpf_map *dst)
+{
+	__u32 i, max_entries = bpf_map__max_entries(src);
+	int err, duration, src_fd, dst_fd;
+
+	src_fd = bpf_map__fd(src);
+	dst_fd = bpf_map__fd(dst);
+
+	for (i = 0; i < max_entries; i++) {
+		__u64 src_cookie, dst_cookie;
+
+		err = bpf_map_lookup_elem(src_fd, &i, &src_cookie);
+		if (err && errno == ENOENT) {
+			err = bpf_map_lookup_elem(dst_fd, &i, &dst_cookie);
+			CHECK(!err, "map_lookup_elem(dst)", "element %u not deleted\n", i);
+			CHECK(err && errno != ENOENT, "map_lookup_elem(dst)", "%s\n",
+			      strerror(errno));
+			continue;
+		}
+		if (CHECK(err, "lookup_elem(src)", "%s\n", strerror(errno)))
+			continue;
+
+		err = bpf_map_lookup_elem(dst_fd, &i, &dst_cookie);
+		if (CHECK(err, "lookup_elem(dst)", "%s\n", strerror(errno)))
+			continue;
+
+		CHECK(dst_cookie != src_cookie, "cookie mismatch",
+		      "%llu != %llu (pos %u)\n", dst_cookie, src_cookie, i);
+	}
+}
+
 /* Create a map, populate it with one socket, and free the map. */
 static void test_sockmap_create_update_free(enum bpf_map_type map_type)
 {
@@ -109,9 +140,9 @@ static void test_skmsg_helpers(enum bpf_map_type map_type)
 static void test_sockmap_update(enum bpf_map_type map_type)
 {
 	struct bpf_prog_test_run_attr tattr;
-	int err, prog, src, dst, duration = 0;
+	int err, prog, src, duration = 0;
 	struct test_sockmap_update *skel;
-	__u64 src_cookie, dst_cookie;
+	struct bpf_map *dst_map;
 	const __u32 zero = 0;
 	char dummy[14] = {0};
 	__s64 sk;
@@ -127,18 +158,14 @@ static void test_sockmap_update(enum bpf_map_type map_type)
 	prog = bpf_program__fd(skel->progs.copy_sock_map);
 	src = bpf_map__fd(skel->maps.src);
 	if (map_type == BPF_MAP_TYPE_SOCKMAP)
-		dst = bpf_map__fd(skel->maps.dst_sock_map);
+		dst_map = skel->maps.dst_sock_map;
 	else
-		dst = bpf_map__fd(skel->maps.dst_sock_hash);
+		dst_map = skel->maps.dst_sock_hash;
 
 	err = bpf_map_update_elem(src, &zero, &sk, BPF_NOEXIST);
 	if (CHECK(err, "update_elem(src)", "errno=%u\n", errno))
 		goto out;
 
-	err = bpf_map_lookup_elem(src, &zero, &src_cookie);
-	if (CHECK(err, "lookup_elem(src, cookie)", "errno=%u\n", errno))
-		goto out;
-
 	tattr = (struct bpf_prog_test_run_attr){
 		.prog_fd = prog,
 		.repeat = 1,
@@ -151,12 +178,7 @@ static void test_sockmap_update(enum bpf_map_type map_type)
 		       "errno=%u retval=%u\n", errno, tattr.retval))
 		goto out;
 
-	err = bpf_map_lookup_elem(dst, &zero, &dst_cookie);
-	if (CHECK(err, "lookup_elem(dst, cookie)", "errno=%u\n", errno))
-		goto out;
-
-	CHECK(dst_cookie != src_cookie, "cookie mismatch", "%llu != %llu\n",
-	      dst_cookie, src_cookie);
+	compare_cookies(skel->maps.src, dst_map);
 
 out:
 	test_sockmap_update__destroy(skel);
-- 
2.25.1

[PATCH bpf-next v2 3/4] selftests: bpf: remove shared header from sockmap iter test

[Lorenz Bauer]

The shared header to define SOCKMAP_MAX_ENTRIES is a bit overkill.
Dynamically allocate the sock_fd array based on bpf_map__max_entries
instead.

Suggested-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 36 +++++++++----------
 .../selftests/bpf/progs/bpf_iter_sockmap.c    |  5 ++-
 .../selftests/bpf/progs/bpf_iter_sockmap.h    |  3 --
 3 files changed, 20 insertions(+), 24 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 67d3301bdf81..e8a4bfb4d9f4 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -8,8 +8,6 @@
 #include "test_sockmap_invalid_update.skel.h"
 #include "bpf_iter_sockmap.skel.h"
 
-#include "progs/bpf_iter_sockmap.h"
-
 #define TCP_REPAIR		19	/* TCP sock is under repair right now */
 
 #define TCP_REPAIR_ON		1
@@ -201,9 +199,9 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
 	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
 	int err, len, src_fd, iter_fd, duration = 0;
 	union bpf_iter_link_info linfo = {0};
-	__s64 sock_fd[SOCKMAP_MAX_ENTRIES];
-	__u32 i, num_sockets, max_elems;
+	__u32 i, num_sockets, num_elems;
 	struct bpf_iter_sockmap *skel;
+	__s64 *sock_fd = NULL;
 	struct bpf_link *link;
 	struct bpf_map *src;
 	char buf[64];
@@ -212,22 +210,23 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
 	if (CHECK(!skel, "bpf_iter_sockmap__open_and_load", "skeleton open_and_load failed\n"))
 		return;
 
-	for (i = 0; i < ARRAY_SIZE(sock_fd); i++)
-		sock_fd[i] = -1;
-
-	/* Make sure we have at least one "empty" entry to test iteration of
-	 * an empty slot.
-	 */
-	num_sockets = ARRAY_SIZE(sock_fd) - 1;
-
 	if (map_type == BPF_MAP_TYPE_SOCKMAP) {
 		src = skel->maps.sockmap;
-		max_elems = bpf_map__max_entries(src);
+		num_elems = bpf_map__max_entries(src);
+		num_sockets = num_elems - 1;
 	} else {
 		src = skel->maps.sockhash;
-		max_elems = num_sockets;
+		num_elems = bpf_map__max_entries(src) - 1;
+		num_sockets = num_elems;
 	}
 
+	sock_fd = calloc(num_sockets, sizeof(*sock_fd));
+	if (CHECK(!sock_fd, "calloc(sock_fd)", "failed to allocate\n"))
+		goto out;
+
+	for (i = 0; i < num_sockets; i++)
+		sock_fd[i] = -1;
+
 	src_fd = bpf_map__fd(src);
 
 	for (i = 0; i < num_sockets; i++) {
@@ -258,8 +257,8 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
 		goto close_iter;
 
 	/* test results */
-	if (CHECK(skel->bss->elems != max_elems, "elems", "got %u expected %u\n",
-		  skel->bss->elems, max_elems))
+	if (CHECK(skel->bss->elems != num_elems, "elems", "got %u expected %u\n",
+		  skel->bss->elems, num_elems))
 		goto close_iter;
 
 	if (CHECK(skel->bss->socks != num_sockets, "socks", "got %u expected %u\n",
@@ -271,10 +270,11 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
 free_link:
 	bpf_link__destroy(link);
 out:
-	for (i = 0; i < num_sockets; i++) {
+	for (i = 0; sock_fd && i < num_sockets; i++)
 		if (sock_fd[i] >= 0)
 			close(sock_fd[i]);
-	}
+	if (sock_fd)
+		free(sock_fd);
 	bpf_iter_sockmap__destroy(skel);
 }
 
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
index 0e27f73dd803..1af7555f6057 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
@@ -2,7 +2,6 @@
 /* Copyright (c) 2020 Cloudflare */
 #include "bpf_iter.h"
 #include "bpf_tracing_net.h"
-#include "bpf_iter_sockmap.h"
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
 #include <errno.h>
@@ -11,14 +10,14 @@ char _license[] SEC("license") = "GPL";
 
 struct {
 	__uint(type, BPF_MAP_TYPE_SOCKMAP);
-	__uint(max_entries, SOCKMAP_MAX_ENTRIES);
+	__uint(max_entries, 64);
 	__type(key, __u32);
 	__type(value, __u64);
 } sockmap SEC(".maps");
 
 struct {
 	__uint(type, BPF_MAP_TYPE_SOCKHASH);
-	__uint(max_entries, SOCKMAP_MAX_ENTRIES);
+	__uint(max_entries, 64);
 	__type(key, __u32);
 	__type(value, __u64);
 } sockhash SEC(".maps");
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h
deleted file mode 100644
index 35a675d13c0f..000000000000
--- a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h
+++ /dev/null
@@ -1,3 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-
-#define SOCKMAP_MAX_ENTRIES (64)
-- 
2.25.1

[PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash

[Lorenz Bauer]

Since we can now call map_update_elem(sockmap) from bpf_iter context
it's possible to copy a sockmap or sockhash in the kernel. Add a
selftest which exercises this.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 14 +++++-----
 .../selftests/bpf/progs/bpf_iter_sockmap.c    | 27 +++++++++++++++----
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index e8a4bfb4d9f4..854a508e81ce 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -194,7 +194,7 @@ static void test_sockmap_invalid_update(void)
 		test_sockmap_invalid_update__destroy(skel);
 }
 
-static void test_sockmap_iter(enum bpf_map_type map_type)
+static void test_sockmap_copy(enum bpf_map_type map_type)
 {
 	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
 	int err, len, src_fd, iter_fd, duration = 0;
@@ -242,7 +242,7 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
 	linfo.map.map_fd = src_fd;
 	opts.link_info = &linfo;
 	opts.link_info_len = sizeof(linfo);
-	link = bpf_program__attach_iter(skel->progs.count_elems, &opts);
+	link = bpf_program__attach_iter(skel->progs.copy, &opts);
 	if (CHECK(IS_ERR(link), "attach_iter", "attach_iter failed\n"))
 		goto out;
 
@@ -265,6 +265,8 @@ static void test_sockmap_iter(enum bpf_map_type map_type)
 		  skel->bss->socks, num_sockets))
 		goto close_iter;
 
+	compare_cookies(src, skel->maps.dst);
+
 close_iter:
 	close(iter_fd);
 free_link:
@@ -294,8 +296,8 @@ void test_sockmap_basic(void)
 		test_sockmap_update(BPF_MAP_TYPE_SOCKHASH);
 	if (test__start_subtest("sockmap update in unsafe context"))
 		test_sockmap_invalid_update();
-	if (test__start_subtest("sockmap iter"))
-		test_sockmap_iter(BPF_MAP_TYPE_SOCKMAP);
-	if (test__start_subtest("sockhash iter"))
-		test_sockmap_iter(BPF_MAP_TYPE_SOCKHASH);
+	if (test__start_subtest("sockmap copy"))
+		test_sockmap_copy(BPF_MAP_TYPE_SOCKMAP);
+	if (test__start_subtest("sockhash copy"))
+		test_sockmap_copy(BPF_MAP_TYPE_SOCKHASH);
 }
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
index 1af7555f6057..f3af0e30cead 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c
@@ -22,21 +22,38 @@ struct {
 	__type(value, __u64);
 } sockhash SEC(".maps");
 
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKHASH);
+	__uint(max_entries, 64);
+	__type(key, __u32);
+	__type(value, __u64);
+} dst SEC(".maps");
+
 __u32 elems = 0;
 __u32 socks = 0;
 
 SEC("iter/sockmap")
-int count_elems(struct bpf_iter__sockmap *ctx)
+int copy(struct bpf_iter__sockmap *ctx)
 {
 	struct sock *sk = ctx->sk;
 	__u32 tmp, *key = ctx->key;
 	int ret;
 
-	if (key)
-		elems++;
+	if (!key)
+		return 0;
 
-	if (sk)
+	elems++;
+
+	/* We need a temporary buffer on the stack, since the verifier doesn't
+	 * let us use the pointer from the context as an argument to the helper.
+	 */
+	tmp = *key;
+
+	if (sk) {
 		socks++;
+		return bpf_map_update_elem(&dst, &tmp, sk, 0) != 0;
+	}
 
-	return 0;
+	ret = bpf_map_delete_elem(&dst, &tmp);
+	return ret && ret != -ENOENT;
 }
-- 
2.25.1

Re: [PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter

[Martin KaFai Lau]

On Mon, Sep 28, 2020 at 10:08:02AM +0100, Lorenz Bauer wrote:
> Allow passing a pointer to a BTF struct sock_common* when updating
> a sockmap or sockhash. Since BTF pointers can fault and therefore be
> NULL at runtime we need to add an additional !sk check to
> sock_map_update_elem. Since we may be passed a request or timewait
> socket we also need to check sk_fullsock. Doing this allows calling
> map_update_elem on sockmap from bpf_iter context, which uses
> BTF pointers.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Martin KaFai Lau]

On Mon, Sep 28, 2020 at 10:08:03AM +0100, Lorenz Bauer wrote:
> We compare socket cookies to ensure that insertion into a sockmap worked.
> Pull this out into a helper function for use in other tests.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  .../selftests/bpf/prog_tests/sockmap_basic.c  | 50 +++++++++++++------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 4b7a527e7e82..67d3301bdf81 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -50,6 +50,37 @@ static int connected_socket_v4(void)
>  	return -1;
>  }
>  
> +static void compare_cookies(struct bpf_map *src, struct bpf_map *dst)
> +{
> +	__u32 i, max_entries = bpf_map__max_entries(src);
> +	int err, duration, src_fd, dst_fd;
This should have a compiler warning.  "duration" is not initialized.

Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash

[Martin KaFai Lau]

On Mon, Sep 28, 2020 at 10:08:05AM +0100, Lorenz Bauer wrote:

[ ... ]

>  SEC("iter/sockmap")
> -int count_elems(struct bpf_iter__sockmap *ctx)
> +int copy(struct bpf_iter__sockmap *ctx)
>  {
>  	struct sock *sk = ctx->sk;
>  	__u32 tmp, *key = ctx->key;
>  	int ret;
>  
> -	if (key)
> -		elems++;
> +	if (!key)
> +		return 0;
>  
> -	if (sk)
> +	elems++;
> +
> +	/* We need a temporary buffer on the stack, since the verifier doesn't
> +	 * let us use the pointer from the context as an argument to the helper.
Is it something that can be improved later?

others LGTM.

Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash

[Lorenz Bauer]

On Tue, 29 Sep 2020 at 07:06, Martin KaFai Lau <kafai@fb.com> wrote:

...

> > +     /* We need a temporary buffer on the stack, since the verifier doesn't
> > +      * let us use the pointer from the context as an argument to the helper.
> Is it something that can be improved later?
>
> others LGTM.

Yeah, I think so. We'd need to do something similar to your
sock_common work for PTR_TO_RDONLY_BUF_OR_NULL. The fact that the
pointer is read only makes it a bit more difficult I think. After
that, a user could just plug the key into map_update_elem directly.
Alternatively, allow specialising map_ops per context.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Alexei Starovoitov]

On Mon, Sep 28, 2020 at 10:59 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > +static void compare_cookies(struct bpf_map *src, struct bpf_map *dst)
> > +{
> > +     __u32 i, max_entries = bpf_map__max_entries(src);
> > +     int err, duration, src_fd, dst_fd;
> This should have a compiler warning.  "duration" is not initialized.

There was a warning. I noticed it while applying and fixed it up.
Lorenz, please upgrade your compiler. This is not the first time such
warning has been missed.

Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash

[Martin KaFai Lau]

On Tue, Sep 29, 2020 at 10:21:05AM +0100, Lorenz Bauer wrote:
> On Tue, 29 Sep 2020 at 07:06, Martin KaFai Lau <kafai@fb.com> wrote:
> 
> ...
> 
> > > +     /* We need a temporary buffer on the stack, since the verifier doesn't
> > > +      * let us use the pointer from the context as an argument to the helper.
> > Is it something that can be improved later?
> >
> > others LGTM.
> 
> Yeah, I think so. We'd need to do something similar to your
> sock_common work for PTR_TO_RDONLY_BUF_OR_NULL. The fact that the
> pointer is read only makes it a bit more difficult I think. After
I thought the key arg should be used as read-only in the map's helper.
or there is map type's helper that modifies the key?

> that, a user could just plug the key into map_update_elem directly.
> Alternatively, allow specialising map_ops per context.

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Lorenz Bauer]

On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:

...

> There was a warning. I noticed it while applying and fixed it up.
> Lorenz, please upgrade your compiler. This is not the first time such
> warning has been missed.

I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc
9.3.0 by removing the initialization of duration:

make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
  TEST-OBJ [test_progs] sockmap_basic.test.o
  TEST-HDR [test_progs] tests.h
  EXT-OBJ  [test_progs] test_progs.o
  EXT-OBJ  [test_progs] cgroup_helpers.o
  EXT-OBJ  [test_progs] trace_helpers.o
  EXT-OBJ  [test_progs] network_helpers.o
  EXT-OBJ  [test_progs] testing_helpers.o
  BINARY   test_progs
make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'

So, gcc doesn't issue a warning. Jakub did the following little experiment:

jkbs@toad ~/tmp $ cat warning.c
#include <stdio.h>

int main(void)
{
        int duration;

        fprintf(stdout, "%d", duration);

        return 0;
}
jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
warning.c: In function ‘main’:
warning.c:7:2: warning: ‘duration’ is used uninitialized in this
function [-Wuninitialized]
    7 |  fprintf(stdout, "%d", duration);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


The simple case seems to work. However, adding the macro breaks things:

jkbs@toad ~/tmp $ cat warning.c
#include <stdio.h>

#define _CHECK(duration) \
        ({                                                      \
                fprintf(stdout, "%d", duration);                \
        })
#define CHECK() _CHECK(duration)

int main(void)
{
        int duration;

        CHECK();

        return 0;
}
jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
jkbs@toad ~/tmp $

Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The
problem is still there on gcc 10. Compiling test_progs with clang does
issue a warning FWIW, but it seems like other things break when doing
that.

--
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash

[Lorenz Bauer]

On Tue, 29 Sep 2020 at 18:23, Martin KaFai Lau <kafai@fb.com> wrote:

...

> > Yeah, I think so. We'd need to do something similar to your
> > sock_common work for PTR_TO_RDONLY_BUF_OR_NULL. The fact that the
> > pointer is read only makes it a bit more difficult I think. After
> I thought the key arg should be used as read-only in the map's helper.
> or there is map type's helper that modifies the key?

I don't know, that's what I meant by more difficult. If map keys are
always read-only like you say this would be straight forward to do
(famous last words).

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Alexei Starovoitov]

On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote:
> On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> 
> ...
> 
> > There was a warning. I noticed it while applying and fixed it up.
> > Lorenz, please upgrade your compiler. This is not the first time such
> > warning has been missed.
> 
> I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc
> 9.3.0 by removing the initialization of duration:
> 
> make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
>   TEST-OBJ [test_progs] sockmap_basic.test.o
>   TEST-HDR [test_progs] tests.h
>   EXT-OBJ  [test_progs] test_progs.o
>   EXT-OBJ  [test_progs] cgroup_helpers.o
>   EXT-OBJ  [test_progs] trace_helpers.o
>   EXT-OBJ  [test_progs] network_helpers.o
>   EXT-OBJ  [test_progs] testing_helpers.o
>   BINARY   test_progs
> make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
> 
> So, gcc doesn't issue a warning. Jakub did the following little experiment:
> 
> jkbs@toad ~/tmp $ cat warning.c
> #include <stdio.h>
> 
> int main(void)
> {
>         int duration;
> 
>         fprintf(stdout, "%d", duration);
> 
>         return 0;
> }
> jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
> warning.c: In function ‘main’:
> warning.c:7:2: warning: ‘duration’ is used uninitialized in this
> function [-Wuninitialized]
>     7 |  fprintf(stdout, "%d", duration);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 
> The simple case seems to work. However, adding the macro breaks things:
> 
> jkbs@toad ~/tmp $ cat warning.c
> #include <stdio.h>
> 
> #define _CHECK(duration) \
>         ({                                                      \
>                 fprintf(stdout, "%d", duration);                \
>         })
> #define CHECK() _CHECK(duration)
> 
> int main(void)
> {
>         int duration;
> 
>         CHECK();
> 
>         return 0;
> }
> jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
> jkbs@toad ~/tmp $

That's very interesting. Thanks for the pointers.
I'm using gcc version 9.1.1 20190605 (Red Hat 9.1.1-2)
and I saw this warning while compiling selftests,
but I don't see it with above warning.c example.
clang warns correctly in both cases.

> Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The
> problem is still there on gcc 10. Compiling test_progs with clang does
> issue a warning FWIW, but it seems like other things break when doing
> that.

That gcc bug has been opened since transition to ssa. That was a huge
transition for gcc. But I think the bug number is not correct. It points to a
different issue. I've checked -fdump-tree-uninit-all dump with and without
macro. They're identical. The tree-ssa-uninit pass suppose to warn, but it
doesn't. I wish I had more time to dig into it. A bit of debugging in
gcc/tree-ssa-uninit.c can probably uncover the root cause.

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Andrii Nakryiko]

On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote:
> > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >
> > ...
> >
> > > There was a warning. I noticed it while applying and fixed it up.
> > > Lorenz, please upgrade your compiler. This is not the first time such
> > > warning has been missed.
> >
> > I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc
> > 9.3.0 by removing the initialization of duration:
> >
> > make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
> >   TEST-OBJ [test_progs] sockmap_basic.test.o
> >   TEST-HDR [test_progs] tests.h
> >   EXT-OBJ  [test_progs] test_progs.o
> >   EXT-OBJ  [test_progs] cgroup_helpers.o
> >   EXT-OBJ  [test_progs] trace_helpers.o
> >   EXT-OBJ  [test_progs] network_helpers.o
> >   EXT-OBJ  [test_progs] testing_helpers.o
> >   BINARY   test_progs
> > make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
> >
> > So, gcc doesn't issue a warning. Jakub did the following little experiment:
> >
> > jkbs@toad ~/tmp $ cat warning.c
> > #include <stdio.h>
> >
> > int main(void)
> > {
> >         int duration;
> >
> >         fprintf(stdout, "%d", duration);
> >
> >         return 0;
> > }
> > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
> > warning.c: In function ‘main’:
> > warning.c:7:2: warning: ‘duration’ is used uninitialized in this
> > function [-Wuninitialized]
> >     7 |  fprintf(stdout, "%d", duration);
> >       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> >
> > The simple case seems to work. However, adding the macro breaks things:
> >
> > jkbs@toad ~/tmp $ cat warning.c
> > #include <stdio.h>
> >
> > #define _CHECK(duration) \
> >         ({                                                      \
> >                 fprintf(stdout, "%d", duration);                \
> >         })
> > #define CHECK() _CHECK(duration)
> >
> > int main(void)
> > {
> >         int duration;
> >
> >         CHECK();
> >
> >         return 0;
> > }
> > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
> > jkbs@toad ~/tmp $
>
> That's very interesting. Thanks for the pointers.
> I'm using gcc version 9.1.1 20190605 (Red Hat 9.1.1-2)
> and I saw this warning while compiling selftests,
> but I don't see it with above warning.c example.
> clang warns correctly in both cases.

I think this might be the same problem I fixed for libbpf with [0].
Turns out, GCC explicitly calls out (somewhere in their docs) that
uninitialized variable warnings work only when compiled in optimized
mode, because some internal data structures used to detect this are
only maintained in optimized mode build.

Laurenz, can you try compiling your example with -O2?

  [0] https://patchwork.ozlabs.org/project/netdev/patch/20200929220604.833631-2-andriin@fb.com/

>
> > Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The
> > problem is still there on gcc 10. Compiling test_progs with clang does
> > issue a warning FWIW, but it seems like other things break when doing
> > that.
>
> That gcc bug has been opened since transition to ssa. That was a huge
> transition for gcc. But I think the bug number is not correct. It points to a
> different issue. I've checked -fdump-tree-uninit-all dump with and without
> macro. They're identical. The tree-ssa-uninit pass suppose to warn, but it
> doesn't. I wish I had more time to dig into it. A bit of debugging in
> gcc/tree-ssa-uninit.c can probably uncover the root cause.

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Alexei Starovoitov]

On Thu, Oct 1, 2020 at 10:09 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote:
> > > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > ...
> > >
> > > > There was a warning. I noticed it while applying and fixed it up.
> > > > Lorenz, please upgrade your compiler. This is not the first time such
> > > > warning has been missed.
> > >
> > > I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc
> > > 9.3.0 by removing the initialization of duration:
> > >
> > > make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
> > >   TEST-OBJ [test_progs] sockmap_basic.test.o
> > >   TEST-HDR [test_progs] tests.h
> > >   EXT-OBJ  [test_progs] test_progs.o
> > >   EXT-OBJ  [test_progs] cgroup_helpers.o
> > >   EXT-OBJ  [test_progs] trace_helpers.o
> > >   EXT-OBJ  [test_progs] network_helpers.o
> > >   EXT-OBJ  [test_progs] testing_helpers.o
> > >   BINARY   test_progs
> > > make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf'
> > >
> > > So, gcc doesn't issue a warning. Jakub did the following little experiment:
> > >
> > > jkbs@toad ~/tmp $ cat warning.c
> > > #include <stdio.h>
> > >
> > > int main(void)
> > > {
> > >         int duration;
> > >
> > >         fprintf(stdout, "%d", duration);
> > >
> > >         return 0;
> > > }
> > > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
> > > warning.c: In function ‘main’:
> > > warning.c:7:2: warning: ‘duration’ is used uninitialized in this
> > > function [-Wuninitialized]
> > >     7 |  fprintf(stdout, "%d", duration);
> > >       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > >
> > > The simple case seems to work. However, adding the macro breaks things:
> > >
> > > jkbs@toad ~/tmp $ cat warning.c
> > > #include <stdio.h>
> > >
> > > #define _CHECK(duration) \
> > >         ({                                                      \
> > >                 fprintf(stdout, "%d", duration);                \
> > >         })
> > > #define CHECK() _CHECK(duration)
> > >
> > > int main(void)
> > > {
> > >         int duration;
> > >
> > >         CHECK();
> > >
> > >         return 0;
> > > }
> > > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c
> > > jkbs@toad ~/tmp $
> >
> > That's very interesting. Thanks for the pointers.
> > I'm using gcc version 9.1.1 20190605 (Red Hat 9.1.1-2)
> > and I saw this warning while compiling selftests,
> > but I don't see it with above warning.c example.
> > clang warns correctly in both cases.
>
> I think this might be the same problem I fixed for libbpf with [0].
> Turns out, GCC explicitly calls out (somewhere in their docs) that
> uninitialized variable warnings work only when compiled in optimized
> mode, because some internal data structures used to detect this are
> only maintained in optimized mode build.
>
> Laurenz, can you try compiling your example with -O2?

All of my experiments I did with -O2.

>   [0] https://patchwork.ozlabs.org/project/netdev/patch/20200929220604.833631-2-andriin@fb.com/
>
> >
> > > Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The
> > > problem is still there on gcc 10. Compiling test_progs with clang does
> > > issue a warning FWIW, but it seems like other things break when doing
> > > that.
> >
> > That gcc bug has been opened since transition to ssa. That was a huge
> > transition for gcc. But I think the bug number is not correct. It points to a
> > different issue. I've checked -fdump-tree-uninit-all dump with and without
> > macro. They're identical. The tree-ssa-uninit pass suppose to warn, but it
> > doesn't. I wish I had more time to dig into it. A bit of debugging in
> > gcc/tree-ssa-uninit.c can probably uncover the root cause.

Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Lorenz Bauer]

On Thu, 1 Oct 2020 at 18:11, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> >
> > I think this might be the same problem I fixed for libbpf with [0].
> > Turns out, GCC explicitly calls out (somewhere in their docs) that
> > uninitialized variable warnings work only when compiled in optimized
> > mode, because some internal data structures used to detect this are
> > only maintained in optimized mode build.
> >
> > Laurenz, can you try compiling your example with -O2?
>
> All of my experiments I did with -O2.

If anybody wants to play with this more: https://godbolt.org/z/77P6P9

Seems like red hat GCC has some special sauce that fixes this behaviour?

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com