* [PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter
2020-09-28 9:08 [PATCH bpf-next v2 0/4] Sockmap copying Lorenz Bauer
@ 2020-09-28 9:08 ` Lorenz Bauer
2020-09-29 5:35 ` Martin KaFai Lau
2020-09-28 9:08 ` [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies Lorenz Bauer
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-28 9:08 UTC (permalink / raw)
To: kafai, Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Jakub Sitnicki, Lorenz Bauer, David S. Miller, Jakub Kicinski
Cc: kernel-team, netdev, bpf, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter
2020-09-28 9:08 ` [PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter Lorenz Bauer
@ 2020-09-29 5:35 ` Martin KaFai Lau
0 siblings, 0 replies; 17+ messages in thread
From: Martin KaFai Lau @ 2020-09-29 5:35 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Jakub Sitnicki, David S. Miller, Jakub Kicinski, kernel-team,
netdev, bpf, linux-kernel
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-09-28 9:08 [PATCH bpf-next v2 0/4] Sockmap copying Lorenz Bauer
2020-09-28 9:08 ` [PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter Lorenz Bauer
@ 2020-09-28 9:08 ` Lorenz Bauer
2020-09-29 5:59 ` Martin KaFai Lau
2020-09-28 9:08 ` [PATCH bpf-next v2 3/4] selftests: bpf: remove shared header from sockmap iter test Lorenz Bauer
2020-09-28 9:08 ` [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash Lorenz Bauer
3 siblings, 1 reply; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-28 9:08 UTC (permalink / raw)
To: kafai, Shuah Khan, Alexei Starovoitov, Daniel Borkmann
Cc: kernel-team, Lorenz Bauer, linux-kselftest, netdev, bpf, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-09-28 9:08 ` [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies Lorenz Bauer
@ 2020-09-29 5:59 ` Martin KaFai Lau
2020-09-29 15:48 ` Alexei Starovoitov
0 siblings, 1 reply; 17+ messages in thread
From: Martin KaFai Lau @ 2020-09-29 5:59 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, kernel-team,
linux-kselftest, netdev, bpf, linux-kernel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-09-29 5:59 ` Martin KaFai Lau
@ 2020-09-29 15:48 ` Alexei Starovoitov
2020-09-30 9:28 ` Lorenz Bauer
0 siblings, 1 reply; 17+ messages in thread
From: Alexei Starovoitov @ 2020-09-29 15:48 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Lorenz Bauer, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
kernel-team, open list:KERNEL SELFTEST FRAMEWORK,
Network Development, bpf, LKML
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-09-29 15:48 ` Alexei Starovoitov
@ 2020-09-30 9:28 ` Lorenz Bauer
2020-10-01 7:23 ` Alexei Starovoitov
0 siblings, 1 reply; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-30 9:28 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Shuah Khan, Alexei Starovoitov,
Daniel Borkmann, kernel-team,
open list:KERNEL SELFTEST FRAMEWORK, Network Development, bpf,
LKML
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-09-30 9:28 ` Lorenz Bauer
@ 2020-10-01 7:23 ` Alexei Starovoitov
2020-10-01 17:09 ` Andrii Nakryiko
0 siblings, 1 reply; 17+ messages in thread
From: Alexei Starovoitov @ 2020-10-01 7:23 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Martin KaFai Lau, Shuah Khan, Alexei Starovoitov,
Daniel Borkmann, kernel-team,
open list:KERNEL SELFTEST FRAMEWORK, Network Development, bpf,
LKML
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-10-01 7:23 ` Alexei Starovoitov
@ 2020-10-01 17:09 ` Andrii Nakryiko
2020-10-01 17:11 ` Alexei Starovoitov
0 siblings, 1 reply; 17+ messages in thread
From: Andrii Nakryiko @ 2020-10-01 17:09 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Lorenz Bauer, Martin KaFai Lau, Shuah Khan, Alexei Starovoitov,
Daniel Borkmann, kernel-team,
open list:KERNEL SELFTEST FRAMEWORK, Network Development, bpf,
LKML
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-10-01 17:09 ` Andrii Nakryiko
@ 2020-10-01 17:11 ` Alexei Starovoitov
2020-10-02 10:08 ` Lorenz Bauer
0 siblings, 1 reply; 17+ messages in thread
From: Alexei Starovoitov @ 2020-10-01 17:11 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Lorenz Bauer, Martin KaFai Lau, Shuah Khan, Alexei Starovoitov,
Daniel Borkmann, kernel-team,
open list:KERNEL SELFTEST FRAMEWORK, Network Development, bpf,
LKML
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
2020-10-01 17:11 ` Alexei Starovoitov
@ 2020-10-02 10:08 ` Lorenz Bauer
0 siblings, 0 replies; 17+ messages in thread
From: Lorenz Bauer @ 2020-10-02 10:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, Martin KaFai Lau, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, kernel-team,
open list:KERNEL SELFTEST FRAMEWORK, Network Development, bpf,
LKML
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH bpf-next v2 3/4] selftests: bpf: remove shared header from sockmap iter test
2020-09-28 9:08 [PATCH bpf-next v2 0/4] Sockmap copying Lorenz Bauer
2020-09-28 9:08 ` [PATCH bpf-next v2 1/4] bpf: sockmap: enable map_update_elem from bpf_iter Lorenz Bauer
2020-09-28 9:08 ` [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies Lorenz Bauer
@ 2020-09-28 9:08 ` Lorenz Bauer
2020-09-28 9:08 ` [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash Lorenz Bauer
3 siblings, 0 replies; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-28 9:08 UTC (permalink / raw)
To: kafai, Shuah Khan, Alexei Starovoitov, Daniel Borkmann
Cc: kernel-team, Lorenz Bauer, Yonghong Song, linux-kernel,
linux-kselftest, netdev, bpf
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash
2020-09-28 9:08 [PATCH bpf-next v2 0/4] Sockmap copying Lorenz Bauer
` (2 preceding siblings ...)
2020-09-28 9:08 ` [PATCH bpf-next v2 3/4] selftests: bpf: remove shared header from sockmap iter test Lorenz Bauer
@ 2020-09-28 9:08 ` Lorenz Bauer
2020-09-29 6:06 ` Martin KaFai Lau
3 siblings, 1 reply; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-28 9:08 UTC (permalink / raw)
To: kafai, Shuah Khan, Alexei Starovoitov, Daniel Borkmann
Cc: kernel-team, Lorenz Bauer, linux-kselftest, netdev, bpf, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash
2020-09-28 9:08 ` [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash Lorenz Bauer
@ 2020-09-29 6:06 ` Martin KaFai Lau
2020-09-29 9:21 ` Lorenz Bauer
0 siblings, 1 reply; 17+ messages in thread
From: Martin KaFai Lau @ 2020-09-29 6:06 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, kernel-team,
linux-kselftest, netdev, bpf, linux-kernel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash
2020-09-29 6:06 ` Martin KaFai Lau
@ 2020-09-29 9:21 ` Lorenz Bauer
2020-09-29 17:23 ` Martin KaFai Lau
0 siblings, 1 reply; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-29 9:21 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, kernel-team,
linux-kselftest, Networking, bpf, LKML
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash
2020-09-29 9:21 ` Lorenz Bauer
@ 2020-09-29 17:23 ` Martin KaFai Lau
2020-09-30 9:37 ` Lorenz Bauer
0 siblings, 1 reply; 17+ messages in thread
From: Martin KaFai Lau @ 2020-09-29 17:23 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, kernel-team,
linux-kselftest, Networking, bpf, LKML
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v2 4/4] selftest: bpf: Test copying a sockmap and sockhash
2020-09-29 17:23 ` Martin KaFai Lau
@ 2020-09-30 9:37 ` Lorenz Bauer
0 siblings, 0 replies; 17+ messages in thread
From: Lorenz Bauer @ 2020-09-30 9:37 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, kernel-team,
open list:KERNEL SELFTEST FRAMEWORK, Networking, bpf, LKML
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
^ permalink raw reply [flat|nested] 17+ messages in thread