* [PATCH bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
@ 2023-07-27 15:06 Yauheni Kaliuta
2023-07-27 17:37 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Yauheni Kaliuta @ 2023-07-27 15:06 UTC (permalink / raw)
To: bpf; +Cc: andrii, ast, Yauheni Kaliuta
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(-)
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 **)¶m = 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 **)¶m = regs;
param.syscall_nr = rec->nr;
param.ret = rec->ret;
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-27 15:06 [PATCH bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t Yauheni Kaliuta
@ 2023-07-27 17:37 ` Yonghong Song
2023-07-28 10:02 ` Yauheni Kaliuta
2023-07-28 14:27 ` [PATCH bpf-next v2] " Yauheni Kaliuta
0 siblings, 2 replies; 11+ messages in thread
From: Yonghong Song @ 2023-07-27 17:37 UTC (permalink / raw)
To: Yauheni Kaliuta, bpf; +Cc: andrii, ast
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 **)¶m = 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 **)¶m = regs;
> param.syscall_nr = rec->nr;
> param.ret = rec->ret;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-27 17:37 ` Yonghong Song
@ 2023-07-28 10:02 ` Yauheni Kaliuta
2023-07-28 14:27 ` [PATCH bpf-next v2] " Yauheni Kaliuta
1 sibling, 0 replies; 11+ messages in thread
From: Yauheni Kaliuta @ 2023-07-28 10:02 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, andrii, ast
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 **)¶m = 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 **)¶m = regs;
>> param.syscall_nr = rec->nr;
>> param.ret = rec->ret;
--
WBR,
Yauheni Kaliuta
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next v2] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-27 17:37 ` Yonghong Song
2023-07-28 10:02 ` Yauheni Kaliuta
@ 2023-07-28 14:27 ` Yauheni Kaliuta
2023-07-28 16:44 ` Yonghong Song
2023-08-01 7:52 ` [PATCH bpf-next v3] " Yauheni Kaliuta
1 sibling, 2 replies; 11+ messages in thread
From: Yauheni Kaliuta @ 2023-07-28 14:27 UTC (permalink / raw)
To: bpf; +Cc: andrii, ast, yonghong.song, Yauheni Kaliuta
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 *));
+
+ /* __bpf_prog_run() requires *regs as the first parameter */
*(struct pt_regs **)¶m = 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 **)¶m = regs;
param.syscall_nr = rec->nr;
param.ret = rec->ret;
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-28 14:27 ` [PATCH bpf-next v2] " Yauheni Kaliuta
@ 2023-07-28 16:44 ` Yonghong Song
2023-07-31 8:07 ` Yauheni Kaliuta
2023-08-01 7:52 ` [PATCH bpf-next v3] " Yauheni Kaliuta
1 sibling, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2023-07-28 16:44 UTC (permalink / raw)
To: Yauheni Kaliuta, bpf; +Cc: andrii, ast
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 **)¶m = 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 **)¶m = regs;
> param.syscall_nr = rec->nr;
> param.ret = rec->ret;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-28 16:44 ` Yonghong Song
@ 2023-07-31 8:07 ` Yauheni Kaliuta
2023-07-31 18:20 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Yauheni Kaliuta @ 2023-07-31 8:07 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, andrii, ast
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 **)¶m = 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 **)¶m = regs;
>> param.syscall_nr = rec->nr;
>> param.ret = rec->ret;
[1] https://lore.kernel.org/bpf/xunyjzy64q9b.fsf@redhat.com/T/#u
--
WBR,
Yauheni Kaliuta
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-31 8:07 ` Yauheni Kaliuta
@ 2023-07-31 18:20 ` Yonghong Song
2023-08-01 7:49 ` Yauheni Kaliuta
0 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2023-07-31 18:20 UTC (permalink / raw)
To: Yauheni Kaliuta; +Cc: bpf, andrii, ast
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.
¶m) */
>
> >> *(struct pt_regs **)¶m = 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 **)¶m = regs;
> >> param.syscall_nr = rec->nr;
> >> param.ret = rec->ret;
>
>
> [1] https://lore.kernel.org/bpf/xunyjzy64q9b.fsf@redhat.com/T/#u
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-31 18:20 ` Yonghong Song
@ 2023-08-01 7:49 ` Yauheni Kaliuta
0 siblings, 0 replies; 11+ messages in thread
From: Yauheni Kaliuta @ 2023-08-01 7:49 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, andrii, ast
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. ¶m) */
Thanks!
--
WBR,
Yauheni Kaliuta
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next v3] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-07-28 14:27 ` [PATCH bpf-next v2] " Yauheni Kaliuta
2023-07-28 16:44 ` Yonghong Song
@ 2023-08-01 7:52 ` Yauheni Kaliuta
2023-08-01 14:31 ` Yonghong Song
2023-08-01 18:00 ` patchwork-bot+netdevbpf
1 sibling, 2 replies; 11+ messages in thread
From: Yauheni Kaliuta @ 2023-08-01 7:52 UTC (permalink / raw)
To: bpf; +Cc: andrii, ast, yonghong.song, Yauheni Kaliuta
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>
---
v3:
- Fixed comment
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..de753403cdaf 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 requires 'regs' to be the first member in the ctx (a.k.a. ¶m) */
*(struct pt_regs **)¶m = 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 requires 'regs' to be the first member in the ctx (a.k.a. ¶m) */
*(struct pt_regs **)¶m = regs;
param.syscall_nr = rec->nr;
param.ret = rec->ret;
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v3] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-08-01 7:52 ` [PATCH bpf-next v3] " Yauheni Kaliuta
@ 2023-08-01 14:31 ` Yonghong Song
2023-08-01 18:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 11+ messages in thread
From: Yonghong Song @ 2023-08-01 14:31 UTC (permalink / raw)
To: Yauheni Kaliuta, bpf; +Cc: andrii, ast
On 8/1/23 12:52 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>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v3] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
2023-08-01 7:52 ` [PATCH bpf-next v3] " Yauheni Kaliuta
2023-08-01 14:31 ` Yonghong Song
@ 2023-08-01 18:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-08-01 18:00 UTC (permalink / raw)
To: Yauheni Kaliuta; +Cc: bpf, andrii, ast, yonghong.song
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Tue, 1 Aug 2023 10:52:22 +0300 you 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>
>
> [...]
Here is the summary with links:
- [bpf-next,v3] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t
https://git.kernel.org/bpf/bpf-next/c/d3c4db86c711
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-08-01 18:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-27 15:06 [PATCH bpf-next] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t Yauheni Kaliuta
2023-07-27 17:37 ` Yonghong Song
2023-07-28 10:02 ` Yauheni Kaliuta
2023-07-28 14:27 ` [PATCH bpf-next v2] " Yauheni Kaliuta
2023-07-28 16:44 ` Yonghong Song
2023-07-31 8:07 ` Yauheni Kaliuta
2023-07-31 18:20 ` Yonghong Song
2023-08-01 7:49 ` Yauheni Kaliuta
2023-08-01 7:52 ` [PATCH bpf-next v3] " Yauheni Kaliuta
2023-08-01 14:31 ` Yonghong Song
2023-08-01 18:00 ` patchwork-bot+netdevbpf
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).