[bpf] selftests/bpf: fix backtrace printing for selftests crashes

Commit Message

Eduard Zingerman Oct. 3, 2024, 9:03 p.m. UTC

test_progs uses glibc specific functions backtrace() and
backtrace_symbols_fd() to print backtrace in case of SIGSEGV.

Recent commit (see fixes) updated test_progs.c to define stub versions
of the same functions with attriubte "weak" in order to allow linking
test_progs against musl libc. Unfortunately this broke the backtrace
handling for glibc builds.

As it turns out, glibc defines backtrace() and backtrace_symbols_fd()
as weak:

  $ llvm-readelf --symbols /lib64/libc.so.6 \
     | grep -P '( backtrace_symbols_fd| backtrace)$'
  4910: 0000000000126b40   161 FUNC    WEAK   DEFAULT    16 backtrace
  6843: 0000000000126f90   852 FUNC    WEAK   DEFAULT    16 backtrace_symbols_fd

So does test_progs:

 $ llvm-readelf --symbols test_progs \
    | grep -P '( backtrace_symbols_fd| backtrace)$'
  2891: 00000000006ad190    15 FUNC    WEAK   DEFAULT    13 backtrace
 11215: 00000000006ad1a0    41 FUNC    WEAK   DEFAULT    13 backtrace_symbols_fd

In such situation dynamic linker is not obliged to favour glibc
implementation over the one defined in test_progs.

Compiling with the following simple modification to test_progs.c
demonstrates the issue:

  $ git diff
  ...
  \--- a/tools/testing/selftests/bpf/test_progs.c
  \+++ b/tools/testing/selftests/bpf/test_progs.c
  \@@ -1817,6 +1817,7 @@ int main(int argc, char **argv)
          if (err)
                  return err;

  +       *(int *)0xdeadbeef  = 42;
          err = cd_flavor_subdir(argv[0]);
          if (err)
                  return err;

  $ ./test_progs
  [0]: Caught signal #11!
  Stack trace:
  <backtrace not supported>
  Segmentation fault (core dumped)

Resolve this by hiding stub definitions behind __GLIBC__ macro check
instead of using "weak" attribute.

Fixes: c9a83e76b5a9 ("selftests/bpf: Fix compile if backtrace support missing in libc")

CC: Tony Ambardar <[e-mail]>
Signed-off-by: Eduard Zingerman <[e-mail]>
---
 tools/testing/selftests/bpf/test_progs.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Eduard Zingerman Oct. 3, 2024, 9:07 p.m. UTC | #1

On Thu, 2024-10-03 at 14:03 -0700, Eduard Zingerman wrote:

[...]

> Resolve this by hiding stub definitions behind __GLIBC__ macro check
> instead of using "weak" attribute.
> 
> Fixes: c9a83e76b5a9 ("selftests/bpf: Fix compile if backtrace support missing in libc")

Hi Tony,

could you please double-check if your musl setup behaves as expected
after these changes?

Thanks,
Eduard

Daniel Xu Oct. 4, 2024, 5:21 p.m. UTC | #2

On Thu, Oct 03, 2024 at 02:03:07PM GMT, Eduard Zingerman wrote:
> test_progs uses glibc specific functions backtrace() and
> backtrace_symbols_fd() to print backtrace in case of SIGSEGV.
> 
> Recent commit (see fixes) updated test_progs.c to define stub versions
> of the same functions with attriubte "weak" in order to allow linking
> test_progs against musl libc. Unfortunately this broke the backtrace
> handling for glibc builds.
> 
> As it turns out, glibc defines backtrace() and backtrace_symbols_fd()
> as weak:
> 
>   $ llvm-readelf --symbols /lib64/libc.so.6 \
>      | grep -P '( backtrace_symbols_fd| backtrace)$'
>   4910: 0000000000126b40   161 FUNC    WEAK   DEFAULT    16 backtrace
>   6843: 0000000000126f90   852 FUNC    WEAK   DEFAULT    16 backtrace_symbols_fd
> 
> So does test_progs:
> 
>  $ llvm-readelf --symbols test_progs \
>     | grep -P '( backtrace_symbols_fd| backtrace)$'
>   2891: 00000000006ad190    15 FUNC    WEAK   DEFAULT    13 backtrace
>  11215: 00000000006ad1a0    41 FUNC    WEAK   DEFAULT    13 backtrace_symbols_fd
> 
> In such situation dynamic linker is not obliged to favour glibc
> implementation over the one defined in test_progs.
> 
> Compiling with the following simple modification to test_progs.c
> demonstrates the issue:
> 
>   $ git diff
>   ...
>   \--- a/tools/testing/selftests/bpf/test_progs.c
>   \+++ b/tools/testing/selftests/bpf/test_progs.c
>   \@@ -1817,6 +1817,7 @@ int main(int argc, char **argv)
>           if (err)
>                   return err;
> 
>   +       *(int *)0xdeadbeef  = 42;
>           err = cd_flavor_subdir(argv[0]);
>           if (err)
>                   return err;
> 
>   $ ./test_progs
>   [0]: Caught signal #11!
>   Stack trace:
>   <backtrace not supported>
>   Segmentation fault (core dumped)
> 
> Resolve this by hiding stub definitions behind __GLIBC__ macro check
> instead of using "weak" attribute.
> 
> Fixes: c9a83e76b5a9 ("selftests/bpf: Fix compile if backtrace support missing in libc")
> 
> CC: Tony Ambardar <[e-mail]>
> Signed-off-by: Eduard Zingerman <[e-mail]>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 7846f7f98908..005ff506b527 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -20,20 +20,23 @@
>  
>  #include "network_helpers.h"
>  
> +/* backtrace() and backtrace_symbols_fd() are glibc specific,
> + * use header file when glibc is available and provide stub
> + * implementations when another libc implementation is used.
> + */
>  #ifdef __GLIBC__
>  #include <execinfo.h> /* backtrace */
> -#endif
> -
> -/* Default backtrace funcs if missing at link */
> -__weak int backtrace(void **buffer, int size)
> +#else
> +int backtrace(void **buffer, int size)
>  {
>  	return 0;
>  }
>  
> -__weak void backtrace_symbols_fd(void *const *buffer, int size, int fd)
> +void backtrace_symbols_fd(void *const *buffer, int size, int fd)
>  {
>  	dprintf(fd, "<backtrace not supported>\n");
>  }
> +#endif /*__GLIBC__ */
>  
>  int env_verbosity = 0;
>  
> -- 
> 2.46.1
> 
> 

