[PATCH bpf-next v1 0/8] selftests/bpf: Improve libc portability / musl support (part 2)

[PATCH bpf-next v1 0/8] selftests/bpf: Improve libc portability / musl support (part 2)

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Hello all,

This is part 2 of a series of fixes for libc-related issues encountered
building for musl-based systems. The series has been tested with the
kernel-patches/bpf CI and locally using mips64el-gcc/musl-libc and QEMU
with an OpenWrt rootfs.

The patches cover a few areas of portability issues:

 - improper libc usage (strtok_r(), reserved identifiers)
 - gcc compile errors (include header ordering, sequence-point errors)
 - POSIX vs GNU basename()
 - missing GNU extensions (<execinfo.h>, C++ <stdbool.h>)
 - Y2038 and setsockopt() / SO_TIMESTAMPNS_NEW

Feedback and suggestions are appreciated!

Thanks,
Tony



Tony Ambardar (8):
  selftests/bpf: Use portable POSIX basename()
  selftests/bpf: Fix arg parsing in veristat, test_progs
  selftests/bpf: Fix error compiling test_lru_map.c
  selftests/bpf: Fix C++ compile error from missing _Bool type
  selftests/bpf: Fix order-of-include compile errors in lwt_reroute.c
  selftests/bpf: Fix compile if backtrace support missing in libc
  selftests/bpf: Fix using stdout, stderr as struct field names
  selftests/bpf: Fix error compiling tc_redirect.c with musl libc

 .../selftests/bpf/prog_tests/lwt_helpers.h    |  3 +-
 .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
 .../selftests/bpf/prog_tests/tc_redirect.c    | 12 +--
 tools/testing/selftests/bpf/test_cpp.cpp      |  4 +
 tools/testing/selftests/bpf/test_lru_map.c    |  3 +-
 tools/testing/selftests/bpf/test_progs.c      | 75 ++++++++++---------
 tools/testing/selftests/bpf/test_progs.h      |  8 +-
 tools/testing/selftests/bpf/testing_helpers.c |  2 +-
 tools/testing/selftests/bpf/veristat.c        | 12 +--
 tools/testing/selftests/bpf/xskxceiver.c      |  1 +
 10 files changed, 68 insertions(+), 54 deletions(-)

-- 
2.34.1

[PATCH bpf-next v1 1/8] selftests/bpf: Use portable POSIX basename()

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Use the POSIX version of basename() to allow compilation against non-gnu
libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using
functions from the latter while preferring POSIX over GNU basename().

In veristat.c, rely on strdupa() to avoid basename() altering the passed
"const char" argument. This is not needed in xskxceiver.c since the arg
is mutable and the program exits immediately after usage.

Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/veristat.c   | 8 +++++---
 tools/testing/selftests/bpf/xskxceiver.c | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index b2854238d4a0..11ec1190d582 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
 #define _GNU_SOURCE
 #include <argp.h>
+#include <libgen.h>
 #include <string.h>
 #include <stdlib.h>
 #include <sched.h>
