diff mbox series

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

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

Checks

Context Check Description
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-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-35 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
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-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 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-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-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-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-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-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
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on s390x with gcc

Commit Message

Yauheni Kaliuta July 28, 2023, 2:27 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>
---
v2:
- remove extra BUILD_BUG_ON
- add structure alignement

---
 kernel/trace/trace_syscalls.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Yonghong Song July 28, 2023, 4:44 p.m. UTC | #1 
On 7/28/23 7:27 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.
> 
> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
> ---
> v2:
> - remove extra BUILD_BUG_ON
> - add structure alignement
> 
> ---
>   kernel/trace/trace_syscalls.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 942ddbdace4a..b7139f8f4ce8 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;
> +	} __aligned(8) param;
>   	int i;
>   
> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));

Considering we used 'unsigned long long regs' before, should
the above BUILD_BUG_ON should be
	BUILD_BUG_ON(sizeof(param.ent) < sizeof(long long));
?

> +
> +	/* __bpf_prog_run() requires *regs as the first parameter */

This comment is not correct.

static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
                                           const void *ctx,
                                           bpf_dispatcher_fn dfunc)
{
	...
}

The first parameter is 'prog'.

Also there is no __bpf_prog_run() referenced in this function
so this comment may confuse readers. So I suggest removing
this comment. The same for perf_call_bpf_exit() below.

>   	*(struct pt_regs **)&param = regs;
>   	param.syscall_nr = rec->nr;
>   	for (i = 0; i < sys_data->nb_args; i++)
> @@ -657,11 +660,12 @@ 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;
> +	} __aligned(8) param;
>   
> +	/* __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 31, 2023, 8:07 a.m. UTC | #2 
Hi, Yonghong!

>>>>> On Fri, 28 Jul 2023 09:44:20 -0700, Yonghong Song  wrote:

 > On 7/28/23 7:27 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.
 >> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
 >> ---
 >> v2:
 >> - remove extra BUILD_BUG_ON
 >> - add structure alignement
 >> ---
 >> kernel/trace/trace_syscalls.c | 12 ++++++++----
 >> 1 file changed, 8 insertions(+), 4 deletions(-)
 >> diff --git a/kernel/trace/trace_syscalls.c
 >> b/kernel/trace/trace_syscalls.c
 >> index 942ddbdace4a..b7139f8f4ce8 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;
 >> +	} __aligned(8) param;
 >> int i;
 >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));

 > Considering we used 'unsigned long long regs' before, should
 > the above BUILD_BUG_ON should be
 > 	BUILD_BUG_ON(sizeof(param.ent) < sizeof(long long));
 > ?

Since the pointer's value is assigned I agree with Alexei (in the
thread [1]) to use void *.

 >> +
 >> +	/* __bpf_prog_run() requires *regs as the first parameter */

 > This comment is not correct.

 > static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
 >                                           const void *ctx,
 >                                           bpf_dispatcher_fn dfunc)
 > {
 > 	...
 > }

 > The first parameter is 'prog'.

 > Also there is no __bpf_prog_run() referenced in this function
 > so this comment may confuse readers. So I suggest removing
 > this comment. The same for perf_call_bpf_exit() below.

Again, in [1] we agreed that it's better to have the comment
since it's even more confusing.

Could you help to formulate it?

"__bpf_prog_run() requires *regs as the first argument for bpf
prog" or something?

But yes, I can remove it of course.

 >> *(struct pt_regs **)&param = regs;
 >> param.syscall_nr = rec->nr;
 >> for (i = 0; i < sys_data->nb_args; i++)
 >> @@ -657,11 +660,12 @@ 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;
 >> +	} __aligned(8) param;
 >> +	/* __bpf_prog_run() requires *regs as the first parameter */
 >> *(struct pt_regs **)&param = regs;
 >> param.syscall_nr = rec->nr;
 >> param.ret = rec->ret;


