| Message ID | 20231111172001.1259065-1-linux@jordanrome.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] perf: get_perf_callchain return NULL for crosstask | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sat, Nov 11, 2023 at 9:20 AM Jordan Rome <linux@jordanrome.com> wrote: > > Return NULL instead of returning 1 incorrect frame, which > currently happens when trying to walk the user stack for > any task that isn't current. Returning NULL is a better > indicator that this behavior is not supported. > > This issue was found using bpf_get_task_stack inside a BPF > iterator ("iter/task"), which iterates over all tasks. The > single address/frame in the buffer when getting user stacks > for tasks that aren't current could not be symbolized (testing > multiple symbolizers). > > Signed-off-by: Jordan Rome <linux@jordanrome.com> > --- > > Changes in v2: > * move user and crosstask check before get_callchain_entry > LGTM. Acked-by: Andrii Nakryiko <andrii@kernel.org> > v1: > https://lore.kernel.org/linux-perf-users/CAEf4BzaWtOeTBb_+b7Td3NHaKjZU+OohuBJje_nvw9kd6xPA3g@mail.gmail.com/T/#t > > kernel/events/callchain.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c > index 1273be84392c..104ea2975a57 100644 > --- a/kernel/events/callchain.c > +++ b/kernel/events/callchain.c > @@ -184,6 +184,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > struct perf_callchain_entry_ctx ctx; > int rctx; > > + if (user && crosstask) > + return NULL; > + > entry = get_callchain_entry(&rctx); > if (!entry) > return NULL; > @@ -209,9 +212,6 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > } > > if (regs) { > - if (crosstask) > - goto exit_put; > - > if (add_mark) > perf_callchain_store_context(&ctx, PERF_CONTEXT_USER); > > @@ -219,7 +219,6 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > } > } > > -exit_put: > put_callchain_entry(rctx); > > return entry; > -- > 2.39.3 >
On Sat, Nov 11, 2023 at 09:20:01AM -0800, Jordan Rome wrote: > Return NULL instead of returning 1 incorrect frame, which > currently happens when trying to walk the user stack for > any task that isn't current. Returning NULL is a better > indicator that this behavior is not supported. > > This issue was found using bpf_get_task_stack inside a BPF > iterator ("iter/task"), which iterates over all tasks. The > single address/frame in the buffer when getting user stacks > for tasks that aren't current could not be symbolized (testing > multiple symbolizers). > > Signed-off-by: Jordan Rome <linux@jordanrome.com> Acked-by: Jiri Olsa <jolsa@kernel.org> jirka > --- > > Changes in v2: > * move user and crosstask check before get_callchain_entry > > v1: > https://lore.kernel.org/linux-perf-users/CAEf4BzaWtOeTBb_+b7Td3NHaKjZU+OohuBJje_nvw9kd6xPA3g@mail.gmail.com/T/#t > > kernel/events/callchain.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c > index 1273be84392c..104ea2975a57 100644 > --- a/kernel/events/callchain.c > +++ b/kernel/events/callchain.c > @@ -184,6 +184,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > struct perf_callchain_entry_ctx ctx; > int rctx; > > + if (user && crosstask) > + return NULL; > + > entry = get_callchain_entry(&rctx); > if (!entry) > return NULL; > @@ -209,9 +212,6 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > } > > if (regs) { > - if (crosstask) > - goto exit_put; > - > if (add_mark) > perf_callchain_store_context(&ctx, PERF_CONTEXT_USER); > > @@ -219,7 +219,6 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > } > } > > -exit_put: > put_callchain_entry(rctx); > > return entry; > -- > 2.39.3 > >
diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c index 1273be84392c..104ea2975a57 100644 --- a/kernel/events/callchain.c +++ b/kernel/events/callchain.c @@ -184,6 +184,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, struct perf_callchain_entry_ctx ctx; int rctx; + if (user && crosstask) + return NULL; + entry = get_callchain_entry(&rctx); if (!entry) return NULL; @@ -209,9 +212,6 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, } if (regs) { - if (crosstask) - goto exit_put; - if (add_mark) perf_callchain_store_context(&ctx, PERF_CONTEXT_USER); @@ -219,7 +219,6 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, } } -exit_put: put_callchain_entry(rctx); return entry;
Return NULL instead of returning 1 incorrect frame, which currently happens when trying to walk the user stack for any task that isn't current. Returning NULL is a better indicator that this behavior is not supported. This issue was found using bpf_get_task_stack inside a BPF iterator ("iter/task"), which iterates over all tasks. The single address/frame in the buffer when getting user stacks for tasks that aren't current could not be symbolized (testing multiple symbolizers). Signed-off-by: Jordan Rome <linux@jordanrome.com> --- Changes in v2: * move user and crosstask check before get_callchain_entry v1: https://lore.kernel.org/linux-perf-users/CAEf4BzaWtOeTBb_+b7Td3NHaKjZU+OohuBJje_nvw9kd6xPA3g@mail.gmail.com/T/#t kernel/events/callchain.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)