| Message ID | 20231110235021.192796-1-linux@jordanrome.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [perf] perf: get_perf_callchain return NULL for crosstask | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Nov 10, 2023 at 3:51 PM 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> > --- > 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..430fa544fa80 100644 > --- a/kernel/events/callchain.c > +++ b/kernel/events/callchain.c > @@ -201,6 +201,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > } > > if (user) { > + if (crosstask) > + return NULL; I think you need that goto exit_put here. > + > if (!user_mode(regs)) { > if (current->mm) > regs = task_pt_regs(current); > @@ -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 Fri, Nov 10, 2023 at 7:10 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Nov 10, 2023 at 3:51 PM 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> > > --- > > 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..430fa544fa80 100644 > > --- a/kernel/events/callchain.c > > +++ b/kernel/events/callchain.c > > @@ -201,6 +201,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > > } > > > > if (user) { > > + if (crosstask) > > + return NULL; > > I think you need that goto exit_put here. > Why is that? Wouldn't that be the same behavior that already exists? That being said we can probably move this check above get_callchain_entry and exit earlier. > > + > > if (!user_mode(regs)) { > > if (current->mm) > > regs = task_pt_regs(current); > > @@ -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 Fri, Nov 10, 2023 at 4:43 PM Jordan Rome <linux@jordanrome.com> wrote: > > On Fri, Nov 10, 2023 at 7:10 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Nov 10, 2023 at 3:51 PM 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> > > > --- > > > 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..430fa544fa80 100644 > > > --- a/kernel/events/callchain.c > > > +++ b/kernel/events/callchain.c > > > @@ -201,6 +201,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > > > } > > > > > > if (user) { > > > + if (crosstask) > > > + return NULL; > > > > I think you need that goto exit_put here. > > > > Why is that? Wouldn't that be the same behavior that already exists? > That being said we can probably move this check above get_callchain_entry > and exit earlier. If I read the code right, get_callchain_entry() does expect put_callchain_entry(), which you are breaking with this return NULL. But indeed, checking it early and bailing out might be the best solution here. > > > > + > > > if (!user_mode(regs)) { > > > if (current->mm) > > > regs = task_pt_regs(current); > > > @@ -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 Fri, Nov 10, 2023 at 11:32 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Nov 10, 2023 at 4:43 PM Jordan Rome <linux@jordanrome.com> wrote: > > > > On Fri, Nov 10, 2023 at 7:10 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Fri, Nov 10, 2023 at 3:51 PM 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> > > > > --- > > > > 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..430fa544fa80 100644 > > > > --- a/kernel/events/callchain.c > > > > +++ b/kernel/events/callchain.c > > > > @@ -201,6 +201,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > > > > } > > > > > > > > if (user) { > > > > + if (crosstask) > > > > + return NULL; > > > > > > I think you need that goto exit_put here. > > > > > > > Why is that? Wouldn't that be the same behavior that already exists? > > That being said we can probably move this check above get_callchain_entry > > and exit earlier. > > If I read the code right, get_callchain_entry() does expect > put_callchain_entry(), which you are breaking with this return NULL. > > But indeed, checking it early and bailing out might be the best solution here. > Sounds good. I'll move this check before `get_callchain_entry`. > > > > > > + > > > > if (!user_mode(regs)) { > > > > if (current->mm) > > > > regs = task_pt_regs(current); > > > > @@ -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..430fa544fa80 100644 --- a/kernel/events/callchain.c +++ b/kernel/events/callchain.c @@ -201,6 +201,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, } if (user) { + if (crosstask) + return NULL; + if (!user_mode(regs)) { if (current->mm) regs = task_pt_regs(current); @@ -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> --- kernel/events/callchain.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)