On Sun, Feb 4, 2024 at 11:45 AM Yonghong Song <yonghong.song@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
