diff mbox series

[bpf-next,1/2] selftests/bpf: Fix dynptr/test_dynptr_is_null

Message ID 20230517040404.4023912-1-yhs@fb.com (mailing list archive)
State Accepted
Commit 12852f8e0f70d9a5263e2834c5483268b08b5e9c
Delegated to: BPF
Headers show
Series [bpf-next,1/2] selftests/bpf: Fix dynptr/test_dynptr_is_null | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 16 maintainers not CCed: kuba@kernel.org hawk@kernel.org netdev@vger.kernel.org kpsingh@kernel.org joannelkoong@gmail.com memxor@gmail.com john.fastabend@gmail.com sdf@google.com martin.lau@linux.dev shuah@kernel.org song@kernel.org mykolal@fb.com davem@davemloft.net linux-kselftest@vger.kernel.org jolsa@kernel.org haoluo@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning CHECK: extern prototypes should be avoided in .h files
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain_full }}
bpf/vmtest-bpf-next-VM_Test-2 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-9 success Logs for veristat

Commit Message

Yonghong Song May 17, 2023, 4:04 a.m. UTC
With latest llvm17, dynptr/test_dynptr_is_null subtest failed in my
testing VM. The failure log looks like below:
  All error logs:
  tester_init:PASS:tester_log_buf 0 nsec
  process_subtest:PASS:obj_open_mem 0 nsec
  process_subtest:PASS:Can't alloc specs array 0 nsec
  verify_success:PASS:dynptr_success__open 0 nsec
  verify_success:PASS:bpf_object__find_program_by_name 0 nsec
  verify_success:PASS:dynptr_success__load 0 nsec
  verify_success:PASS:bpf_program__attach 0 nsec
  verify_success:FAIL:err unexpected err: actual 4 != expected 0
  #65/9    dynptr/test_dynptr_is_null:FAIL

The error happens for bpf prog test_dynptr_is_null in dynptr_success.c.
        if (bpf_dynptr_is_null(&ptr2)) {
                err = 4;
                goto exit;
        }

The bpf_dynptr_is_null(&ptr) unexpectedly returned a non-zero
value and the control went to the error path.

Digging further, I found the root cause is due to function
signature difference between kernel and user space.

In kernel, we have
  __bpf_kfunc bool bpf_dynptr_is_null(struct bpf_dynptr_kern *ptr)
while in bpf_kfuncs.h we have
  extern int bpf_dynptr_is_null(const struct bpf_dynptr *ptr) __ksym;

The kernel bpf_dynptr_is_null disasm code:
  ffffffff812f1a90 <bpf_dynptr_is_null>:
  ffffffff812f1a90: f3 0f 1e fa           endbr64
  ffffffff812f1a94: 0f 1f 44 00 00        nopl    (%rax,%rax)
  ffffffff812f1a99: 53                    pushq   %rbx
  ffffffff812f1a9a: 48 89 fb              movq    %rdi, %rbx
  ffffffff812f1a9d: e8 ae 29 17 00        callq   0xffffffff81464450 <__asan_load8_noabort>
  ffffffff812f1aa2: 48 83 3b 00           cmpq    $0x0, (%rbx)
  ffffffff812f1aa6: 0f 94 c0              sete    %al
  ffffffff812f1aa9: 5b                    popq    %rbx
  ffffffff812f1aaa: c3                    retq

Note that only 1-byte register %al is set and the other 7-bytes are not touched.

In bpf program, the asm code for the above bpf_dynptr_is_null(&ptr2):
       266:       85 10 00 00 ff ff ff ff call -0x1
       267:       b4 01 00 00 04 00 00 00 w1 = 0x4
       268:       16 00 03 00 00 00 00 00 if w0 == 0x0 goto +0x3 <LBB9_8>

Basically, 4-byte subregister is tested. This might cause error
as the value other than the lowest byte might not be 0.

