* [PATCH bpf-next] selftests/bpf: fix probe_user test failure with clang build kernel
@ 2021-09-27 17:05 Yonghong Song
2021-09-28 5:11 ` Andrii Nakryiko
0 siblings, 1 reply; 4+ messages in thread
From: Yonghong Song @ 2021-09-27 17:05 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
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)
+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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix probe_user test failure with clang build kernel
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
0 siblings, 1 reply; 4+ messages in thread
From: Andrii Nakryiko @ 2021-09-28 5:11 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team
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?
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?
> +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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix probe_user test failure with clang build kernel
2021-09-28 5:11 ` Andrii Nakryiko
@ 2021-09-28 15:46 ` Yonghong Song
2021-09-28 22:46 ` Andrii Nakryiko
0 siblings, 1 reply; 4+ messages in thread
From: Yonghong Song @ 2021-09-28 15:46 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team
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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix probe_user test failure with clang build kernel
2021-09-28 15:46 ` Yonghong Song
@ 2021-09-28 22:46 ` Andrii Nakryiko
0 siblings, 0 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2021-09-28 22:46 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team
On Tue, Sep 28, 2021 at 8:46 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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.
io_uring for this would be overkill
>
> 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.
Yep, makes sense.
>
> 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.
Ok, sounds good.
>
> >
> >
> >> +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
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-28 22:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2021-09-28 22:46 ` Andrii Nakryiko
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).