diff mbox series

[bpf-next] selftests/bpf: Fix flaky test ptr_untrusted

Message ID 20240204194452.2785936-1-yonghong.song@linux.dev (mailing list archive)
State Accepted
Commit 7e428638bd784fd9e8944bfbf11513520e141b91
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: Fix flaky test ptr_untrusted | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
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/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 12 maintainers not CCed: eddyz87@gmail.com jolsa@kernel.org martin.lau@linux.dev mykolal@fb.com sdf@google.com linux-kselftest@vger.kernel.org john.fastabend@gmail.com song@kernel.org haoluo@google.com shuah@kernel.org laoar.shao@gmail.com kpsingh@kernel.org
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 success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / veristat

Commit Message

Yonghong Song Feb. 4, 2024, 7:44 p.m. UTC 
Somehow recently I frequently hit the following test failure
with either ./test_progs or ./test_progs-cpuv4:
  serial_test_ptr_untrusted:PASS:skel_open 0 nsec
  serial_test_ptr_untrusted:PASS:lsm_attach 0 nsec
  serial_test_ptr_untrusted:PASS:raw_tp_attach 0 nsec
  serial_test_ptr_untrusted:FAIL:cmp_tp_name unexpected cmp_tp_name: actual -115 != expected 0
  #182     ptr_untrusted:FAIL

Further investigation found the failure is due to
  bpf_probe_read_user_str()
where reading user-level string attr->raw_tracepoint.name
is not successfully, most likely due to the
string itself still in disk and not populated into memory yet.

One solution is do a printf() call of the string before doing bpf
syscall which will force the raw_tracepoint.name into memory.
But I think a more robust solution is to use bpf_copy_from_user()
which is used in sleepable program and can tolerate page fault,
and the fix here used the latter approach.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/testing/selftests/bpf/progs/test_ptr_untrusted.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org Feb. 5, 2024, 6:50 p.m. UTC | #1 
Hello:

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

On Sun,  4 Feb 2024 11:44:52 -0800 you wrote:
> Somehow recently I frequently hit the following test failure
> with either ./test_progs or ./test_progs-cpuv4:
>   serial_test_ptr_untrusted:PASS:skel_open 0 nsec
>   serial_test_ptr_untrusted:PASS:lsm_attach 0 nsec
>   serial_test_ptr_untrusted:PASS:raw_tp_attach 0 nsec
>   serial_test_ptr_untrusted:FAIL:cmp_tp_name unexpected cmp_tp_name: actual -115 != expected 0
>   #182     ptr_untrusted:FAIL
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: Fix flaky test ptr_untrusted
    https://git.kernel.org/bpf/bpf-next/c/7e428638bd78