This patch fixed the issue by using the identical func prototype
across kernel and selftest user space. The fixed bpf asm code:
       267:       85 10 00 00 ff ff ff ff call -0x1
       268:       54 00 00 00 01 00 00 00 w0 &= 0x1
       269:       b4 01 00 00 04 00 00 00 w1 = 0x4
       270:       16 00 03 00 00 00 00 00 if w0 == 0x0 goto +0x3 <LBB9_8>

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/bpf_kfuncs.h            | 2 +-
 tools/testing/selftests/bpf/progs/dynptr_fail.c     | 1 +
 tools/testing/selftests/bpf/progs/dynptr_success.c  | 1 +
 tools/testing/selftests/bpf/progs/test_xdp_dynptr.c | 1 +
 4 files changed, 4 insertions(+), 1 deletion(-)

NOTE: It would be we can auto generate a header file for all kfunc's.
Similar to bpf_helper_defs.h, all kfunc prototypes can be put into
autogenerated bpf_kfunc_defs.h which can be used by selftests and
applications.

Comments

patchwork-bot+netdevbpf@kernel.org May 17, 2023, 3 p.m. UTC | #1
Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Tue, 16 May 2023 21:04:04 -0700 you wrote:
> With latest llvm17, dynptr/test_dynptr_is_null subtest failed in my
> testing VM. The failure log looks like below:
>   All error logs:
>   tester_init:PASS:tester_log_buf 0 nsec
>   process_subtest:PASS:obj_open_mem 0 nsec
>   process_subtest:PASS:Can't alloc specs array 0 nsec
>   verify_success:PASS:dynptr_success__open 0 nsec
>   verify_success:PASS:bpf_object__find_program_by_name 0 nsec
>   verify_success:PASS:dynptr_success__load 0 nsec
>   verify_success:PASS:bpf_program__attach 0 nsec
>   verify_success:FAIL:err unexpected err: actual 4 != expected 0
>   #65/9    dynptr/test_dynptr_is_null:FAIL
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] selftests/bpf: Fix dynptr/test_dynptr_is_null
    https://git.kernel.org/bpf/bpf-next/c/12852f8e0f70
  - [bpf-next,2/2] selftests/bpf: Make bpf_dynptr_is_rdonly() prototyype consistent with kernel
    https://git.kernel.org/bpf/bpf-next/c/effcf6241624

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index f3c41f8902a0..821c25b7d0df 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -36,7 +36,7 @@  extern void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr *ptr, __u32 offset,
 			      void *buffer, __u32 buffer__szk) __ksym;
 
 extern int bpf_dynptr_adjust(const struct bpf_dynptr *ptr, __u32 start, __u32 end) __ksym;
-extern int bpf_dynptr_is_null(const struct bpf_dynptr *ptr) __ksym;
+extern bool bpf_dynptr_is_null(const struct bpf_dynptr *ptr) __ksym;
 extern int bpf_dynptr_is_rdonly(const struct bpf_dynptr *ptr) __ksym;
 extern __u32 bpf_dynptr_size(const struct bpf_dynptr *ptr) __ksym;
 extern int bpf_dynptr_clone(const struct bpf_dynptr *ptr, struct bpf_dynptr *clone__init) __ksym;
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index c2f0e18af951..7ce7e827d5f0 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -3,6 +3,7 @@ 
 
 #include <errno.h>
 #include <string.h>
+#include <stdbool.h>
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include <linux/if_ether.h>
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index 0c053976f8f9..5985920d162e 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -2,6 +2,7 @@ 
 /* Copyright (c) 2022 Facebook */
 
 #include <string.h>
+#include <stdbool.h>
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include "bpf_misc.h"
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c b/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
index 25ee4a22e48d..78c368e71797 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
@@ -2,6 +2,7 @@ 
 /* Copyright (c) 2022 Meta */
 #include <stddef.h>
 #include <string.h>
+#include <stdbool.h>
 #include <linux/bpf.h>
 #include <linux/if_ether.h>
 #include <linux/if_packet.h>