diff mbox series

[bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t

Message ID 20230727150647.397626-1-ykaliuta@redhat.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
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: 1345 this patch: 1345
netdev/cc_maintainers fail 3 maintainers not CCed: mhiramat@kernel.org linux-trace-kernel@vger.kernel.org rostedt@goodmis.org
netdev/build_clang success Errors and warnings before: 1365 this patch: 1365
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: 1367 this patch: 1367
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 31 lines checked
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-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on s390x with gcc

Commit Message

Yauheni Kaliuta July 27, 2023, 3:06 p.m. UTC 
bpf tracepoint program uses struct trace_event_raw_sys_enter as
argument where trace_entry is the first field. Use the same instead
of unsigned long long since if it's amended (for example by RT
patch) it accesses data with wrong offset.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
---
 kernel/trace/trace_syscalls.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Yonghong Song July 27, 2023, 5:37 p.m. UTC | #1 
On 7/27/23 8:06 AM, Yauheni Kaliuta wrote:
> bpf tracepoint program uses struct trace_event_raw_sys_enter as
> argument where trace_entry is the first field. Use the same instead
> of unsigned long long since if it's amended (for example by RT
> patch) it accesses data with wrong offset.

Is this 'amended by RT patch' a real thing?

> 
> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
> ---
>   kernel/trace/trace_syscalls.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 942ddbdace4a..07f4fa395e99 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -555,12 +555,15 @@ static int perf_call_bpf_enter(struct trace_event_call *call, struct pt_regs *re
>   			       struct syscall_trace_enter *rec)
>   {
>   	struct syscall_tp_t {
> -		unsigned long long regs;
> +		struct trace_entry ent;
>   		unsigned long syscall_nr;
>   		unsigned long args[SYSCALL_DEFINE_MAXARGS];
>   	} param;

I suspect we may have issues for 32bit kernel.
In 32bit kernel, with the change, the alignment for
param could be 4. That means, the 'ctx' pointer
may have an alignment 4 for bpf program, if user
tries to do ctx->regs, which will be a mis-aligned
access and it may not work for all architectures.

>   	int i;
>   
> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
> +
> +	/* __bpf_prog_run() requires *regs as the first parameter */
>   	*(struct pt_regs **)&param = regs;
>   	param.syscall_nr = rec->nr;
>   	for (i = 0; i < sys_data->nb_args; i++)
> @@ -657,11 +660,14 @@ static int perf_call_bpf_exit(struct trace_event_call *call, struct pt_regs *reg
>   			      struct syscall_trace_exit *rec)
>   {
>   	struct syscall_tp_t {
> -		unsigned long long regs;
> +		struct trace_entry ent;
>   		unsigned long syscall_nr;
>   		unsigned long ret;
>   	} param;
>   
> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));

You already have BUILD_BUG_ON in perf_call_enter. There is no need
to have another one here.

> +
> +	/* __bpf_prog_run() requires *regs as the first parameter */
>   	*(struct pt_regs **)&param = regs;
>   	param.syscall_nr = rec->nr;
>   	param.ret = rec->ret;
Yauheni Kaliuta July 28, 2023, 10:02 a.m. UTC | #2 
Hi, Yonghong!

>>>>> On Thu, 27 Jul 2023 10:37:10 -0700, Yonghong Song  wrote:

 > On 7/27/23 8:06 AM, Yauheni Kaliuta wrote:
 >> bpf tracepoint program uses struct trace_event_raw_sys_enter as
 >> argument where trace_entry is the first field. Use the same instead
 >> of unsigned long long since if it's amended (for example by RT
 >> patch) it accesses data with wrong offset.

 > Is this 'amended by RT patch' a real thing?

Yes for me.

>> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
 >> ---
 >> kernel/trace/trace_syscalls.c | 10 ++++++++--
 >> 1 file changed, 8 insertions(+), 2 deletions(-)
 >> diff --git a/kernel/trace/trace_syscalls.c
 >> b/kernel/trace/trace_syscalls.c
 >> index 942ddbdace4a..07f4fa395e99 100644
 >> --- a/kernel/trace/trace_syscalls.c
 >> +++ b/kernel/trace/trace_syscalls.c
 >> @@ -555,12 +555,15 @@ static int perf_call_bpf_enter(struct trace_event_call *call, struct pt_regs *re
 >> struct syscall_trace_enter *rec)
 >> {
 >> struct syscall_tp_t {
 >> -		unsigned long long regs;
 >> +		struct trace_entry ent;
 >> unsigned long syscall_nr;
 >> unsigned long args[SYSCALL_DEFINE_MAXARGS];
 >> } param;

 > I suspect we may have issues for 32bit kernel.
 > In 32bit kernel, with the change, the alignment for
 > param could be 4. That means, the 'ctx' pointer
 > may have an alignment 4 for bpf program, if user
 > tries to do ctx->regs, which will be a mis-aligned
 > access and it may not work for all architectures.

well, will __aligned(8) save the world?

 >> int i;
 >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
 >> +
 >> +	/* __bpf_prog_run() requires *regs as the first parameter */
 >> *(struct pt_regs **)&param = regs;
 >> param.syscall_nr = rec->nr;
 >> for (i = 0; i < sys_data->nb_args; i++)
 >> @@ -657,11 +660,14 @@ static int perf_call_bpf_exit(struct trace_event_call *call, struct pt_regs *reg
 >> struct syscall_trace_exit *rec)
 >> {
 >> struct syscall_tp_t {
 >> -		unsigned long long regs;
 >> +		struct trace_entry ent;
 >> unsigned long syscall_nr;
 >> unsigned long ret;
 >> } param;
 >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));

 > You already have BUILD_BUG_ON in perf_call_enter. There is no need
 > to have another one here.

Oh yes, thanks  :)

 >> +
 >> +	/* __bpf_prog_run() requires *regs as the first parameter */
 >> *(struct pt_regs **)&param = regs;
 >> param.syscall_nr = rec->nr;
 >> param.ret = rec->ret;
diff mbox series

Patch

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 942ddbdace4a..07f4fa395e99 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -555,12 +555,15 @@  static int perf_call_bpf_enter(struct trace_event_call *call, struct pt_regs *re
 			       struct syscall_trace_enter *rec)
 {
 	struct syscall_tp_t {
-		unsigned long long regs;
+		struct trace_entry ent;
 		unsigned long syscall_nr;
 		unsigned long args[SYSCALL_DEFINE_MAXARGS];
 	} param;
 	int i;
 
+	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
+
+	/* __bpf_prog_run() requires *regs as the first parameter */
 	*(struct pt_regs **)&param = regs;
 	param.syscall_nr = rec->nr;
 	for (i = 0; i < sys_data->nb_args; i++)
@@ -657,11 +660,14 @@  static int perf_call_bpf_exit(struct trace_event_call *call, struct pt_regs *reg
 			      struct syscall_trace_exit *rec)
 {
 	struct syscall_tp_t {
-		unsigned long long regs;
+		struct trace_entry ent;
 		unsigned long syscall_nr;
 		unsigned long ret;
 	} param;
 
+	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
+
+	/* __bpf_prog_run() requires *regs as the first parameter */
 	*(struct pt_regs **)&param = regs;
 	param.syscall_nr = rec->nr;
 	param.ret = rec->ret;