@@ -988,8 +989,8 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
 
 static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
 {
+	const char *base_filename = basename(strdupa(filename));
 	const char *prog_name = bpf_program__name(prog);
-	const char *base_filename = basename(filename);
 	char *buf;
 	int buf_sz, log_level;
 	struct verif_stats *stats;
@@ -1056,13 +1057,14 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 
 static int process_obj(const char *filename)
 {
+	const char *base_filename = basename(strdupa(filename));
 	struct bpf_object *obj = NULL, *tobj;
 	struct bpf_program *prog, *tprog, *lprog;
 	libbpf_print_fn_t old_libbpf_print_fn;
 	LIBBPF_OPTS(bpf_object_open_opts, opts);
 	int err = 0, prog_cnt = 0;
 
-	if (!should_process_file_prog(basename(filename), NULL)) {
+	if (!should_process_file_prog(base_filename, NULL)) {
 		if (env.verbose)
 			printf("Skipping '%s' due to filters...\n", filename);
 		env.files_skipped++;
@@ -1076,7 +1078,7 @@ static int process_obj(const char *filename)
 	}
 
 	if (!env.quiet && env.out_fmt == RESFMT_TABLE)
-		printf("Processing '%s'...\n", basename(filename));
+		printf("Processing '%s'...\n", base_filename);
 
 	old_libbpf_print_fn = libbpf_set_print(libbpf_print_fn);
 	obj = bpf_object__open_file(filename, &opts);
diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 8144fd145237..92af633faea8 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -90,6 +90,7 @@
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <libgen.h>
 #include <string.h>
 #include <stddef.h>
 #include <sys/mman.h>
-- 
2.34.1

[PATCH bpf-next v1 2/8] selftests/bpf: Fix arg parsing in veristat, test_progs

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Current code parses arguments with strtok_r() using a construct like

    char *state = NULL;
    while ((next = strtok_r(state ? NULL : input, ",", &state))) {
        ...
    }

where logic assumes the 'state' var can distinguish between first and
subsequent strtok_r() calls, and adjusts parameters accordingly. However,
'state' is strictly internal context for strtok_r() and no such assumptions
are supported in the man page. Moreover, the exact behaviour of 'state'
depends on the libc implementation, making the above code fragile.

Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang,
with the above code in an infinite loop.

Similarly, we see strange behaviour running 'veristat' on mips64el/musl:

    $ ./veristat -e file,prog,verdict,insns -C two-ok add-failure
    Can't specify more than 9 stats

Rewrite code using a 'for' loop without logic dependent on var 'state', the
same approach already used in cgroup_helpers.c.

Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests")
Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat")
Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/testing_helpers.c | 2 +-
 tools/testing/selftests/bpf/veristat.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index ac7c66f4fc7b..2a73b72feb18 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -227,7 +227,7 @@ int parse_test_list(const char *s,
 	if (!input)
 		return -ENOMEM;
 
-	while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
+	for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) {
 		err = insert_test(set, test_spec, is_glob_pattern);
 		if (err)
 			break;
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 11ec1190d582..6808679827ac 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs)
 	if (!input)
 		return -ENOMEM;
 
-	while ((next = strtok_r(state ? NULL : input, ",", &state))) {
+	for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) {
 		err = parse_stat(next, specs);
 		if (err) {
 			free(input);
@@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs,
 			*stat_cntp += 1;
 		}
 
-		while ((next = strtok_r(state ? NULL : input, ",\n", &state))) {
+		for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) {
 			if (header) {
 				/* for the first line, set up spec stats */
 				err = parse_stat(next, specs);
-- 
2.34.1

Re: [PATCH bpf-next v1 2/8] selftests/bpf: Fix arg parsing in veristat, test_progs

[Andrii Nakryiko]

On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@gmail.com>
>
> Current code parses arguments with strtok_r() using a construct like
>
>     char *state = NULL;
>     while ((next = strtok_r(state ? NULL : input, ",", &state))) {
>         ...
>     }
>
> where logic assumes the 'state' var can distinguish between first and
> subsequent strtok_r() calls, and adjusts parameters accordingly. However,
> 'state' is strictly internal context for strtok_r() and no such assumptions
> are supported in the man page. Moreover, the exact behaviour of 'state'
> depends on the libc implementation, making the above code fragile.
>
> Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang,
> with the above code in an infinite loop.
>
> Similarly, we see strange behaviour running 'veristat' on mips64el/musl:
>
>     $ ./veristat -e file,prog,verdict,insns -C two-ok add-failure
>     Can't specify more than 9 stats
>
> Rewrite code using a 'for' loop without logic dependent on var 'state', the
> same approach already used in cgroup_helpers.c.
>
> Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests")
> Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat")
> Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files")
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  tools/testing/selftests/bpf/testing_helpers.c | 2 +-
>  tools/testing/selftests/bpf/veristat.c        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index ac7c66f4fc7b..2a73b72feb18 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -227,7 +227,7 @@ int parse_test_list(const char *s,
>         if (!input)
>                 return -ENOMEM;
>
> -       while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
> +       for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) {

oh, this is so long and verbose, let's just add a counter and use that
to determine whether to pass NULL or input, ok?

>                 err = insert_test(set, test_spec, is_glob_pattern);
>                 if (err)
>                         break;
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 11ec1190d582..6808679827ac 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs)
>         if (!input)
>                 return -ENOMEM;
>
> -       while ((next = strtok_r(state ? NULL : input, ",", &state))) {
> +       for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) {

ditto, let's not duplicate strtok_r() calls

>                 err = parse_stat(next, specs);
>                 if (err) {
>                         free(input);
> @@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs,
>                         *stat_cntp += 1;
>                 }
>
> -               while ((next = strtok_r(state ? NULL : input, ",\n", &state))) {
> +               for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) {
>                         if (header) {
>                                 /* for the first line, set up spec stats */
>                                 err = parse_stat(next, specs);
> --
> 2.34.1
>

Re: [PATCH bpf-next v1 2/8] selftests/bpf: Fix arg parsing in veristat, test_progs

[Tony Ambardar]

On Thu, Jul 25, 2024 at 01:09:24PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >

...

> > --- a/tools/testing/selftests/bpf/testing_helpers.c
> > +++ b/tools/testing/selftests/bpf/testing_helpers.c
> > @@ -227,7 +227,7 @@ int parse_test_list(const char *s,
> >         if (!input)
> >                 return -ENOMEM;
> >
> > -       while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
> > +       for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) {
> 
> oh, this is so long and verbose, let's just add a counter and use that
> to determine whether to pass NULL or input, ok?
> 

...

> > -       while ((next = strtok_r(state ? NULL : input, ",", &state))) {
> > +       for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) {
> 
> ditto, let's not duplicate strtok_r() calls
> 

Sounds good. I'll update for v2 and thanks for the suggestion.

...

[PATCH bpf-next v1 3/8] selftests/bpf: Fix error compiling test_lru_map.c

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Although the post-increment in macro 'CPU_SET(next++, &cpuset)' seems safe,
the sequencing can raise compile errors, so move the increment outside the
macro. This avoids an error seen using gcc 12.3.0 for mips64el/musl-libc:

  In file included from test_lru_map.c:11:
  test_lru_map.c: In function 'sched_next_online':
  test_lru_map.c:129:29: error: operation on 'next' may be undefined [-Werror=sequence-point]
    129 |                 CPU_SET(next++, &cpuset);
        |                             ^
  cc1: all warnings being treated as errors

Fixes: 3fbfadce6012 ("bpf: Fix test_lru_sanity5() in test_lru_map.c")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/test_lru_map.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_lru_map.c b/tools/testing/selftests/bpf/test_lru_map.c
index 4d0650cfb5cd..fda7589c5023 100644
--- a/tools/testing/selftests/bpf/test_lru_map.c
+++ b/tools/testing/selftests/bpf/test_lru_map.c
@@ -126,7 +126,8 @@ static int sched_next_online(int pid, int *next_to_try)
 
 	while (next < nr_cpus) {
 		CPU_ZERO(&cpuset);
-		CPU_SET(next++, &cpuset);
+		CPU_SET(next, &cpuset);
+		next++;
 		if (!sched_setaffinity(pid, sizeof(cpuset), &cpuset)) {
 			ret = 0;
 			break;
-- 
2.34.1

[PATCH bpf-next v1 4/8] selftests/bpf: Fix C++ compile error from missing _Bool type

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

While building, bpftool makes a skeleton from test_core_extern.c, which
itself includes <stdbool.h> and uses the 'bool' type. However, the skeleton
test_core_extern.skel.h generated *does not* include <stdbool.h> or use the
'bool' type, instead using the C-only '_Bool' type. Compiling test_cpp.cpp
with g++ 12.3 for mips64el/musl-libc then fails with error:

  In file included from test_cpp.cpp:9:
  test_core_extern.skel.h:45:17: error: '_Bool' does not name a type
     45 |                 _Bool CONFIG_BOOL;
        |                 ^~~~~

This was likely missed previously because glibc uses a GNU extension for
<stdbool.h> with C++ (#define _Bool bool), not supported by musl libc.

Normally, a C fragment would include <stdbool.h> and use the 'bool' type,
and thus cleanly work after import by C++. The ideal fix would be for
'bpftool gen skeleton' to output the correct type/include supporting C++,
but in the meantime add a conditional define as above.

Fixes: 7c8dce4b1661 ("bpftool: Make skeleton C code compilable with C++ compiler")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/test_cpp.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_cpp.cpp b/tools/testing/selftests/bpf/test_cpp.cpp
index dde0bb16e782..abc2a56ab261 100644
--- a/tools/testing/selftests/bpf/test_cpp.cpp
+++ b/tools/testing/selftests/bpf/test_cpp.cpp
@@ -6,6 +6,10 @@
 #include <bpf/libbpf.h>
 #include <bpf/bpf.h>
 #include <bpf/btf.h>
+
+#ifndef _Bool
+#define _Bool bool
+#endif
 #include "test_core_extern.skel.h"
 #include "struct_ops_module.skel.h"
 
-- 
2.34.1

[PATCH bpf-next v1 5/8] selftests/bpf: Fix order-of-include compile errors in lwt_reroute.c

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Fix redefinition errors seen compiling lwt_reroute.c for mips64el/musl-libc
by adjusting the order of includes in lwt_helpers.h. The ordering required
is:
<net/if.h>  -->  <arpa/inet.h> (from "test_progs.h")  -->  <linux/icmp.h>.

Because of the complexity and large number of includes, ordering appears to
be fragile however. Previously, with "test_progs.h" at the end of this
sequence, compiling with GCC 12.3 for mips64el/musl-libc yields errors:

In file included from .../include/arpa/inet.h:9,
                 from ./test_progs.h:18,
                 from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
.../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
   23 | struct in6_addr {
      |        ^~~~~~~~
In file included from .../include/linux/icmp.h:24,
                 from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9:
.../include/linux/in6.h:33:8: note: originally defined here
   33 | struct in6_addr {
      |        ^~~~~~~~
.../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
   34 | struct sockaddr_in6 {
      |        ^~~~~~~~~~~~
.../include/linux/in6.h:50:8: note: originally defined here
   50 | struct sockaddr_in6 {
      |        ^~~~~~~~~~~~
.../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
   42 | struct ipv6_mreq {
      |        ^~~~~~~~~
.../include/linux/in6.h:60:8: note: originally defined here
   60 | struct ipv6_mreq {
      |        ^~~~~~~~~

Similarly, with "test_progs.h" at the beginning of this sequence, compiling
with GCC 12.3 for x86_64 using glibc would fail like this:

In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
/usr/include/linux/if.h:83:9: error: redeclaration of enumerator ‘IFF_UP’
   83 |         IFF_UP                          = 1<<0,  /* sysfs */
      |         ^~~~~~
/usr/include/net/if.h:44:5: note: previous definition of ‘IFF_UP’ with type ‘enum <anonymous>’
   44 |     IFF_UP = 0x1,               /* Interface is up.  */
      |     ^~~~~~
/usr/include/linux/if.h:84:9: error: redeclaration of enumerator ‘IFF_BROADCAST’
   84 |         IFF_BROADCAST                   = 1<<1,  /* __volatile__ */
      |         ^~~~~~~~~~~~~
/usr/include/net/if.h:46:5: note: previous definition of ‘IFF_BROADCAST’ with type ‘enum <anonymous>’
   46 |     IFF_BROADCAST = 0x2,        /* Broadcast address valid.  */
      |     ^~~~~~~~~~~~~

...

In file included from /usr/include/linux/icmp.h:23,
                 from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
/usr/include/linux/if.h:194:8: error: redefinition of ‘struct ifmap’
  194 | struct ifmap {
      |        ^~~~~
In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
/usr/include/net/if.h:111:8: note: originally defined here
  111 | struct ifmap
      |        ^~~~~
In file included from /usr/include/linux/icmp.h:23,
                 from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
/usr/include/linux/if.h:232:8: error: redefinition of ‘struct ifreq’
  232 | struct ifreq {
      |        ^~~~~
In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
/usr/include/net/if.h:126:8: note: originally defined here
  126 | struct ifreq
      |        ^~~~~

Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/lwt_helpers.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
index fb1eb8c67361..8e5e28af03c5 100644
--- a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
@@ -6,10 +6,9 @@
 #include <time.h>
 #include <net/if.h>
 #include <linux/if_tun.h>
+#include "test_progs.h" /* between <net/if.h> and <linux/icmp.h> or errors */
 #include <linux/icmp.h>
 
-#include "test_progs.h"
-
 #define log_err(MSG, ...) \
 	fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
 		__FILE__, __LINE__, strerror(errno), ##__VA_ARGS__)
-- 
2.34.1

Re: [PATCH bpf-next v1 5/8] selftests/bpf: Fix order-of-include compile errors in lwt_reroute.c

[Andrii Nakryiko]

On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@gmail.com>
>
> Fix redefinition errors seen compiling lwt_reroute.c for mips64el/musl-libc
> by adjusting the order of includes in lwt_helpers.h. The ordering required
> is:
> <net/if.h>  -->  <arpa/inet.h> (from "test_progs.h")  -->  <linux/icmp.h>.
>
> Because of the complexity and large number of includes, ordering appears to
> be fragile however. Previously, with "test_progs.h" at the end of this
> sequence, compiling with GCC 12.3 for mips64el/musl-libc yields errors:
>
> In file included from .../include/arpa/inet.h:9,
>                  from ./test_progs.h:18,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> .../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
>    23 | struct in6_addr {
>       |        ^~~~~~~~
> In file included from .../include/linux/icmp.h:24,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9:
> .../include/linux/in6.h:33:8: note: originally defined here
>    33 | struct in6_addr {
>       |        ^~~~~~~~
> .../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
>    34 | struct sockaddr_in6 {
>       |        ^~~~~~~~~~~~
> .../include/linux/in6.h:50:8: note: originally defined here
>    50 | struct sockaddr_in6 {
>       |        ^~~~~~~~~~~~
> .../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
>    42 | struct ipv6_mreq {
>       |        ^~~~~~~~~
> .../include/linux/in6.h:60:8: note: originally defined here
>    60 | struct ipv6_mreq {
>       |        ^~~~~~~~~
>
> Similarly, with "test_progs.h" at the beginning of this sequence, compiling
> with GCC 12.3 for x86_64 using glibc would fail like this:
>
> In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> /usr/include/linux/if.h:83:9: error: redeclaration of enumerator ‘IFF_UP’
>    83 |         IFF_UP                          = 1<<0,  /* sysfs */
>       |         ^~~~~~
> /usr/include/net/if.h:44:5: note: previous definition of ‘IFF_UP’ with type ‘enum <anonymous>’
>    44 |     IFF_UP = 0x1,               /* Interface is up.  */
>       |     ^~~~~~
> /usr/include/linux/if.h:84:9: error: redeclaration of enumerator ‘IFF_BROADCAST’
>    84 |         IFF_BROADCAST                   = 1<<1,  /* __volatile__ */
>       |         ^~~~~~~~~~~~~
> /usr/include/net/if.h:46:5: note: previous definition of ‘IFF_BROADCAST’ with type ‘enum <anonymous>’
>    46 |     IFF_BROADCAST = 0x2,        /* Broadcast address valid.  */
>       |     ^~~~~~~~~~~~~
>
> ...
>
> In file included from /usr/include/linux/icmp.h:23,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> /usr/include/linux/if.h:194:8: error: redefinition of ‘struct ifmap’
>   194 | struct ifmap {
>       |        ^~~~~
> In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> /usr/include/net/if.h:111:8: note: originally defined here
>   111 | struct ifmap
>       |        ^~~~~
> In file included from /usr/include/linux/icmp.h:23,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> /usr/include/linux/if.h:232:8: error: redefinition of ‘struct ifreq’
>   232 | struct ifreq {
>       |        ^~~~~
> In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
>                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> /usr/include/net/if.h:126:8: note: originally defined here
>   126 | struct ifreq
>       |        ^~~~~
>
> Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT")
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/lwt_helpers.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
> index fb1eb8c67361..8e5e28af03c5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
> +++ b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
> @@ -6,10 +6,9 @@
>  #include <time.h>
>  #include <net/if.h>
>  #include <linux/if_tun.h>
> +#include "test_progs.h" /* between <net/if.h> and <linux/icmp.h> or errors */

Now we'll be papering over the real issue. Can you see if you can
untangle this mess and ensure that we consistently use either net/if.h
or linux/if.h headers?

pw-bot: cr

>  #include <linux/icmp.h>
>
> -#include "test_progs.h"
> -
>  #define log_err(MSG, ...) \
>         fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
>                 __FILE__, __LINE__, strerror(errno), ##__VA_ARGS__)
> --
> 2.34.1
>

Re: [PATCH bpf-next v1 5/8] selftests/bpf: Fix order-of-include compile errors in lwt_reroute.c

[Tony Ambardar]

On Thu, Jul 25, 2024 at 01:18:04PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@gmail.com>
> >
> > Fix redefinition errors seen compiling lwt_reroute.c for mips64el/musl-libc
> > by adjusting the order of includes in lwt_helpers.h. The ordering required
> > is:
> > <net/if.h>  -->  <arpa/inet.h> (from "test_progs.h")  -->  <linux/icmp.h>.
> >
> > Because of the complexity and large number of includes, ordering appears to
> > be fragile however. Previously, with "test_progs.h" at the end of this
> > sequence, compiling with GCC 12.3 for mips64el/musl-libc yields errors:
> >
> > In file included from .../include/arpa/inet.h:9,
> >                  from ./test_progs.h:18,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> > .../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
> >    23 | struct in6_addr {
> >       |        ^~~~~~~~
> > In file included from .../include/linux/icmp.h:24,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9:
> > .../include/linux/in6.h:33:8: note: originally defined here
> >    33 | struct in6_addr {
> >       |        ^~~~~~~~
> > .../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
> >    34 | struct sockaddr_in6 {
> >       |        ^~~~~~~~~~~~
> > .../include/linux/in6.h:50:8: note: originally defined here
> >    50 | struct sockaddr_in6 {
> >       |        ^~~~~~~~~~~~
> > .../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
> >    42 | struct ipv6_mreq {
> >       |        ^~~~~~~~~
> > .../include/linux/in6.h:60:8: note: originally defined here
> >    60 | struct ipv6_mreq {
> >       |        ^~~~~~~~~
> >
> > Similarly, with "test_progs.h" at the beginning of this sequence, compiling
> > with GCC 12.3 for x86_64 using glibc would fail like this:
> >
> > In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> > /usr/include/linux/if.h:83:9: error: redeclaration of enumerator ‘IFF_UP’
> >    83 |         IFF_UP                          = 1<<0,  /* sysfs */
> >       |         ^~~~~~
> > /usr/include/net/if.h:44:5: note: previous definition of ‘IFF_UP’ with type ‘enum <anonymous>’
> >    44 |     IFF_UP = 0x1,               /* Interface is up.  */
> >       |     ^~~~~~
> > /usr/include/linux/if.h:84:9: error: redeclaration of enumerator ‘IFF_BROADCAST’
> >    84 |         IFF_BROADCAST                   = 1<<1,  /* __volatile__ */
> >       |         ^~~~~~~~~~~~~
> > /usr/include/net/if.h:46:5: note: previous definition of ‘IFF_BROADCAST’ with type ‘enum <anonymous>’
> >    46 |     IFF_BROADCAST = 0x2,        /* Broadcast address valid.  */
> >       |     ^~~~~~~~~~~~~
> >
> > ...
> >
> > In file included from /usr/include/linux/icmp.h:23,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> > /usr/include/linux/if.h:194:8: error: redefinition of ‘struct ifmap’
> >   194 | struct ifmap {
> >       |        ^~~~~
> > In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> > /usr/include/net/if.h:111:8: note: originally defined here
> >   111 | struct ifmap
> >       |        ^~~~~
> > In file included from /usr/include/linux/icmp.h:23,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> > /usr/include/linux/if.h:232:8: error: redefinition of ‘struct ifreq’
> >   232 | struct ifreq {
> >       |        ^~~~~
> > In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8,
> >                  from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
> > /usr/include/net/if.h:126:8: note: originally defined here
> >   126 | struct ifreq
> >       |        ^~~~~
> >
> > Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT")
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/prog_tests/lwt_helpers.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
> > index fb1eb8c67361..8e5e28af03c5 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
> > +++ b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h
> > @@ -6,10 +6,9 @@
> >  #include <time.h>
> >  #include <net/if.h>
> >  #include <linux/if_tun.h>
> > +#include "test_progs.h" /* between <net/if.h> and <linux/icmp.h> or errors */
> 
> Now we'll be papering over the real issue. Can you see if you can
> untangle this mess and ensure that we consistently use either net/if.h
> or linux/if.h headers?
> 

Well, "untangle this mess" is certainly the right phrase, so I'll give it
another go and see what I can find.

> pw-bot: cr
> 
> >  #include <linux/icmp.h>
> >
> > -#include "test_progs.h"
> > -
> >  #define log_err(MSG, ...) \
> >         fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
> >                 __FILE__, __LINE__, strerror(errno), ##__VA_ARGS__)
> > --
> > 2.34.1
> >

[PATCH bpf-next v1 6/8] selftests/bpf: Fix compile if backtrace support missing in libc

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Use backtrace functions only with glibc and otherwise provide stubs in
test_progs.c. This avoids compile errors (e.g. with musl libc) like:

  test_progs.c:13:10: fatal error: execinfo.h: No such file or directory
     13 | #include <execinfo.h> /* backtrace */
        |          ^~~~~~~~~~~~
  test_progs.c: In function 'crash_handler':
  test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration]
   1034 |         sz = backtrace(bt, ARRAY_SIZE(bt));
        |              ^~~~~~~~~
  test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration]
   1045 |         backtrace_symbols_fd(bt, sz, STDERR_FILENO);
        |         ^~~~~~~~~~~~~~~~~~~~

Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/test_progs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 60c5ec0f6abf..f6cfc6a8e8f0 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -10,7 +10,6 @@
 #include <sched.h>
 #include <signal.h>
 #include <string.h>
-#include <execinfo.h> /* backtrace */
 #include <sys/sysinfo.h> /* get_nprocs */
 #include <netinet/in.h>
 #include <sys/select.h>
@@ -19,6 +18,14 @@
 #include <bpf/btf.h>
 #include "json_writer.h"
 
+#ifdef __GLIBC__
+#include <execinfo.h> /* backtrace */
+#else
+#define backtrace(...) (0)
+#define backtrace_symbols_fd(bt, sz, fd) \
+	dprintf(fd, "<backtrace not supported>\n", bt, sz)
+#endif
+
 static bool verbose(void)
 {
 	return env.verbosity > VERBOSE_NONE;
-- 
2.34.1

Re: [PATCH bpf-next v1 6/8] selftests/bpf: Fix compile if backtrace support missing in libc

[Andrii Nakryiko]

On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@gmail.com>
>
> Use backtrace functions only with glibc and otherwise provide stubs in
> test_progs.c. This avoids compile errors (e.g. with musl libc) like:
>
>   test_progs.c:13:10: fatal error: execinfo.h: No such file or directory
>      13 | #include <execinfo.h> /* backtrace */
>         |          ^~~~~~~~~~~~
>   test_progs.c: In function 'crash_handler':
>   test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration]
>    1034 |         sz = backtrace(bt, ARRAY_SIZE(bt));
>         |              ^~~~~~~~~
>   test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration]
>    1045 |         backtrace_symbols_fd(bt, sz, STDERR_FILENO);
>         |         ^~~~~~~~~~~~~~~~~~~~
>
> Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs")
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 60c5ec0f6abf..f6cfc6a8e8f0 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -10,7 +10,6 @@
>  #include <sched.h>
>  #include <signal.h>
>  #include <string.h>
> -#include <execinfo.h> /* backtrace */
>  #include <sys/sysinfo.h> /* get_nprocs */
>  #include <netinet/in.h>
>  #include <sys/select.h>
> @@ -19,6 +18,14 @@
>  #include <bpf/btf.h>
>  #include "json_writer.h"
>
> +#ifdef __GLIBC__
> +#include <execinfo.h> /* backtrace */
> +#else
> +#define backtrace(...) (0)
> +#define backtrace_symbols_fd(bt, sz, fd) \
> +       dprintf(fd, "<backtrace not supported>\n", bt, sz)
> +#endif

First, let's define backtrace() and backtrace_symbols_fd() as proper
functions, not a macro?

And second, what if we then make those functions __weak, so they
provide default implementations if libc doesn't provide those
functions?

This parts seems unavoidable, though:

#ifdef __GLIBC__
#include <execinfo.h>
#endif


> +
>  static bool verbose(void)
>  {
>         return env.verbosity > VERBOSE_NONE;
> --
> 2.34.1
>

Re: [PATCH bpf-next v1 6/8] selftests/bpf: Fix compile if backtrace support missing in libc

[Tony Ambardar]

On Thu, Jul 25, 2024 at 01:22:37PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@gmail.com>
> >
> > Use backtrace functions only with glibc and otherwise provide stubs in
> > test_progs.c. This avoids compile errors (e.g. with musl libc) like:
> >
> >   test_progs.c:13:10: fatal error: execinfo.h: No such file or directory
> >      13 | #include <execinfo.h> /* backtrace */
> >         |          ^~~~~~~~~~~~
> >   test_progs.c: In function 'crash_handler':
> >   test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration]
> >    1034 |         sz = backtrace(bt, ARRAY_SIZE(bt));
> >         |              ^~~~~~~~~
> >   test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration]
> >    1045 |         backtrace_symbols_fd(bt, sz, STDERR_FILENO);
> >         |         ^~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs")
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/test_progs.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 60c5ec0f6abf..f6cfc6a8e8f0 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -10,7 +10,6 @@
> >  #include <sched.h>
> >  #include <signal.h>
> >  #include <string.h>
> > -#include <execinfo.h> /* backtrace */
> >  #include <sys/sysinfo.h> /* get_nprocs */
> >  #include <netinet/in.h>
> >  #include <sys/select.h>
> > @@ -19,6 +18,14 @@
> >  #include <bpf/btf.h>
> >  #include "json_writer.h"
> >
> > +#ifdef __GLIBC__
> > +#include <execinfo.h> /* backtrace */
> > +#else
> > +#define backtrace(...) (0)
> > +#define backtrace_symbols_fd(bt, sz, fd) \
> > +       dprintf(fd, "<backtrace not supported>\n", bt, sz)
> > +#endif
> 
> First, let's define backtrace() and backtrace_symbols_fd() as proper
> functions, not a macro?
> 
> And second, what if we then make those functions __weak, so they
> provide default implementations if libc doesn't provide those
> functions?
> 
> This parts seems unavoidable, though:
> 
> #ifdef __GLIBC__
> #include <execinfo.h>
> #endif
> 

I agree that would be cleaner, will work on a v2 with this.

Out of curiosity, I saw that tools/build includes feature-detection code
(incl backtrace) and wondered if selftests/bpf ever used this facility?
> 
> > +
> >  static bool verbose(void)
> >  {
> >         return env.verbosity > VERBOSE_NONE;
> > --
> > 2.34.1
> >

Re: [PATCH bpf-next v1 6/8] selftests/bpf: Fix compile if backtrace support missing in libc

[Andrii Nakryiko]

On Fri, Jul 26, 2024 at 8:48 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> On Thu, Jul 25, 2024 at 01:22:37PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> > >
> > > From: Tony Ambardar <tony.ambardar@gmail.com>
> > >
> > > Use backtrace functions only with glibc and otherwise provide stubs in
> > > test_progs.c. This avoids compile errors (e.g. with musl libc) like:
> > >
> > >   test_progs.c:13:10: fatal error: execinfo.h: No such file or directory
> > >      13 | #include <execinfo.h> /* backtrace */
> > >         |          ^~~~~~~~~~~~
> > >   test_progs.c: In function 'crash_handler':
> > >   test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration]
> > >    1034 |         sz = backtrace(bt, ARRAY_SIZE(bt));
> > >         |              ^~~~~~~~~
> > >   test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration]
> > >    1045 |         backtrace_symbols_fd(bt, sz, STDERR_FILENO);
> > >         |         ^~~~~~~~~~~~~~~~~~~~
> > >
> > > Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs")
> > > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > > ---
> > >  tools/testing/selftests/bpf/test_progs.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index 60c5ec0f6abf..f6cfc6a8e8f0 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -10,7 +10,6 @@
> > >  #include <sched.h>
> > >  #include <signal.h>
> > >  #include <string.h>
> > > -#include <execinfo.h> /* backtrace */
> > >  #include <sys/sysinfo.h> /* get_nprocs */
> > >  #include <netinet/in.h>
> > >  #include <sys/select.h>
> > > @@ -19,6 +18,14 @@
> > >  #include <bpf/btf.h>
> > >  #include "json_writer.h"
> > >
> > > +#ifdef __GLIBC__
> > > +#include <execinfo.h> /* backtrace */
> > > +#else
> > > +#define backtrace(...) (0)
> > > +#define backtrace_symbols_fd(bt, sz, fd) \
> > > +       dprintf(fd, "<backtrace not supported>\n", bt, sz)
> > > +#endif
> >
> > First, let's define backtrace() and backtrace_symbols_fd() as proper
> > functions, not a macro?
> >
> > And second, what if we then make those functions __weak, so they
> > provide default implementations if libc doesn't provide those
> > functions?
> >
> > This parts seems unavoidable, though:
> >
> > #ifdef __GLIBC__
> > #include <execinfo.h>
> > #endif
> >
>
> I agree that would be cleaner, will work on a v2 with this.
>

v2 looks good, thanks

> Out of curiosity, I saw that tools/build includes feature-detection code
> (incl backtrace) and wondered if selftests/bpf ever used this facility?

I don't remember, tbh, it might have at some point in the past.

> >
> > > +
> > >  static bool verbose(void)
> > >  {
> > >         return env.verbosity > VERBOSE_NONE;
> > > --
> > > 2.34.1
> > >

[PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Typically stdin, stdout, stderr are treated as reserved identifiers under
ISO/ANSI C, and a libc implementation is free to define these as macros.
This is the case in musl libc and results in compile errors when these
names are reused as struct fields, as with 'struct test_env' and related
usage in test_progs.[ch] and reg_bounds.c.

Rename the fields to _stdout and _stderr to avoid many errors seen building
against musl, e.g.:

  In file included from test_progs.h:6,
                   from test_progs.c:5:
  test_progs.c: In function 'print_test_result':
  test_progs.c:237:21: error: expected identifier before '(' token
    237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
        |                     ^~~~~~
  test_progs.c:237:9: error: too few arguments to function 'fprintf'
    237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
        |         ^~~~~~~

Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
 tools/testing/selftests/bpf/test_progs.c      | 66 +++++++++----------
 tools/testing/selftests/bpf/test_progs.h      |  8 +--
 3 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
index 0da4225749bd..ff4ebc9eaf3f 100644
--- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
+++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
@@ -1487,7 +1487,7 @@ static int verify_case_opt(struct ctx *ctx, enum num_t init_t, enum num_t cond_t
 			u64 elapsed_ns = get_time_ns() - ctx->start_ns;
 			double remain_ns = elapsed_ns / progress * (1 - progress);
 
-			fprintf(env.stderr, "PROGRESS (%s): %d/%d (%.2lf%%), "
+			fprintf(env._stderr, "PROGRESS (%s): %d/%d (%.2lf%%), "
 					    "elapsed %llu mins (%.2lf hrs), "
 					    "ETA %.0lf mins (%.2lf hrs)\n",
 				ctx->progress_ctx,
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index f6cfc6a8e8f0..091b49bf671a 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -44,15 +44,15 @@ static void stdio_hijack_init(char **log_buf, size_t *log_cnt)
 
 	stdout = open_memstream(log_buf, log_cnt);
 	if (!stdout) {
-		stdout = env.stdout;
+		stdout = env._stdout;
 		perror("open_memstream");
 		return;
 	}
 
 	if (env.subtest_state)
-		env.subtest_state->stdout = stdout;
+		env.subtest_state->_stdout = stdout;
 	else
-		env.test_state->stdout = stdout;
+		env.test_state->_stdout = stdout;
 
 	stderr = stdout;
 #endif
@@ -66,8 +66,8 @@ static void stdio_hijack(char **log_buf, size_t *log_cnt)
 		return;
 	}
 
-	env.stdout = stdout;
-	env.stderr = stderr;
+	env._stdout = stdout;
+	env._stderr = stderr;
 
 	stdio_hijack_init(log_buf, log_cnt);
 #endif
@@ -84,13 +84,13 @@ static void stdio_restore_cleanup(void)
 	fflush(stdout);
 
 	if (env.subtest_state) {
-		fclose(env.subtest_state->stdout);
-		env.subtest_state->stdout = NULL;
-		stdout = env.test_state->stdout;
-		stderr = env.test_state->stdout;
+		fclose(env.subtest_state->_stdout);
+		env.subtest_state->_stdout = NULL;
+		stdout = env.test_state->_stdout;
+		stderr = env.test_state->_stdout;
 	} else {
-		fclose(env.test_state->stdout);
-		env.test_state->stdout = NULL;
+		fclose(env.test_state->_stdout);
+		env.test_state->_stdout = NULL;
 	}
 #endif
 }
@@ -103,13 +103,13 @@ static void stdio_restore(void)
 		return;
 	}
 
-	if (stdout == env.stdout)
+	if (stdout == env._stdout)
 		return;
 
 	stdio_restore_cleanup();
 
-	stdout = env.stdout;
-	stderr = env.stderr;
+	stdout = env._stdout;
+	stderr = env._stderr;
 #endif
 }
 
@@ -237,25 +237,25 @@ static void print_test_result(const struct prog_test_def *test, const struct tes
 	int skipped_cnt = test_state->skip_cnt;
 	int subtests_cnt = test_state->subtest_num;
 
-	fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
+	fprintf(env._stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
 	if (test_state->error_cnt)
-		fprintf(env.stdout, "FAIL");
+		fprintf(env._stdout, "FAIL");
 	else if (!skipped_cnt)
-		fprintf(env.stdout, "OK");
+		fprintf(env._stdout, "OK");
 	else if (skipped_cnt == subtests_cnt || !subtests_cnt)
-		fprintf(env.stdout, "SKIP");
+		fprintf(env._stdout, "SKIP");
 	else
-		fprintf(env.stdout, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt);
+		fprintf(env._stdout, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt);
 
-	fprintf(env.stdout, "\n");
+	fprintf(env._stdout, "\n");
 }
 
 static void print_test_log(char *log_buf, size_t log_cnt)
 {
 	log_buf[log_cnt] = '\0';
-	fprintf(env.stdout, "%s", log_buf);
+	fprintf(env._stdout, "%s", log_buf);
 	if (log_buf[log_cnt - 1] != '\n')
-		fprintf(env.stdout, "\n");
+		fprintf(env._stdout, "\n");
 }
 
 static void print_subtest_name(int test_num, int subtest_num,
@@ -266,14 +266,14 @@ static void print_subtest_name(int test_num, int subtest_num,
 
 	snprintf(test_num_str, sizeof(test_num_str), "%d/%d", test_num, subtest_num);
 
-	fprintf(env.stdout, "#%-*s %s/%s",
+	fprintf(env._stdout, "#%-*s %s/%s",
 		TEST_NUM_WIDTH, test_num_str,
 		test_name, subtest_name);
 
 	if (result)
-		fprintf(env.stdout, ":%s", result);
+		fprintf(env._stdout, ":%s", result);
 
-	fprintf(env.stdout, "\n");
+	fprintf(env._stdout, "\n");
 }
 
 static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
@@ -458,7 +458,7 @@ bool test__start_subtest(const char *subtest_name)
 	memset(subtest_state, 0, sub_state_size);
 
 	if (!subtest_name || !subtest_name[0]) {
-		fprintf(env.stderr,
+		fprintf(env._stderr,
 			"Subtest #%d didn't provide sub-test name!\n",
 			state->subtest_num);
 		return false;
@@ -466,7 +466,7 @@ bool test__start_subtest(const char *subtest_name)
 
 	subtest_state->name = strdup(subtest_name);
 	if (!subtest_state->name) {
-		fprintf(env.stderr,
+		fprintf(env._stderr,
 			"Subtest #%d: failed to copy subtest name!\n",
 			state->subtest_num);
 		return false;
@@ -1036,7 +1036,7 @@ void crash_handler(int signum)
 
 	sz = backtrace(bt, ARRAY_SIZE(bt));
 
-	if (env.stdout)
+	if (env._stdout)
 		stdio_restore();
 	if (env.test) {
 		env.test_state->error_cnt++;
@@ -1352,7 +1352,7 @@ static void calculate_summary_and_print_errors(struct test_env *env)
 	if (env->json) {
 		w = jsonw_new(env->json);
 		if (!w)
-			fprintf(env->stderr, "Failed to create new JSON stream.");
+			fprintf(env->_stderr, "Failed to create new JSON stream.");
 	}
 
 	if (w) {
@@ -1701,8 +1701,8 @@ int main(int argc, char **argv)
 		return -1;
 	}
 
-	env.stdout = stdout;
-	env.stderr = stderr;
+	env._stdout = stdout;
+	env._stderr = stderr;
 
 	env.has_testmod = true;
 	if (!env.list_test_names) {
@@ -1710,7 +1710,7 @@ int main(int argc, char **argv)
 		unload_bpf_testmod(verbose());
 
 		if (load_bpf_testmod(verbose())) {
-			fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
+			fprintf(env._stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
 			env.has_testmod = false;
 		}
 	}
@@ -1788,7 +1788,7 @@ int main(int argc, char **argv)
 		}
 
 		if (env.list_test_names) {
-			fprintf(env.stdout, "%s\n", test->test_name);
+			fprintf(env._stdout, "%s\n", test->test_name);
 			env.succ_cnt++;
 			continue;
 		}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index b1e949fb16cf..f42f1ae59c6e 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -75,7 +75,7 @@ struct subtest_state {
 	bool skipped;
 	bool filtered;
 
-	FILE *stdout;
+	FILE *_stdout;
 };
 
 struct test_state {
@@ -92,7 +92,7 @@ struct test_state {
 	size_t log_cnt;
 	char *log_buf;
 
-	FILE *stdout;
+	FILE *_stdout;
 };
 
 struct test_env {
@@ -111,8 +111,8 @@ struct test_env {
 	struct test_state *test_state; /* current running test state */
 	struct subtest_state *subtest_state; /* current running subtest state */
 
-	FILE *stdout;
-	FILE *stderr;
+	FILE *_stdout;
+	FILE *_stderr;
 	int nr_cpus;
 	FILE *json;
 
-- 
2.34.1

Re: [PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

[Andrii Nakryiko]

On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> From: Tony Ambardar <tony.ambardar@gmail.com>
>
> Typically stdin, stdout, stderr are treated as reserved identifiers under
> ISO/ANSI C, and a libc implementation is free to define these as macros.

Ok, wow that. Do you have a pointer to where in the standard it is
said that stdin/stdout/stderr is some sort of reserved identifier that
can't be used as a field name?


I really don't like these underscored field names. If we have to
rename, I'd prefer something like env.saved_stdout instead of
env._stdout. But I'd prefer even more if musl wasn't doing this macro
definition, of course...

> This is the case in musl libc and results in compile errors when these
> names are reused as struct fields, as with 'struct test_env' and related
> usage in test_progs.[ch] and reg_bounds.c.
>
> Rename the fields to _stdout and _stderr to avoid many errors seen building
> against musl, e.g.:
>
>   In file included from test_progs.h:6,
>                    from test_progs.c:5:
>   test_progs.c: In function 'print_test_result':
>   test_progs.c:237:21: error: expected identifier before '(' token
>     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
>         |                     ^~~~~~
>   test_progs.c:237:9: error: too few arguments to function 'fprintf'
>     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
>         |         ^~~~~~~
>
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
>  tools/testing/selftests/bpf/test_progs.c      | 66 +++++++++----------
>  tools/testing/selftests/bpf/test_progs.h      |  8 +--
>  3 files changed, 38 insertions(+), 38 deletions(-)
>

[...]

Re: [PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

[Tony Ambardar]

On Thu, Jul 25, 2024 at 01:27:03PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@gmail.com>
> >
> > Typically stdin, stdout, stderr are treated as reserved identifiers under
> > ISO/ANSI C, and a libc implementation is free to define these as macros.
> 
> Ok, wow that. Do you have a pointer to where in the standard it is
> said that stdin/stdout/stderr is some sort of reserved identifier that
> can't be used as a field name?
> 

I'll need to dig around to share some references. The short answer IIRC
is there's enough potential variation in their definitions that their
use requires care (or better avoidance).

> 
> I really don't like these underscored field names. If we have to
> rename, I'd prefer something like env.saved_stdout instead of
> env._stdout. But I'd prefer even more if musl wasn't doing this macro
> definition, of course...

OK, I'll use clearer names for a v2.

I believe the macro definitions are quite common and old, but "how"
makes a difference: specifically, using parenthesis happens to break our
.stdxxx field names.


In glibc <stdio.h> we have for example:
...
/* Standard streams.  */
extern FILE *stdin;             /* Standard input stream.  */
extern FILE *stdout;            /* Standard output stream.  */
extern FILE *stderr;            /* Standard error output stream.  */
/* C89/C99 say they're macros.  Make them happy.  */
#define stdin stdin
#define stdout stdout
#define stderr stderr
...

while in musl <stdio.h> we have:
...
extern FILE *const stdin;
extern FILE *const stdout;
extern FILE *const stderr;

#define stdin  (stdin)
#define stdout (stdout)
#define stderr (stderr)
...

which borks code in test_progs.c:
...
env.stderr = stderr;
env.stdout = stdout;
...


> 
> > This is the case in musl libc and results in compile errors when these
> > names are reused as struct fields, as with 'struct test_env' and related
> > usage in test_progs.[ch] and reg_bounds.c.
> >
> > Rename the fields to _stdout and _stderr to avoid many errors seen building
> > against musl, e.g.:
> >
> >   In file included from test_progs.h:6,
> >                    from test_progs.c:5:
> >   test_progs.c: In function 'print_test_result':
> >   test_progs.c:237:21: error: expected identifier before '(' token
> >     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
> >         |                     ^~~~~~
> >   test_progs.c:237:9: error: too few arguments to function 'fprintf'
> >     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
> >         |         ^~~~~~~
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
> >  tools/testing/selftests/bpf/test_progs.c      | 66 +++++++++----------
> >  tools/testing/selftests/bpf/test_progs.h      |  8 +--
> >  3 files changed, 38 insertions(+), 38 deletions(-)
> >
> 
> [...]

Re: [PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

[Tony Ambardar]

On Fri, Jul 26, 2024 at 09:22:38PM -0700, Tony Ambardar wrote:
> On Thu, Jul 25, 2024 at 01:27:03PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> > >
> > > From: Tony Ambardar <tony.ambardar@gmail.com>
> > >
> > > Typically stdin, stdout, stderr are treated as reserved identifiers under
> > > ISO/ANSI C, and a libc implementation is free to define these as macros.
> > 
> > Ok, wow that. Do you have a pointer to where in the standard it is
> > said that stdin/stdout/stderr is some sort of reserved identifier that
> > can't be used as a field name?
> > 
> 
> I'll need to dig around to share some references. The short answer IIRC
> is there's enough potential variation in their definitions that their
> use requires care (or better avoidance).
> 

Hi Andrii,

Following up on your request for pointers, some excerpts from a quasi-draft
C17 ISO doc located here:
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf


7.1.2 Standard headers
	(2) The standard headers are ... <stdio.h> ...
	(5) Any definition of an object-like macro ... shall expand to
	code that is fully protected by parentheses ...

7.1.3 Reserved identifiers
	(1) ... Each macro name in any of the following subclauses ...
	is reserved for use as specified if any of its associated headers
	is included ...

7.21.1 Input/output <stdio.h>, Introduction
	(1) The header <stdio.h> defines several macros ...
	(3) The macros are ... stderr stdin stdout which are expressions
	of type "pointer to FILE" ...

7.21.5.4 The freopen function
	(2) (Footnote 278) The primary use of the freopen function is to
	change the file associated with a standard text stream (stderr,
	stdin, or stdout), as those identifiers need not be modifiable
	lvalues ...


So we have reserved idents (IANALL so not sure of field names), macros,
parentheses, and potentially unassignable stdout/stderr that might break
the output redirection hack in test_progs.c. More than enough to tread
carefully I think... 

Cheers,
Tony

[PATCH bpf-next v1 8/8] selftests/bpf: Fix error compiling tc_redirect.c with musl libc

[Tony Ambardar]

From: Tony Ambardar <tony.ambardar@gmail.com>

Linux 5.1 implemented 64-bit time types and related syscalls to address the
Y2038 problem generally across archs. Userspace handling of Y2038 varies
with the libc however. While musl libc uses 64-bit time across all 32-bit
and 64-bit platforms, GNU glibc uses 64-bit time on 64-bit platforms but
defaults to 32-bit time on 32-bit platforms unless they "opt-in" to 64-bit
time or explicitly use 64-bit syscalls and time structures.

One specific area is the standard setsockopt() call, SO_TIMESTAMPNS option
used for timestamping, and the related output 'struct timespec'. GNU glibc
defaults as above, also exposing the SO_TIMESTAMPNS_NEW flag to explicitly
use a 64-bit call and 'struct __kernel_timespec'. Since these are not
exposed or needed with musl libc, their use in tc_redirect.c leads to
compile errors building for mips64el/musl:

  tc_redirect.c: In function 'rcv_tstamp':
  tc_redirect.c:425:32: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'?
    425 |             cmsg->cmsg_type == SO_TIMESTAMPNS_NEW)
        |                                ^~~~~~~~~~~~~~~~~~
        |                                SO_TIMESTAMPNS
  tc_redirect.c:425:32: note: each undeclared identifier is reported only once for each function it appears in
  tc_redirect.c: In function 'test_inet_dtime':
  tc_redirect.c:491:49: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'?
    491 |         err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
        |                                                 ^~~~~~~~~~~~~~~~~~
        |                                                 SO_TIMESTAMPNS

However, using SO_TIMESTAMPNS_NEW isn't strictly needed, nor is Y2038 being
explicitly tested. The timestamp checks in tc_redirect.c are simple: the
packet receive timestamp is non-zero and processed/handled in less than 5
seconds.

Switch to using the standard setsockopt() call and SO_TIMESTAMPNS option to
ensure compatibility across glibc and musl libc. In the worst-case, there
is a 5-second window 14 years from now where tc_redirect tests may fail on
32-bit systems. However, we should reasonably expect glibc to adopt a
64-bit mandate rather than the current "opt-in" policy before the Y2038
roll-over.

Fixes: ce6f6cffaeaa ("selftests/bpf: Wait for the netstamp_needed_key static key to be turned on")
Fixes: c803475fd8dd ("bpf: selftests: test skb->tstamp in redirect_neigh")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index 327d51f59142..53b8ffc943dc 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
@@ -471,7 +471,7 @@ static int set_forwarding(bool enable)
 
 static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp)
 {
-	struct __kernel_timespec pkt_ts = {};
+	struct timespec pkt_ts = {};
 	char ctl[CMSG_SPACE(sizeof(pkt_ts))];
 	struct timespec now_ts;
 	struct msghdr msg = {};
@@ -495,7 +495,7 @@ static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp)
 
 	cmsg = CMSG_FIRSTHDR(&msg);
 	if (cmsg && cmsg->cmsg_level == SOL_SOCKET &&
-	    cmsg->cmsg_type == SO_TIMESTAMPNS_NEW)
+	    cmsg->cmsg_type == SO_TIMESTAMPNS)
 		memcpy(&pkt_ts, CMSG_DATA(cmsg), sizeof(pkt_ts));
 
 	pkt_ns = pkt_ts.tv_sec * NSEC_PER_SEC + pkt_ts.tv_nsec;
@@ -537,9 +537,9 @@ static int wait_netstamp_needed_key(void)
 	if (!ASSERT_GE(srv_fd, 0, "start_server"))
 		goto done;
 
-	err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
+	err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS,
 			 &opt, sizeof(opt));
-	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)"))
+	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)"))
 		goto done;
 
 	cli_fd = connect_to_fd(srv_fd, TIMEOUT_MILLIS);
@@ -621,9 +621,9 @@ static void test_inet_dtime(int family, int type, const char *addr, __u16 port)
 		return;
 
 	/* Ensure the kernel puts the (rcv) timestamp for all skb */
-	err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
+	err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS,
 			 &opt, sizeof(opt));
-	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)"))
+	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)"))
 		goto done;
 
 	if (type == SOCK_STREAM) {
-- 
2.34.1

[PATCH bpf-next v2 0/8] selftests/bpf: Improve libc portability / musl support (part 2)

[Tony Ambardar]

Hello all,

This is part 2 of a series of fixes for libc-related issues encountered
building for musl-based systems. The series has been tested with the
kernel-patches/bpf CI and locally using mips64el-gcc/musl-libc and QEMU
with an OpenWrt rootfs.

The patches cover a few areas of portability issues:

 - problematic libc usage (strtok_r(), stdio macros/reserved identifiers)
 - gcc compile errors (include header ordering, sequence-point errors)
 - POSIX vs GNU basename()
 - missing GNU extensions (<execinfo.h>, C++ <stdbool.h>)
 - Y2038 and setsockopt() / SO_TIMESTAMPNS_NEW

Feedback and suggestions are appreciated!

Thanks,
Tony


Changelog:
----------

v1->v2: (feedback from Andrii)
 - P2: rewrite simpler code using counter
 - P5: update description/fix after more research
 - P6: use weak functions for backtrace stubs
 - P7: use stdxxx_saved names, update desc



Tony Ambardar (8):
  selftests/bpf: Use portable POSIX basename()
  selftests/bpf: Fix arg parsing in veristat, test_progs
  selftests/bpf: Fix error compiling test_lru_map.c
  selftests/bpf: Fix C++ compile error from missing _Bool type
  selftests/bpf: Fix redefinition errors compiling lwt_reroute.c
  selftests/bpf: Fix compile if backtrace support missing in libc
  selftests/bpf: Fix using stdout, stderr as struct field names
  selftests/bpf: Fix error compiling tc_redirect.c with musl libc

 .../selftests/bpf/prog_tests/lwt_reroute.c    |  1 +
 .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
 .../selftests/bpf/prog_tests/tc_redirect.c    | 12 +--
 tools/testing/selftests/bpf/test_cpp.cpp      |  4 +
 tools/testing/selftests/bpf/test_lru_map.c    |  3 +-
 tools/testing/selftests/bpf/test_progs.c      | 82 +++++++++++--------
 tools/testing/selftests/bpf/test_progs.h      |  8 +-
 tools/testing/selftests/bpf/testing_helpers.c |  4 +-
 tools/testing/selftests/bpf/veristat.c        | 16 ++--
 tools/testing/selftests/bpf/xskxceiver.c      |  1 +
 10 files changed, 78 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH bpf-next v2 1/8] selftests/bpf: Use portable POSIX basename()

[Tony Ambardar]

Use the POSIX version of basename() to allow compilation against non-gnu
libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using
functions from the latter while preferring POSIX over GNU basename().

In veristat.c, rely on strdupa() to avoid basename() altering the passed
"const char" argument. This is not needed in xskxceiver.c since the arg
is mutable and the program exits immediately after usage.

Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/veristat.c   | 8 +++++---
 tools/testing/selftests/bpf/xskxceiver.c | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index b2854238d4a0..11ec1190d582 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
 #define _GNU_SOURCE
 #include <argp.h>
+#include <libgen.h>
 #include <string.h>
 #include <stdlib.h>
 #include <sched.h>
@@ -988,8 +989,8 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
 
 static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
 {
+	const char *base_filename = basename(strdupa(filename));
 	const char *prog_name = bpf_program__name(prog);
-	const char *base_filename = basename(filename);
 	char *buf;
 	int buf_sz, log_level;
 	struct verif_stats *stats;
@@ -1056,13 +1057,14 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 
 static int process_obj(const char *filename)
 {
+	const char *base_filename = basename(strdupa(filename));
 	struct bpf_object *obj = NULL, *tobj;
 	struct bpf_program *prog, *tprog, *lprog;
 	libbpf_print_fn_t old_libbpf_print_fn;
 	LIBBPF_OPTS(bpf_object_open_opts, opts);
 	int err = 0, prog_cnt = 0;
 
-	if (!should_process_file_prog(basename(filename), NULL)) {
+	if (!should_process_file_prog(base_filename, NULL)) {
 		if (env.verbose)
 			printf("Skipping '%s' due to filters...\n", filename);
 		env.files_skipped++;
@@ -1076,7 +1078,7 @@ static int process_obj(const char *filename)
 	}
 
 	if (!env.quiet && env.out_fmt == RESFMT_TABLE)
-		printf("Processing '%s'...\n", basename(filename));
+		printf("Processing '%s'...\n", base_filename);
 
 	old_libbpf_print_fn = libbpf_set_print(libbpf_print_fn);
 	obj = bpf_object__open_file(filename, &opts);
diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 8144fd145237..92af633faea8 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -90,6 +90,7 @@
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <libgen.h>
 #include <string.h>
 #include <stddef.h>
 #include <sys/mman.h>
-- 
2.34.1

Re: [PATCH bpf-next v2 1/8] selftests/bpf: Use portable POSIX basename()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Mon, 29 Jul 2024 02:24:17 -0700 you wrote:
> Use the POSIX version of basename() to allow compilation against non-gnu
> libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using
> functions from the latter while preferring POSIX over GNU basename().
> 
> In veristat.c, rely on strdupa() to avoid basename() altering the passed
> "const char" argument. This is not needed in xskxceiver.c since the arg
> is mutable and the program exits immediately after usage.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/8] selftests/bpf: Use portable POSIX basename()
    https://git.kernel.org/bpf/bpf-next/c/c0247800ee7d
  - [bpf-next,v2,2/8] selftests/bpf: Fix arg parsing in veristat, test_progs
    https://git.kernel.org/bpf/bpf-next/c/03bfcda1fbc3
  - [bpf-next,v2,3/8] selftests/bpf: Fix error compiling test_lru_map.c
    https://git.kernel.org/bpf/bpf-next/c/cacf2a5a78cd
  - [bpf-next,v2,4/8] selftests/bpf: Fix C++ compile error from missing _Bool type
    https://git.kernel.org/bpf/bpf-next/c/aa95073fd290
  - [bpf-next,v2,5/8] selftests/bpf: Fix redefinition errors compiling lwt_reroute.c
    https://git.kernel.org/bpf/bpf-next/c/16b795cc5952
  - [bpf-next,v2,6/8] selftests/bpf: Fix compile if backtrace support missing in libc
    https://git.kernel.org/bpf/bpf-next/c/c9a83e76b5a9
  - [bpf-next,v2,7/8] selftests/bpf: Fix using stdout, stderr as struct field names
    https://git.kernel.org/bpf/bpf-next/c/06eeca1217a8
  - [bpf-next,v2,8/8] selftests/bpf: Fix error compiling tc_redirect.c with musl libc
    https://git.kernel.org/bpf/bpf-next/c/21c5f4f55da7

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH bpf-next v2 2/8] selftests/bpf: Fix arg parsing in veristat, test_progs

[Tony Ambardar]

Current code parses arguments with strtok_r() using a construct like

    char *state = NULL;
    while ((next = strtok_r(state ? NULL : input, ",", &state))) {
        ...
    }

where logic assumes the 'state' var can distinguish between first and
subsequent strtok_r() calls, and adjusts parameters accordingly. However,
'state' is strictly internal context for strtok_r() and no such assumptions
are supported in the man page. Moreover, the exact behaviour of 'state'
depends on the libc implementation, making the above code fragile.

Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang,
with the above code in an infinite loop.

Similarly, we see strange behaviour running 'veristat' on mips64el/musl:

    $ ./veristat -e file,prog,verdict,insns -C two-ok add-failure
    Can't specify more than 9 stats

Rewrite code using a counter to distinguish between strtok_r() calls.

Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests")
Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat")
Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/testing_helpers.c | 4 ++--
 tools/testing/selftests/bpf/veristat.c        | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index ac7c66f4fc7b..c217e12bd9da 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -221,13 +221,13 @@ int parse_test_list(const char *s,
 		    bool is_glob_pattern)
 {
 	char *input, *state = NULL, *test_spec;
-	int err = 0;
+	int err = 0, cnt = 0;
 
 	input = strdup(s);
 	if (!input)
 		return -ENOMEM;
 
-	while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
+	while ((test_spec = strtok_r(cnt++ ? NULL : input, ",", &state))) {
 		err = insert_test(set, test_spec, is_glob_pattern);
 		if (err)
 			break;
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 11ec1190d582..1ec5c4c47235 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -785,13 +785,13 @@ static int parse_stat(const char *stat_name, struct stat_specs *specs)
 static int parse_stats(const char *stats_str, struct stat_specs *specs)
 {
 	char *input, *state = NULL, *next;
-	int err;
+	int err, cnt = 0;
 
 	input = strdup(stats_str);
 	if (!input)
 		return -ENOMEM;
 
-	while ((next = strtok_r(state ? NULL : input, ",", &state))) {
+	while ((next = strtok_r(cnt++ ? NULL : input, ",", &state))) {
 		err = parse_stat(next, specs);
 		if (err) {
 			free(input);
@@ -1495,7 +1495,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs,
 	while (fgets(line, sizeof(line), f)) {
 		char *input = line, *state = NULL, *next;
 		struct verif_stats *st = NULL;
-		int col = 0;
+		int col = 0, cnt = 0;
 
 		if (!header) {
 			void *tmp;
@@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs,
 			*stat_cntp += 1;
 		}
 
-		while ((next = strtok_r(state ? NULL : input, ",\n", &state))) {
+		while ((next = strtok_r(cnt++ ? NULL : input, ",\n", &state))) {
 			if (header) {
 				/* for the first line, set up spec stats */
 				err = parse_stat(next, specs);
-- 
2.34.1

[PATCH bpf-next v2 3/8] selftests/bpf: Fix error compiling test_lru_map.c

[Tony Ambardar]

Although the post-increment in macro 'CPU_SET(next++, &cpuset)' seems safe,
the sequencing can raise compile errors, so move the increment outside the
macro. This avoids an error seen using gcc 12.3.0 for mips64el/musl-libc:

  In file included from test_lru_map.c:11:
  test_lru_map.c: In function 'sched_next_online':
  test_lru_map.c:129:29: error: operation on 'next' may be undefined [-Werror=sequence-point]
    129 |                 CPU_SET(next++, &cpuset);
        |                             ^
  cc1: all warnings being treated as errors

Fixes: 3fbfadce6012 ("bpf: Fix test_lru_sanity5() in test_lru_map.c")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/test_lru_map.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_lru_map.c b/tools/testing/selftests/bpf/test_lru_map.c
index 4d0650cfb5cd..fda7589c5023 100644
--- a/tools/testing/selftests/bpf/test_lru_map.c
+++ b/tools/testing/selftests/bpf/test_lru_map.c
@@ -126,7 +126,8 @@ static int sched_next_online(int pid, int *next_to_try)
 
 	while (next < nr_cpus) {
 		CPU_ZERO(&cpuset);
-		CPU_SET(next++, &cpuset);
+		CPU_SET(next, &cpuset);
+		next++;
 		if (!sched_setaffinity(pid, sizeof(cpuset), &cpuset)) {
 			ret = 0;
 			break;
-- 
2.34.1

[PATCH bpf-next v2 4/8] selftests/bpf: Fix C++ compile error from missing _Bool type

[Tony Ambardar]

While building, bpftool makes a skeleton from test_core_extern.c, which
itself includes <stdbool.h> and uses the 'bool' type. However, the skeleton
test_core_extern.skel.h generated *does not* include <stdbool.h> or use the
'bool' type, instead using the C-only '_Bool' type. Compiling test_cpp.cpp
with g++ 12.3 for mips64el/musl-libc then fails with error:

  In file included from test_cpp.cpp:9:
  test_core_extern.skel.h:45:17: error: '_Bool' does not name a type
     45 |                 _Bool CONFIG_BOOL;
        |                 ^~~~~

This was likely missed previously because glibc uses a GNU extension for
<stdbool.h> with C++ (#define _Bool bool), not supported by musl libc.

Normally, a C fragment would include <stdbool.h> and use the 'bool' type,
and thus cleanly work after import by C++. The ideal fix would be for
'bpftool gen skeleton' to output the correct type/include supporting C++,
but in the meantime add a conditional define as above.

Fixes: 7c8dce4b1661 ("bpftool: Make skeleton C code compilable with C++ compiler")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/test_cpp.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_cpp.cpp b/tools/testing/selftests/bpf/test_cpp.cpp
index dde0bb16e782..abc2a56ab261 100644
--- a/tools/testing/selftests/bpf/test_cpp.cpp
+++ b/tools/testing/selftests/bpf/test_cpp.cpp
@@ -6,6 +6,10 @@
 #include <bpf/libbpf.h>
 #include <bpf/bpf.h>
 #include <bpf/btf.h>
+
+#ifndef _Bool
+#define _Bool bool
+#endif
 #include "test_core_extern.skel.h"
 #include "struct_ops_module.skel.h"
 
-- 
2.34.1

[PATCH bpf-next v2 5/8] selftests/bpf: Fix redefinition errors compiling lwt_reroute.c

[Tony Ambardar]

Compiling lwt_reroute.c with GCC 12.3 for mips64el/musl-libc yields errors:

In file included from .../include/arpa/inet.h:9,
                 from ./test_progs.h:18,
                 from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11,
                 from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
.../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
   23 | struct in6_addr {
      |        ^~~~~~~~
In file included from .../include/linux/icmp.h:24,
                 from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9:
.../include/linux/in6.h:33:8: note: originally defined here
   33 | struct in6_addr {
      |        ^~~~~~~~
.../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
   34 | struct sockaddr_in6 {
      |        ^~~~~~~~~~~~
.../include/linux/in6.h:50:8: note: originally defined here
   50 | struct sockaddr_in6 {
      |        ^~~~~~~~~~~~
.../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
   42 | struct ipv6_mreq {
      |        ^~~~~~~~~
.../include/linux/in6.h:60:8: note: originally defined here
   60 | struct ipv6_mreq {
      |        ^~~~~~~~~

These errors occur because <linux/in6.h> is included before <netinet/in.h>,
bypassing the Linux uapi/libc compat mechanism's partial musl support. As
described in [1] and [2], fix these errors by including <netinet/in.h> in
lwt_reroute.c before any uapi headers.

[1]: commit c0bace798436 ("uapi libc compat: add fallback for unsupported libcs")
[2]: https://git.musl-libc.org/cgit/musl/commit/?id=04983f227238

Fixes: 6c77997bc639 ("selftests/bpf: Add lwt_xmit tests for BPF_REROUTE")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/lwt_reroute.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c b/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c
index 03825d2b45a8..6c50c0f63f43 100644
--- a/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c
+++ b/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c
@@ -49,6 +49,7 @@
  *  is not crashed, it is considered successful.
  */
 #define NETNS "ns_lwt_reroute"
+#include <netinet/in.h>
 #include "lwt_helpers.h"
 #include "network_helpers.h"
 #include <linux/net_tstamp.h>
-- 
2.34.1

[PATCH bpf-next v2 6/8] selftests/bpf: Fix compile if backtrace support missing in libc

[Tony Ambardar]

Include GNU <execinfo.h> header only with glibc and provide weak, stubbed
backtrace functions as a fallback in test_progs.c. This allows for non-GNU
replacements while avoiding compile errors (e.g. with musl libc) like:

  test_progs.c:13:10: fatal error: execinfo.h: No such file or directory
     13 | #include <execinfo.h> /* backtrace */
        |          ^~~~~~~~~~~~
  test_progs.c: In function 'crash_handler':
  test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration]
   1034 |         sz = backtrace(bt, ARRAY_SIZE(bt));
        |              ^~~~~~~~~
  test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration]
   1045 |         backtrace_symbols_fd(bt, sz, STDERR_FILENO);
        |         ^~~~~~~~~~~~~~~~~~~~

Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/test_progs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 60c5ec0f6abf..d5d0cb4eb197 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -10,7 +10,6 @@
 #include <sched.h>
 #include <signal.h>
 #include <string.h>
-#include <execinfo.h> /* backtrace */
 #include <sys/sysinfo.h> /* get_nprocs */
 #include <netinet/in.h>
 #include <sys/select.h>
@@ -19,6 +18,21 @@
 #include <bpf/btf.h>
 #include "json_writer.h"
 
+#ifdef __GLIBC__
+#include <execinfo.h> /* backtrace */
+#endif
+
+/* Default backtrace funcs if missing at link */
+__weak int backtrace(void **buffer, int size)
+{
+	return 0;
+}
+
+__weak void backtrace_symbols_fd(void *const *buffer, int size, int fd)
+{
+	dprintf(fd, "<backtrace not supported>\n");
+}
+
 static bool verbose(void)
 {
 	return env.verbosity > VERBOSE_NONE;
-- 
2.34.1

[PATCH bpf-next v2 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

[Tony Ambardar]

Typically stdin, stdout, stderr are treated as reserved identifiers under
ISO/ANSI C and libc implementations further define these as macros, both in
glibc and musl <stdio.h>.

However, while glibc defines:
    ...
    /* Standard streams.  */
    extern FILE *stdin;             /* Standard input stream.  */
    extern FILE *stdout;            /* Standard output stream.  */
    extern FILE *stderr;            /* Standard error output stream.  */
    /* C89/C99 say they're macros.  Make them happy.  */
    #define stdin stdin
    #define stdout stdout
    #define stderr stderr
    ...

musl instead uses (legally):
    ...
    extern FILE *const stdin;
    extern FILE *const stdout;
    extern FILE *const stderr;

    #define stdin  (stdin)
    #define stdout (stdout)
    #define stderr (stderr)
    ...

The latter results in compile errors when the names are reused as fields of
'struct test_env' and elsewhere in test_progs.[ch] and reg_bounds.c.

Rename the fields to stdout_saved and stderr_saved to avoid many errors
seen building against musl, e.g.:

  In file included from test_progs.h:6,
                   from test_progs.c:5:
  test_progs.c: In function 'print_test_result':
  test_progs.c:237:21: error: expected identifier before '(' token
    237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
        |                     ^~~~~~
  test_progs.c:237:9: error: too few arguments to function 'fprintf'
    237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
        |         ^~~~~~~

Link: https://lore.kernel.org/bpf/ZqR2DuHdBXPX%2Fyx8@kodidev-ubuntu/
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
 tools/testing/selftests/bpf/test_progs.c      | 66 +++++++++----------
 tools/testing/selftests/bpf/test_progs.h      |  8 +--
 3 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
index 0da4225749bd..467027236d30 100644
--- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
+++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
@@ -1487,7 +1487,7 @@ static int verify_case_opt(struct ctx *ctx, enum num_t init_t, enum num_t cond_t
 			u64 elapsed_ns = get_time_ns() - ctx->start_ns;
 			double remain_ns = elapsed_ns / progress * (1 - progress);
 
-			fprintf(env.stderr, "PROGRESS (%s): %d/%d (%.2lf%%), "
+			fprintf(env.stderr_saved, "PROGRESS (%s): %d/%d (%.2lf%%), "
 					    "elapsed %llu mins (%.2lf hrs), "
 					    "ETA %.0lf mins (%.2lf hrs)\n",
 				ctx->progress_ctx,
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index d5d0cb4eb197..60fafa2f1ed7 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -51,15 +51,15 @@ static void stdio_hijack_init(char **log_buf, size_t *log_cnt)
 
 	stdout = open_memstream(log_buf, log_cnt);
 	if (!stdout) {
-		stdout = env.stdout;
+		stdout = env.stdout_saved;
 		perror("open_memstream");
 		return;
 	}
 
 	if (env.subtest_state)
-		env.subtest_state->stdout = stdout;
+		env.subtest_state->stdout_saved = stdout;
 	else
-		env.test_state->stdout = stdout;
+		env.test_state->stdout_saved = stdout;
 
 	stderr = stdout;
 #endif
@@ -73,8 +73,8 @@ static void stdio_hijack(char **log_buf, size_t *log_cnt)
 		return;
 	}
 
-	env.stdout = stdout;
-	env.stderr = stderr;
+	env.stdout_saved = stdout;
+	env.stderr_saved = stderr;
 
 	stdio_hijack_init(log_buf, log_cnt);
 #endif
@@ -91,13 +91,13 @@ static void stdio_restore_cleanup(void)
 	fflush(stdout);
 
 	if (env.subtest_state) {
-		fclose(env.subtest_state->stdout);
-		env.subtest_state->stdout = NULL;
-		stdout = env.test_state->stdout;
-		stderr = env.test_state->stdout;
+		fclose(env.subtest_state->stdout_saved);
+		env.subtest_state->stdout_saved = NULL;
+		stdout = env.test_state->stdout_saved;
+		stderr = env.test_state->stdout_saved;
 	} else {
-		fclose(env.test_state->stdout);
-		env.test_state->stdout = NULL;
+		fclose(env.test_state->stdout_saved);
+		env.test_state->stdout_saved = NULL;
 	}
 #endif
 }
@@ -110,13 +110,13 @@ static void stdio_restore(void)
 		return;
 	}
 
-	if (stdout == env.stdout)
+	if (stdout == env.stdout_saved)
 		return;
 
 	stdio_restore_cleanup();
 
-	stdout = env.stdout;
-	stderr = env.stderr;
+	stdout = env.stdout_saved;
+	stderr = env.stderr_saved;
 #endif
 }
 
@@ -244,25 +244,25 @@ static void print_test_result(const struct prog_test_def *test, const struct tes
 	int skipped_cnt = test_state->skip_cnt;
 	int subtests_cnt = test_state->subtest_num;
 
-	fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
+	fprintf(env.stdout_saved, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
 	if (test_state->error_cnt)
-		fprintf(env.stdout, "FAIL");
+		fprintf(env.stdout_saved, "FAIL");
 	else if (!skipped_cnt)
-		fprintf(env.stdout, "OK");
+		fprintf(env.stdout_saved, "OK");
 	else if (skipped_cnt == subtests_cnt || !subtests_cnt)
-		fprintf(env.stdout, "SKIP");
+		fprintf(env.stdout_saved, "SKIP");
 	else
-		fprintf(env.stdout, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt);
+		fprintf(env.stdout_saved, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt);
 
-	fprintf(env.stdout, "\n");
+	fprintf(env.stdout_saved, "\n");
 }
 
 static void print_test_log(char *log_buf, size_t log_cnt)
 {
 	log_buf[log_cnt] = '\0';
-	fprintf(env.stdout, "%s", log_buf);
+	fprintf(env.stdout_saved, "%s", log_buf);
 	if (log_buf[log_cnt - 1] != '\n')
-		fprintf(env.stdout, "\n");
+		fprintf(env.stdout_saved, "\n");
 }
 
 static void print_subtest_name(int test_num, int subtest_num,
@@ -273,14 +273,14 @@ static void print_subtest_name(int test_num, int subtest_num,
 
 	snprintf(test_num_str, sizeof(test_num_str), "%d/%d", test_num, subtest_num);
 
-	fprintf(env.stdout, "#%-*s %s/%s",
+	fprintf(env.stdout_saved, "#%-*s %s/%s",
 		TEST_NUM_WIDTH, test_num_str,
 		test_name, subtest_name);
 
 	if (result)
-		fprintf(env.stdout, ":%s", result);
+		fprintf(env.stdout_saved, ":%s", result);
 
-	fprintf(env.stdout, "\n");
+	fprintf(env.stdout_saved, "\n");
 }
 
 static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt)
@@ -465,7 +465,7 @@ bool test__start_subtest(const char *subtest_name)
 	memset(subtest_state, 0, sub_state_size);
 
 	if (!subtest_name || !subtest_name[0]) {
-		fprintf(env.stderr,
+		fprintf(env.stderr_saved,
 			"Subtest #%d didn't provide sub-test name!\n",
 			state->subtest_num);
 		return false;
@@ -473,7 +473,7 @@ bool test__start_subtest(const char *subtest_name)
 
 	subtest_state->name = strdup(subtest_name);
 	if (!subtest_state->name) {
-		fprintf(env.stderr,
+		fprintf(env.stderr_saved,
 			"Subtest #%d: failed to copy subtest name!\n",
 			state->subtest_num);
 		return false;
@@ -1043,7 +1043,7 @@ void crash_handler(int signum)
 
 	sz = backtrace(bt, ARRAY_SIZE(bt));
 
-	if (env.stdout)
+	if (env.stdout_saved)
 		stdio_restore();
 	if (env.test) {
 		env.test_state->error_cnt++;
@@ -1359,7 +1359,7 @@ static void calculate_summary_and_print_errors(struct test_env *env)
 	if (env->json) {
 		w = jsonw_new(env->json);
 		if (!w)
-			fprintf(env->stderr, "Failed to create new JSON stream.");
+			fprintf(env->stderr_saved, "Failed to create new JSON stream.");
 	}
 
 	if (w) {
@@ -1708,8 +1708,8 @@ int main(int argc, char **argv)
 		return -1;
 	}
 
-	env.stdout = stdout;
-	env.stderr = stderr;
+	env.stdout_saved = stdout;
+	env.stderr_saved = stderr;
 
 	env.has_testmod = true;
 	if (!env.list_test_names) {
@@ -1717,7 +1717,7 @@ int main(int argc, char **argv)
 		unload_bpf_testmod(verbose());
 
 		if (load_bpf_testmod(verbose())) {
-			fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
+			fprintf(env.stderr_saved, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
 			env.has_testmod = false;
 		}
 	}
@@ -1795,7 +1795,7 @@ int main(int argc, char **argv)
 		}
 
 		if (env.list_test_names) {
-			fprintf(env.stdout, "%s\n", test->test_name);
+			fprintf(env.stdout_saved, "%s\n", test->test_name);
 			env.succ_cnt++;
 			continue;
 		}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index b1e949fb16cf..cb9d6d46826b 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -75,7 +75,7 @@ struct subtest_state {
 	bool skipped;
 	bool filtered;
 
-	FILE *stdout;
+	FILE *stdout_saved;
 };
 
 struct test_state {
@@ -92,7 +92,7 @@ struct test_state {
 	size_t log_cnt;
 	char *log_buf;
 
-	FILE *stdout;
+	FILE *stdout_saved;
 };
 
 struct test_env {
@@ -111,8 +111,8 @@ struct test_env {
 	struct test_state *test_state; /* current running test state */
 	struct subtest_state *subtest_state; /* current running subtest state */
 
-	FILE *stdout;
-	FILE *stderr;
+	FILE *stdout_saved;
+	FILE *stderr_saved;
 	int nr_cpus;
 	FILE *json;
 
-- 
2.34.1

[PATCH bpf-next v2 8/8] selftests/bpf: Fix error compiling tc_redirect.c with musl libc

[Tony Ambardar]

Linux 5.1 implemented 64-bit time types and related syscalls to address the
Y2038 problem generally across archs. Userspace handling of Y2038 varies
with the libc however. While musl libc uses 64-bit time across all 32-bit
and 64-bit platforms, GNU glibc uses 64-bit time on 64-bit platforms but
defaults to 32-bit time on 32-bit platforms unless they "opt-in" to 64-bit
time or explicitly use 64-bit syscalls and time structures.

One specific area is the standard setsockopt() call, SO_TIMESTAMPNS option
used for timestamping, and the related output 'struct timespec'. GNU glibc
defaults as above, also exposing the SO_TIMESTAMPNS_NEW flag to explicitly
use a 64-bit call and 'struct __kernel_timespec'. Since these are not
exposed or needed with musl libc, their use in tc_redirect.c leads to
compile errors building for mips64el/musl:

  tc_redirect.c: In function 'rcv_tstamp':
  tc_redirect.c:425:32: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'?
    425 |             cmsg->cmsg_type == SO_TIMESTAMPNS_NEW)
        |                                ^~~~~~~~~~~~~~~~~~
        |                                SO_TIMESTAMPNS
  tc_redirect.c:425:32: note: each undeclared identifier is reported only once for each function it appears in
  tc_redirect.c: In function 'test_inet_dtime':
  tc_redirect.c:491:49: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'?
    491 |         err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
        |                                                 ^~~~~~~~~~~~~~~~~~
        |                                                 SO_TIMESTAMPNS

However, using SO_TIMESTAMPNS_NEW isn't strictly needed, nor is Y2038 being
explicitly tested. The timestamp checks in tc_redirect.c are simple: the
packet receive timestamp is non-zero and processed/handled in less than 5
seconds.

Switch to using the standard setsockopt() call and SO_TIMESTAMPNS option to
ensure compatibility across glibc and musl libc. In the worst-case, there
is a 5-second window 14 years from now where tc_redirect tests may fail on
32-bit systems. However, we should reasonably expect glibc to adopt a
64-bit mandate rather than the current "opt-in" policy before the Y2038
roll-over.

Fixes: ce6f6cffaeaa ("selftests/bpf: Wait for the netstamp_needed_key static key to be turned on")
Fixes: c803475fd8dd ("bpf: selftests: test skb->tstamp in redirect_neigh")
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index 327d51f59142..53b8ffc943dc 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
@@ -471,7 +471,7 @@ static int set_forwarding(bool enable)
 
 static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp)
 {
-	struct __kernel_timespec pkt_ts = {};
+	struct timespec pkt_ts = {};
 	char ctl[CMSG_SPACE(sizeof(pkt_ts))];
 	struct timespec now_ts;
 	struct msghdr msg = {};
@@ -495,7 +495,7 @@ static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp)
 
 	cmsg = CMSG_FIRSTHDR(&msg);
 	if (cmsg && cmsg->cmsg_level == SOL_SOCKET &&
-	    cmsg->cmsg_type == SO_TIMESTAMPNS_NEW)
+	    cmsg->cmsg_type == SO_TIMESTAMPNS)
 		memcpy(&pkt_ts, CMSG_DATA(cmsg), sizeof(pkt_ts));
 
 	pkt_ns = pkt_ts.tv_sec * NSEC_PER_SEC + pkt_ts.tv_nsec;
@@ -537,9 +537,9 @@ static int wait_netstamp_needed_key(void)
 	if (!ASSERT_GE(srv_fd, 0, "start_server"))
 		goto done;
 
-	err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
+	err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS,
 			 &opt, sizeof(opt));
-	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)"))
+	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)"))
 		goto done;
 
 	cli_fd = connect_to_fd(srv_fd, TIMEOUT_MILLIS);
@@ -621,9 +621,9 @@ static void test_inet_dtime(int family, int type, const char *addr, __u16 port)
 		return;
 
 	/* Ensure the kernel puts the (rcv) timestamp for all skb */
-	err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
+	err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS,
 			 &opt, sizeof(opt));
-	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)"))
+	if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)"))
 		goto done;
 
 	if (type == SOCK_STREAM) {
-- 
2.34.1