bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next] selftests/bpf: fix probe_user test failure with clang build kernel
Date: Tue, 28 Sep 2021 08:46:14 -0700	[thread overview]
Message-ID: <c9d610e7-eb5b-ba8e-2b2d-6c37eea57ef7@fb.com> (raw)
In-Reply-To: <CAEf4BzZG+qGoLdgtUTx208AP6MM4qMsBZE1Ua_in1ycA__QJEg@mail.gmail.com>



On 9/27/21 10:11 PM, Andrii Nakryiko wrote:
> On Mon, Sep 27, 2021 at 10:05 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> clang build kernel failed the selftest probe_user.
>>    $ ./test_progs -t probe_user
>>    $ ...
>>    $ test_probe_user:PASS:get_kprobe_res 0 nsec
>>    $ test_probe_user:FAIL:check_kprobe_res wrong kprobe res from probe read: 0.0.0.0:0
>>    $ #94 probe_user:FAIL
>>
>> The test attached to kernel function __sys_connect(). In net/socket.c, we have
>>    int __sys_connect(int fd, struct sockaddr __user *uservaddr, int addrlen)
>>    {
>>          ......
>>    }
>>    ...
>>    SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
>>                    int, addrlen)
>>    {
>>          return __sys_connect(fd, uservaddr, addrlen);
>>    }
>>
>> The gcc compiler (8.5.0) does not inline __sys_connect() in syscall entry
>> function. But latest clang trunk did the inlining. So the bpf program
>> is not triggered.
>>
>> To make the test more reliable, let us kprobe the syscall entry function
>> instead. But x86_64, arm64 and s390 has syscall wrapper and they have
>> to be handled specially. I also changed the test to use vmlinux.h and CORE
>> to accommodate relocatable pt_regs structure, similar to
>> samples/bpf/test_probe_write_user_kern.c.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   .../selftests/bpf/prog_tests/probe_user.c     |  4 +--
>>   .../selftests/bpf/progs/test_probe_user.c     | 30 +++++++++++++++----
>>   2 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/probe_user.c b/tools/testing/selftests/bpf/prog_tests/probe_user.c
>> index 95bd12097358..52fe157e2a90 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/probe_user.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/probe_user.c
>> @@ -3,7 +3,7 @@
>>
>>   void test_probe_user(void)
>>   {
>> -       const char *prog_name = "kprobe/__sys_connect";
>> +       const char *prog_name = "handle_sys_connect";
>>          const char *obj_file = "./test_probe_user.o";
>>          DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts, );
>>          int err, results_map_fd, sock_fd, duration = 0;
>> @@ -18,7 +18,7 @@ void test_probe_user(void)
>>          if (!ASSERT_OK_PTR(obj, "obj_open_file"))
>>                  return;
>>
>> -       kprobe_prog = bpf_object__find_program_by_title(obj, prog_name);
>> +       kprobe_prog = bpf_object__find_program_by_name(obj, prog_name);
>>          if (CHECK(!kprobe_prog, "find_probe",
>>                    "prog '%s' not found\n", prog_name))
>>                  goto cleanup;
>> diff --git a/tools/testing/selftests/bpf/progs/test_probe_user.c b/tools/testing/selftests/bpf/progs/test_probe_user.c
>> index 89b3532ccc75..9b3ddbf6289d 100644
>> --- a/tools/testing/selftests/bpf/progs/test_probe_user.c
>> +++ b/tools/testing/selftests/bpf/progs/test_probe_user.c
>> @@ -1,21 +1,39 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>
>> -#include <linux/ptrace.h>
>> -#include <linux/bpf.h>
>> -
>> -#include <netinet/in.h>
>> +#include "vmlinux.h"
>>
>>   #include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_core_read.h>
>>   #include <bpf/bpf_tracing.h>
>>
>> +#if defined(__TARGET_ARCH_x86)
> 
> I mangled this check (locally) to test the #else case, and it failed
> compilation:
> 
> progs/test_probe_user.c:24:15: error: expected ')'
> SEC("kprobe/" SYS_PREFIX "sys_connect")
> 
> adding #define SYS_PREFIX "" fixes the issue.
> 
> But I'm also curious:
> 
> 1. do we need all this arch-specific logic? maybe we can find some
> other and more stable interface to attach to? e.g., would
> move_addr_to_kernel() work? it's a global function, so it shouldn't be
> inlined, right? Or did you intend to also demo the use of
> PT_REGS_PARM1_CORE() with this?

