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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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.
On Mon, Sep 28, 2020 at 10:59 PM Martin KaFai Lau <[email protected]> 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.
On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
<[email protected]> 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
http://www.cloudflare.com
On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote:
> On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
> <[email protected]> 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.
On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov
<[email protected]> wrote:
>
> On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote:
> > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
> > <[email protected]> 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/[email protected]/
>
> > 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.
On Thu, Oct 1, 2020 at 10:09 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote:
> > > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov
> > > <[email protected]> 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/[email protected]/
>
> >
> > > 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.
On Thu, 1 Oct 2020 at 18:11, Alexei Starovoitov
<[email protected]> 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
http://www.cloudflare.com