[1] https://lore.kernel.org/bpf/xunyjzy64q9b.fsf@redhat.com/T/#u
Yonghong Song July 31, 2023, 6:20 p.m. UTC | #3 
On 7/31/23 1:07 AM, Yauheni Kaliuta wrote:
> Hi, Yonghong!
> 
>>>>>> On Fri, 28 Jul 2023 09:44:20 -0700, Yonghong Song  wrote:
> 
>   > On 7/28/23 7:27 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.
>   >> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
>   >> ---
>   >> v2:
>   >> - remove extra BUILD_BUG_ON
>   >> - add structure alignement
>   >> ---
>   >> kernel/trace/trace_syscalls.c | 12 ++++++++----
>   >> 1 file changed, 8 insertions(+), 4 deletions(-)
>   >> diff --git a/kernel/trace/trace_syscalls.c
>   >> b/kernel/trace/trace_syscalls.c
>   >> index 942ddbdace4a..b7139f8f4ce8 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;
>   >> +	} __aligned(8) param;
>   >> int i;
>   >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
> 
>   > Considering we used 'unsigned long long regs' before, should
>   > the above BUILD_BUG_ON should be
>   > 	BUILD_BUG_ON(sizeof(param.ent) < sizeof(long long));
>   > ?
> 
> Since the pointer's value is assigned I agree with Alexei (in the
> thread [1]) to use void *.

Okay, let us compare to sizeof(void *) then.

> 
>   >> +
>   >> +	/* __bpf_prog_run() requires *regs as the first parameter */
> 
>   > This comment is not correct.
> 
>   > static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
>   >                                           const void *ctx,
>   >                                           bpf_dispatcher_fn dfunc)
>   > {
>   > 	...
>   > }
> 
>   > The first parameter is 'prog'.
> 
>   > Also there is no __bpf_prog_run() referenced in this function
>   > so this comment may confuse readers. So I suggest removing
>   > this comment. The same for perf_call_bpf_exit() below.
> 
> Again, in [1] we agreed that it's better to have the comment
> since it's even more confusing.
> 
> Could you help to formulate it?
> 
> "__bpf_prog_run() requires *regs as the first argument for bpf
> prog" or something?
> 
> But yes, I can remove it of course.

You could have a comment like below:
	/* bpf prog requires 'regs' to be the first member in the ctx (a.k.a. 
&param) */

> 
>   >> *(struct pt_regs **)&param = regs;
>   >> param.syscall_nr = rec->nr;
>   >> for (i = 0; i < sys_data->nb_args; i++)
>   >> @@ -657,11 +660,12 @@ 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;
>   >> +	} __aligned(8) param;
>   >> +	/* __bpf_prog_run() requires *regs as the first parameter */
>   >> *(struct pt_regs **)&param = regs;
>   >> param.syscall_nr = rec->nr;
>   >> param.ret = rec->ret;
> 
> 
> [1] https://lore.kernel.org/bpf/xunyjzy64q9b.fsf@redhat.com/T/#u
>
Yauheni Kaliuta Aug. 1, 2023, 7:49 a.m. UTC | #4 
Hi, Yonghong!

>>>>> On Mon, 31 Jul 2023 11:20:55 -0700, Yonghong Song  wrote:


 >> >> +
 >> >> +	/* __bpf_prog_run() requires *regs as the first parameter */
 >> > This comment is not correct.
 >> > static __always_inline u32 __bpf_prog_run(const struct bpf_prog
 >> *prog,
 >> >                                           const void *ctx,
 >> >                                           bpf_dispatcher_fn dfunc)
 >> > {
 >> > 	...
 >> > }
 >> > The first parameter is 'prog'.
 >> > Also there is no __bpf_prog_run() referenced in this function
 >> > so this comment may confuse readers. So I suggest removing
 >> > this comment. The same for perf_call_bpf_exit() below.
 >> Again, in [1] we agreed that it's better to have the comment
 >> since it's even more confusing.
 >> Could you help to formulate it?
 >> "__bpf_prog_run() requires *regs as the first argument for bpf
 >> prog" or something?
 >> But yes, I can remove it of course.

 > You could have a comment like below:
 > 	/* bpf prog requires 'regs' to be the first member in the ctx
 > 	(a.k.a. &param) */


Thanks!
diff mbox series

Patch

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 942ddbdace4a..b7139f8f4ce8 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;
+	} __aligned(8) 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,12 @@  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;
+	} __aligned(8) param;
 
+	/* __bpf_prog_run() requires *regs as the first parameter */
 	*(struct pt_regs **)&param = regs;
 	param.syscall_nr = rec->nr;
 	param.ret = rec->ret;