The following are move_add_to_kernel() call usages:

fs/io_uring.c:  return move_addr_to_kernel(conn->addr, conn->addr_len, 
&io->address);
fs/io_uring.c:          ret = move_addr_to_kernel(req->connect.addr,
include/linux/socket.h:extern int move_addr_to_kernel(void __user 
*uaddr, int ulen, struct sockaddr_storage *kaddr);
net/compat.c:                   err = 
move_addr_to_kernel(compat_ptr(msg.msg_name),
net/socket.c: * move_addr_to_kernel     -       copy a socket address 
into kernel space
net/socket.c:int move_addr_to_kernel(void __user *uaddr, int ulen, 
struct sockaddr_storage *kaddr)
net/socket.c:           err = move_addr_to_kernel(umyaddr, addrlen, 
&address);
net/socket.c:           ret = move_addr_to_kernel(uservaddr, addrlen, 
&address);
net/socket.c:           err = move_addr_to_kernel(addr, addr_len, &address);
net/socket.c:                   err = move_addr_to_kernel(msg.msg_name,

inlining could happen within net/socket.c. Another two use cases are
fs/io_uring.c and net/compat.c. Using compat syscall may be too complex, 
I assume. Using io_uring with bpf selftest may be a choice but I think
it makes thing too complicated.

More importantly, assuming in the future the kernel may be compiled
with LTO, move_addr_to_kernel() might be inlined into fs/io_uring.c.
So that is the main reason I am using syscall entry point directly.

You are right, vmlinux.h is generated from the very kernel we are 
testing so we don't need CORE. I added CORE similar to
samples/bpf/test_probe_write_user_kern.c, but it is not really
needed here.

> 
> 2. global .rodata variable isn't really necessary. Static one would
> work just fine and would be eliminated by the compiler at compilation
> time. Or equivalently just doing #define SYS_WRAPPER 1 for all cases
> but #else would work as well. I don't mind global var, but I'm just
> curious if you explicitly wanted to test .rodata global variable in
> this case?

Good point. This test is not to test .rodata variable. So using macro
to differentiate different cases is actually better.

> 
> 
>> +volatile const int syscall_wrapper = 1;
>> +#define SYS_PREFIX "__x64_"
>> +#elif defined(__TARGET_ARCH_s390)
>> +volatile const int syscall_wrapper = 1;
>> +#define SYS_PREFIX "__s390x_"
>> +#elif defined(__TARGET_ARCH_arm64)
>> +volatile const int syscall_wrapper = 1;
>> +#define SYS_PREFIX "__arm64_"
>> +#else
>> +volatile const int syscall_wrapper = 0;
>> +#endif
>> +
>>   static struct sockaddr_in old;
>>
>> -SEC("kprobe/__sys_connect")
>> +SEC("kprobe/" SYS_PREFIX "sys_connect")
>>   int BPF_KPROBE(handle_sys_connect)
>>   {
>> -       void *ptr = (void *)PT_REGS_PARM2(ctx);
>> +       void *ptr;
>>          struct sockaddr_in new;
>>
>> +       if (syscall_wrapper == 0) {
>> +               ptr = (void *)PT_REGS_PARM2_CORE(ctx);
>> +       } else {
>> +               struct pt_regs *real_regs = (struct pt_regs *)PT_REGS_PARM1_CORE(ctx);
>> +               ptr = (void *)PT_REGS_PARM2_CORE(real_regs);
>> +       }
>> +
>>          bpf_probe_read_user(&old, sizeof(old), ptr);
>>          __builtin_memset(&new, 0xab, sizeof(new));
>>          bpf_probe_write_user(ptr, &new, sizeof(new));
>> --
>> 2.30.2
>>

  reply	other threads:[~2021-09-28 15:46 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-27 17:05 [PATCH bpf-next] selftests/bpf: fix probe_user test failure with clang build kernel Yonghong Song
2021-09-28  5:11 ` Andrii Nakryiko
2021-09-28 15:46   ` Yonghong Song [this message]
2021-09-28 22:46     ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c9d610e7-eb5b-ba8e-2b2d-6c37eea57ef7@fb.com \
    --to=yhs@fb.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).