You are awesome, thank you!
Andrii Nakryiko Feb. 5, 2024, 6:56 p.m. UTC | #2 
On Sun, Feb 4, 2024 at 11:45 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Somehow recently I frequently hit the following test failure
> with either ./test_progs or ./test_progs-cpuv4:
>   serial_test_ptr_untrusted:PASS:skel_open 0 nsec
>   serial_test_ptr_untrusted:PASS:lsm_attach 0 nsec
>   serial_test_ptr_untrusted:PASS:raw_tp_attach 0 nsec
>   serial_test_ptr_untrusted:FAIL:cmp_tp_name unexpected cmp_tp_name: actual -115 != expected 0
>   #182     ptr_untrusted:FAIL
>
> Further investigation found the failure is due to
>   bpf_probe_read_user_str()
> where reading user-level string attr->raw_tracepoint.name
> is not successfully, most likely due to the
> string itself still in disk and not populated into memory yet.
>
> One solution is do a printf() call of the string before doing bpf
> syscall which will force the raw_tracepoint.name into memory.
> But I think a more robust solution is to use bpf_copy_from_user()
> which is used in sleepable program and can tolerate page fault,
> and the fix here used the latter approach.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/testing/selftests/bpf/progs/test_ptr_untrusted.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c b/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
> index 4bdd65b5aa2d..2fdc44e76624 100644
> --- a/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
> +++ b/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
> @@ -6,13 +6,13 @@
>
>  char tp_name[128];
>
> -SEC("lsm/bpf")
> +SEC("lsm.s/bpf")
>  int BPF_PROG(lsm_run, int cmd, union bpf_attr *attr, unsigned int size)
>  {
>         switch (cmd) {
>         case BPF_RAW_TRACEPOINT_OPEN:
> -               bpf_probe_read_user_str(tp_name, sizeof(tp_name) - 1,
> -                                       (void *)attr->raw_tracepoint.name);
> +               bpf_copy_from_user(tp_name, sizeof(tp_name) - 1,
> +                                  (void *)attr->raw_tracepoint.name);

Should we also add bpf_copy_from_user_str (and
bpf_copy_from_user_str_task) kfuncs to complete bpf_copy_from_user?
This change is not strictly equivalent (though for tests it's fine,
but in real-world apps it would be problematic).

>                 break;
>         default:
>                 break;
> --
> 2.34.1
>
Yonghong Song Feb. 6, 2024, 6:37 a.m. UTC | #3 
On 2/5/24 10:56 AM, Andrii Nakryiko wrote:
> On Sun, Feb 4, 2024 at 11:45 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Somehow recently I frequently hit the following test failure
>> with either ./test_progs or ./test_progs-cpuv4:
>>    serial_test_ptr_untrusted:PASS:skel_open 0 nsec
>>    serial_test_ptr_untrusted:PASS:lsm_attach 0 nsec
>>    serial_test_ptr_untrusted:PASS:raw_tp_attach 0 nsec
>>    serial_test_ptr_untrusted:FAIL:cmp_tp_name unexpected cmp_tp_name: actual -115 != expected 0
>>    #182     ptr_untrusted:FAIL
>>
>> Further investigation found the failure is due to
>>    bpf_probe_read_user_str()
>> where reading user-level string attr->raw_tracepoint.name
>> is not successfully, most likely due to the
>> string itself still in disk and not populated into memory yet.
>>
>> One solution is do a printf() call of the string before doing bpf
>> syscall which will force the raw_tracepoint.name into memory.
>> But I think a more robust solution is to use bpf_copy_from_user()
>> which is used in sleepable program and can tolerate page fault,
>> and the fix here used the latter approach.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/testing/selftests/bpf/progs/test_ptr_untrusted.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c b/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
>> index 4bdd65b5aa2d..2fdc44e76624 100644
>> --- a/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
>> +++ b/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
>> @@ -6,13 +6,13 @@
>>
>>   char tp_name[128];
>>
>> -SEC("lsm/bpf")
>> +SEC("lsm.s/bpf")
>>   int BPF_PROG(lsm_run, int cmd, union bpf_attr *attr, unsigned int size)
>>   {
>>          switch (cmd) {
>>          case BPF_RAW_TRACEPOINT_OPEN:
>> -               bpf_probe_read_user_str(tp_name, sizeof(tp_name) - 1,
>> -                                       (void *)attr->raw_tracepoint.name);
>> +               bpf_copy_from_user(tp_name, sizeof(tp_name) - 1,
>> +                                  (void *)attr->raw_tracepoint.name);
> Should we also add bpf_copy_from_user_str (and
> bpf_copy_from_user_str_task) kfuncs to complete bpf_copy_from_user?
> This change is not strictly equivalent (though for tests it's fine,
> but in real-world apps it would be problematic).

Sounds a good idea. Let me do some investigations!

>
>>                  break;
>>          default:
>>                  break;
>> --
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c b/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
index 4bdd65b5aa2d..2fdc44e76624 100644
--- a/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
+++ b/tools/testing/selftests/bpf/progs/test_ptr_untrusted.c
@@ -6,13 +6,13 @@ 
 
 char tp_name[128];
 
-SEC("lsm/bpf")
+SEC("lsm.s/bpf")
 int BPF_PROG(lsm_run, int cmd, union bpf_attr *attr, unsigned int size)
 {
 	switch (cmd) {
 	case BPF_RAW_TRACEPOINT_OPEN:
-		bpf_probe_read_user_str(tp_name, sizeof(tp_name) - 1,
-					(void *)attr->raw_tracepoint.name);
+		bpf_copy_from_user(tp_name, sizeof(tp_name) - 1,
+				   (void *)attr->raw_tracepoint.name);
 		break;
 	default:
 		break;