Acked-by: Daniel Xu <[e-mail]>

Tony Ambardar Oct. 6, 2024, 8:12 a.m. UTC | #3

On Thu, Oct 03, 2024 at 02:07:23PM -0700, Eduard Zingerman wrote:
> On Thu, 2024-10-03 at 14:03 -0700, Eduard Zingerman wrote:
> 
> [...]
> 
> > Resolve this by hiding stub definitions behind __GLIBC__ macro check
> > instead of using "weak" attribute.
> > 
> > Fixes: c9a83e76b5a9 ("selftests/bpf: Fix compile if backtrace support missing in libc")
> 
> Hi Tony,
> 
> could you please double-check if your musl setup behaves as expected
> after these changes?
> 

Hi Eduard,

I discovered building for musl has broken over the last month or so, and
it took some time to find fixes and workarounds before I could retest.

Since glibc execinfo.h also defines its functions as weak, and given the
linking issues that can cause, I think changing the #ifdef as you did is
the right approach. But could you leave the fallback stub functions as
"__weak" like before to simplify overriding in the non-GLIBC case?

Otherwise:

Reviewed-by: Tony Ambardar <[e-mail]>
Tested-by: Tony Ambardar <[e-mail]>

Thanks,
Tony

> Thanks,
> Eduard
>

Andrii Nakryiko Oct. 8, 2024, 3:39 a.m. UTC | #4

On Sun, Oct 6, 2024 at 1:12 AM Tony Ambardar <[e-mail]> wrote:
>
> On Thu, Oct 03, 2024 at 02:07:23PM -0700, Eduard Zingerman wrote:
> > On Thu, 2024-10-03 at 14:03 -0700, Eduard Zingerman wrote:
> >
> > [...]
> >
> > > Resolve this by hiding stub definitions behind __GLIBC__ macro check
> > > instead of using "weak" attribute.
> > >
> > > Fixes: c9a83e76b5a9 ("selftests/bpf: Fix compile if backtrace support missing in libc")
> >
> > Hi Tony,
> >
> > could you please double-check if your musl setup behaves as expected
> > after these changes?
> >
>
> Hi Eduard,
>
> I discovered building for musl has broken over the last month or so, and
> it took some time to find fixes and workarounds before I could retest.
>
> Since glibc execinfo.h also defines its functions as weak, and given the
> linking issues that can cause, I think changing the #ifdef as you did is
> the right approach. But could you leave the fallback stub functions as
> "__weak" like before to simplify overriding in the non-GLIBC case?
>

I added __weak back while applying.

> Otherwise:
>
> Reviewed-by: Tony Ambardar <[e-mail]>
> Tested-by: Tony Ambardar <[e-mail]>
>
> Thanks,
> Tony
>
> > Thanks,
> > Eduard
> >

[e-mail] Oct. 8, 2024, 3:40 a.m. UTC | #5

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <[e-mail]>:

On Thu,  3 Oct 2024 14:03:07 -0700 you wrote:
> test_progs uses glibc specific functions backtrace() and
> backtrace_symbols_fd() to print backtrace in case of SIGSEGV.
> 
> Recent commit (see fixes) updated test_progs.c to define stub versions
> of the same functions with attriubte "weak" in order to allow linking
> test_progs against musl libc. Unfortunately this broke the backtrace
> handling for glibc builds.
> 
> [...]

Here is the summary with links:
  - [bpf] selftests/bpf: fix backtrace printing for selftests crashes
    https://git.kernel.org/bpf/bpf-next/c/5bf1557e3d6a

You are awesome, thank you!

Patch

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 7846f7f98908..005ff506b527 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -20,20 +20,23 @@ 
 
 #include "network_helpers.h"
 
+/* backtrace() and backtrace_symbols_fd() are glibc specific,
+ * use header file when glibc is available and provide stub
+ * implementations when another libc implementation is used.
+ */
 #ifdef __GLIBC__
 #include <execinfo.h> /* backtrace */
-#endif
-
-/* Default backtrace funcs if missing at link */
-__weak int backtrace(void **buffer, int size)
+#else
+int backtrace(void **buffer, int size)
 {
 	return 0;
 }
 
-__weak void backtrace_symbols_fd(void *const *buffer, int size, int fd)
+void backtrace_symbols_fd(void *const *buffer, int size, int fd)
 {
 	dprintf(fd, "<backtrace not supported>\n");
 }
+#endif /*__GLIBC__ */
 
 int env_verbosity = 0;