To simplify the stacktrace with pt_regs from kretprobe handler,
set the correct return address to the instruction pointer in
the pt_regs before calling kretprobe handlers.
Suggested-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
Tested-by: Andrii Nakryik <[email protected]>
---
Changes in v3:
- Cast the correct_ret_addr to unsigned long.
---
kernel/kprobes.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 54e5b89aad67..1598aca375c9 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
BUG_ON(1);
}
+ /* Set the instruction pointer to the correct address */
+ instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
+
/* Run them. */
first = current->kretprobe_instances.first;
while (first) {
On Wed, Jun 16, 2021 at 11:39:11PM -0500, Josh Poimboeuf wrote:
> On Thu, May 27, 2021 at 03:40:29PM +0900, Masami Hiramatsu wrote:
> > To simplify the stacktrace with pt_regs from kretprobe handler,
> > set the correct return address to the instruction pointer in
> > the pt_regs before calling kretprobe handlers.
> >
> > Suggested-by: Josh Poimboeuf <[email protected]>
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > Tested-by: Andrii Nakryik <[email protected]>
> > ---
> > Changes in v3:
> > - Cast the correct_ret_addr to unsigned long.
> > ---
> > kernel/kprobes.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 54e5b89aad67..1598aca375c9 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > BUG_ON(1);
> > }
> >
> > + /* Set the instruction pointer to the correct address */
> > + instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
> > +
> > /* Run them. */
> > first = current->kretprobe_instances.first;
> > while (first) {
> >
>
> Hi Masami,
>
> I know I suggested this patch, but I believe it would only be useful in
> combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> think that would be tricky to pull off correctly. Instead, we have
> UNWIND_HINT_FUNC, which is working fine.
>
> So I'd suggest dropping this patch, as the unwinder isn't actually
> reading regs->ip after all.
... and I guess this means patches 6-8 are no longer necessary.
--
Josh
On Thu, May 27, 2021 at 03:40:29PM +0900, Masami Hiramatsu wrote:
> To simplify the stacktrace with pt_regs from kretprobe handler,
> set the correct return address to the instruction pointer in
> the pt_regs before calling kretprobe handlers.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> Tested-by: Andrii Nakryik <[email protected]>
> ---
> Changes in v3:
> - Cast the correct_ret_addr to unsigned long.
> ---
> kernel/kprobes.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 54e5b89aad67..1598aca375c9 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> BUG_ON(1);
> }
>
> + /* Set the instruction pointer to the correct address */
> + instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
> +
> /* Run them. */
> first = current->kretprobe_instances.first;
> while (first) {
>
Hi Masami,
I know I suggested this patch, but I believe it would only be useful in
combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
think that would be tricky to pull off correctly. Instead, we have
UNWIND_HINT_FUNC, which is working fine.
So I'd suggest dropping this patch, as the unwinder isn't actually
reading regs->ip after all.
(I apologize for the churn, and my lack of responsiveness in general.
I've been working on moving to California, which is no small task with a
family, and in this housing market...)
--
Josh
On Wed, 16 Jun 2021 23:40:32 -0500
Josh Poimboeuf <[email protected]> wrote:
> On Wed, Jun 16, 2021 at 11:39:11PM -0500, Josh Poimboeuf wrote:
> > On Thu, May 27, 2021 at 03:40:29PM +0900, Masami Hiramatsu wrote:
> > > To simplify the stacktrace with pt_regs from kretprobe handler,
> > > set the correct return address to the instruction pointer in
> > > the pt_regs before calling kretprobe handlers.
> > >
> > > Suggested-by: Josh Poimboeuf <[email protected]>
> > > Signed-off-by: Masami Hiramatsu <[email protected]>
> > > Tested-by: Andrii Nakryik <[email protected]>
> > > ---
> > > Changes in v3:
> > > - Cast the correct_ret_addr to unsigned long.
> > > ---
> > > kernel/kprobes.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > index 54e5b89aad67..1598aca375c9 100644
> > > --- a/kernel/kprobes.c
> > > +++ b/kernel/kprobes.c
> > > @@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > > BUG_ON(1);
> > > }
> > >
> > > + /* Set the instruction pointer to the correct address */
> > > + instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
> > > +
> > > /* Run them. */
> > > first = current->kretprobe_instances.first;
> > > while (first) {
> > >
> >
> > Hi Masami,
> >
> > I know I suggested this patch, but I believe it would only be useful in
> > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > think that would be tricky to pull off correctly. Instead, we have
> > UNWIND_HINT_FUNC, which is working fine.
> >
> > So I'd suggest dropping this patch, as the unwinder isn't actually
> > reading regs->ip after all.
>
> ... and I guess this means patches 6-8 are no longer necessary.
OK, I also confirmed that dropping those patche does not make any change
on the stacktrace.
Let me update the series without those.
Thank you,
>
> --
> Josh
>
--
Masami Hiramatsu <[email protected]>
On Thu, 17 Jun 2021 23:40:01 +0900
Masami Hiramatsu <[email protected]> wrote:
> On Wed, 16 Jun 2021 23:40:32 -0500
> Josh Poimboeuf <[email protected]> wrote:
>
> > On Wed, Jun 16, 2021 at 11:39:11PM -0500, Josh Poimboeuf wrote:
> > > On Thu, May 27, 2021 at 03:40:29PM +0900, Masami Hiramatsu wrote:
> > > > To simplify the stacktrace with pt_regs from kretprobe handler,
> > > > set the correct return address to the instruction pointer in
> > > > the pt_regs before calling kretprobe handlers.
> > > >
> > > > Suggested-by: Josh Poimboeuf <[email protected]>
> > > > Signed-off-by: Masami Hiramatsu <[email protected]>
> > > > Tested-by: Andrii Nakryik <[email protected]>
> > > > ---
> > > > Changes in v3:
> > > > - Cast the correct_ret_addr to unsigned long.
> > > > ---
> > > > kernel/kprobes.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > index 54e5b89aad67..1598aca375c9 100644
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > > > BUG_ON(1);
> > > > }
> > > >
> > > > + /* Set the instruction pointer to the correct address */
> > > > + instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
> > > > +
> > > > /* Run them. */
> > > > first = current->kretprobe_instances.first;
> > > > while (first) {
> > > >
> > >
> > > Hi Masami,
> > >
> > > I know I suggested this patch, but I believe it would only be useful in
> > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > think that would be tricky to pull off correctly. Instead, we have
> > > UNWIND_HINT_FUNC, which is working fine.
> > >
> > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > reading regs->ip after all.
> >
> > ... and I guess this means patches 6-8 are no longer necessary.
>
> OK, I also confirmed that dropping those patche does not make any change
> on the stacktrace.
> Let me update the series without those.
Oops, Andrii, can you also test the kernel without this patch?
(you don't need to drop patch 6-8)
This changes the kretprobe to pass the return address via regs->ip to handler.
Dynamic-event doesn't use it, but I'm not sure bcc is using it or not.
Thank you,
>
> Thank you,
>
> >
> > --
> > Josh
> >
>
>
> --
> Masami Hiramatsu <[email protected]>
--
Masami Hiramatsu <[email protected]>
On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[email protected]> wrote:
>
> On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > >
> > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > reading regs->ip after all.
> > > > >
> > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > >
> > > > OK, I also confirmed that dropping those patche does not make any change
> > > > on the stacktrace.
> > > > Let me update the series without those.
> > >
> > > Oops, Andrii, can you also test the kernel without this patch?
> > > (you don't need to drop patch 6-8)
> >
> > Hi Masami,
> >
> > Dropping this patch and leaving all the other in place breaks stack
> > traces from kretprobes for BPF. I double checked with and without this
> > patch. Without this patch we are back to having broken stack traces. I
> > see either
> >
> > kretprobe_trampoline+0x0
> >
> > or
> >
> > ftrace_trampoline+0xc8
> > kretprobe_trampoline+0x0
> >
> > Is there any problem if you leave this patch as is?
>
> Hm, I must be missing something then. The patch is probably fine to
> keep, we just may need to improve the commit log so that it makes sense
> to me.
>
> Which unwinder are you using (CONFIG_UNWINDER_*)?
>
$ rg UNWINDER ~/linux-build/default/.config
5585:CONFIG_UNWINDER_ORC=y
5586:# CONFIG_UNWINDER_FRAME_POINTER is not set
5587:# CONFIG_UNWINDER_GUESS is not set
> --
> Josh
>
On Thu, Jun 17, 2021 at 8:02 AM Masami Hiramatsu <[email protected]> wrote:
>
> On Thu, 17 Jun 2021 23:40:01 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
> > On Wed, 16 Jun 2021 23:40:32 -0500
> > Josh Poimboeuf <[email protected]> wrote:
> >
> > > On Wed, Jun 16, 2021 at 11:39:11PM -0500, Josh Poimboeuf wrote:
> > > > On Thu, May 27, 2021 at 03:40:29PM +0900, Masami Hiramatsu wrote:
> > > > > To simplify the stacktrace with pt_regs from kretprobe handler,
> > > > > set the correct return address to the instruction pointer in
> > > > > the pt_regs before calling kretprobe handlers.
> > > > >
> > > > > Suggested-by: Josh Poimboeuf <[email protected]>
> > > > > Signed-off-by: Masami Hiramatsu <[email protected]>
> > > > > Tested-by: Andrii Nakryik <[email protected]>
> > > > > ---
> > > > > Changes in v3:
> > > > > - Cast the correct_ret_addr to unsigned long.
> > > > > ---
> > > > > kernel/kprobes.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > > index 54e5b89aad67..1598aca375c9 100644
> > > > > --- a/kernel/kprobes.c
> > > > > +++ b/kernel/kprobes.c
> > > > > @@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > > > > BUG_ON(1);
> > > > > }
> > > > >
> > > > > + /* Set the instruction pointer to the correct address */
> > > > > + instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
> > > > > +
> > > > > /* Run them. */
> > > > > first = current->kretprobe_instances.first;
> > > > > while (first) {
> > > > >
> > > >
> > > > Hi Masami,
> > > >
> > > > I know I suggested this patch, but I believe it would only be useful in
> > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > think that would be tricky to pull off correctly. Instead, we have
> > > > UNWIND_HINT_FUNC, which is working fine.
> > > >
> > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > reading regs->ip after all.
> > >
> > > ... and I guess this means patches 6-8 are no longer necessary.
> >
> > OK, I also confirmed that dropping those patche does not make any change
> > on the stacktrace.
> > Let me update the series without those.
>
> Oops, Andrii, can you also test the kernel without this patch?
> (you don't need to drop patch 6-8)
Hi Masami,
Dropping this patch and leaving all the other in place breaks stack
traces from kretprobes for BPF. I double checked with and without this
patch. Without this patch we are back to having broken stack traces. I
see either
kretprobe_trampoline+0x0
or
ftrace_trampoline+0xc8
kretprobe_trampoline+0x0
Is there any problem if you leave this patch as is?
> This changes the kretprobe to pass the return address via regs->ip to handler.
> Dynamic-event doesn't use it, but I'm not sure bcc is using it or not.
>
> Thank you,
>
> >
> > Thank you,
> >
> > >
> > > --
> > > Josh
> > >
> >
> >
> > --
> > Masami Hiramatsu <[email protected]>
>
>
> --
> Masami Hiramatsu <[email protected]>
On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > >
> > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > reading regs->ip after all.
> > > >
> > > > ... and I guess this means patches 6-8 are no longer necessary.
> > >
> > > OK, I also confirmed that dropping those patche does not make any change
> > > on the stacktrace.
> > > Let me update the series without those.
> >
> > Oops, Andrii, can you also test the kernel without this patch?
> > (you don't need to drop patch 6-8)
>
> Hi Masami,
>
> Dropping this patch and leaving all the other in place breaks stack
> traces from kretprobes for BPF. I double checked with and without this
> patch. Without this patch we are back to having broken stack traces. I
> see either
>
> kretprobe_trampoline+0x0
>
> or
>
> ftrace_trampoline+0xc8
> kretprobe_trampoline+0x0
>
> Is there any problem if you leave this patch as is?
Hm, I must be missing something then. The patch is probably fine to
keep, we just may need to improve the commit log so that it makes sense
to me.
Which unwinder are you using (CONFIG_UNWINDER_*)?
--
Josh
On Thu, Jun 17, 2021 at 11:31:03AM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[email protected]> wrote:
> >
> > On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > > >
> > > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > > reading regs->ip after all.
> > > > > >
> > > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > > >
> > > > > OK, I also confirmed that dropping those patche does not make any change
> > > > > on the stacktrace.
> > > > > Let me update the series without those.
> > > >
> > > > Oops, Andrii, can you also test the kernel without this patch?
> > > > (you don't need to drop patch 6-8)
> > >
> > > Hi Masami,
> > >
> > > Dropping this patch and leaving all the other in place breaks stack
> > > traces from kretprobes for BPF. I double checked with and without this
> > > patch. Without this patch we are back to having broken stack traces. I
> > > see either
> > >
> > > kretprobe_trampoline+0x0
> > >
> > > or
> > >
> > > ftrace_trampoline+0xc8
> > > kretprobe_trampoline+0x0
Do the stack traces end there? Or do they continue normally after that?
--
Josh
On Thu, Jun 17, 2021 at 12:26 PM Josh Poimboeuf <[email protected]> wrote:
>
> On Thu, Jun 17, 2021 at 11:31:03AM -0700, Andrii Nakryiko wrote:
> > On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[email protected]> wrote:
> > >
> > > On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > > > >
> > > > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > > > reading regs->ip after all.
> > > > > > >
> > > > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > > > >
> > > > > > OK, I also confirmed that dropping those patche does not make any change
> > > > > > on the stacktrace.
> > > > > > Let me update the series without those.
> > > > >
> > > > > Oops, Andrii, can you also test the kernel without this patch?
> > > > > (you don't need to drop patch 6-8)
> > > >
> > > > Hi Masami,
> > > >
> > > > Dropping this patch and leaving all the other in place breaks stack
> > > > traces from kretprobes for BPF. I double checked with and without this
> > > > patch. Without this patch we are back to having broken stack traces. I
> > > > see either
> > > >
> > > > kretprobe_trampoline+0x0
> > > >
> > > > or
> > > >
> > > > ftrace_trampoline+0xc8
> > > > kretprobe_trampoline+0x0
>
> Do the stack traces end there? Or do they continue normally after that?
That's the entire stack trace.
>
> --
> Josh
>
On Thu, 17 Jun 2021 13:21:59 -0500
Josh Poimboeuf <[email protected]> wrote:
> On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > >
> > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > reading regs->ip after all.
> > > > >
> > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > >
> > > > OK, I also confirmed that dropping those patche does not make any change
> > > > on the stacktrace.
> > > > Let me update the series without those.
> > >
> > > Oops, Andrii, can you also test the kernel without this patch?
> > > (you don't need to drop patch 6-8)
> >
> > Hi Masami,
> >
> > Dropping this patch and leaving all the other in place breaks stack
> > traces from kretprobes for BPF. I double checked with and without this
> > patch. Without this patch we are back to having broken stack traces. I
> > see either
> >
> > kretprobe_trampoline+0x0
> >
> > or
> >
> > ftrace_trampoline+0xc8
> > kretprobe_trampoline+0x0
Thanks for confirmation.
> >
> > Is there any problem if you leave this patch as is?
>
> Hm, I must be missing something then. The patch is probably fine to
> keep, we just may need to improve the commit log so that it makes sense
> to me.
Yeah, I need to update the commit message so that this will help
the stacktrace from kretprobe's pt_regs, which will be used in bpf.
Thank you!
>
> Which unwinder are you using (CONFIG_UNWINDER_*)?
>
> --
> Josh
>
--
Masami Hiramatsu <[email protected]>
On Thu, 17 Jun 2021 12:46:19 -0700
Andrii Nakryiko <[email protected]> wrote:
> On Thu, Jun 17, 2021 at 12:26 PM Josh Poimboeuf <[email protected]> wrote:
> >
> > On Thu, Jun 17, 2021 at 11:31:03AM -0700, Andrii Nakryiko wrote:
> > > On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[email protected]> wrote:
> > > >
> > > > On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > > > > >
> > > > > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > > > > reading regs->ip after all.
> > > > > > > >
> > > > > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > > > > >
> > > > > > > OK, I also confirmed that dropping those patche does not make any change
> > > > > > > on the stacktrace.
> > > > > > > Let me update the series without those.
> > > > > >
> > > > > > Oops, Andrii, can you also test the kernel without this patch?
> > > > > > (you don't need to drop patch 6-8)
> > > > >
> > > > > Hi Masami,
> > > > >
> > > > > Dropping this patch and leaving all the other in place breaks stack
> > > > > traces from kretprobes for BPF. I double checked with and without this
> > > > > patch. Without this patch we are back to having broken stack traces. I
> > > > > see either
> > > > >
> > > > > kretprobe_trampoline+0x0
> > > > >
> > > > > or
> > > > >
> > > > > ftrace_trampoline+0xc8
> > > > > kretprobe_trampoline+0x0
> >
> > Do the stack traces end there? Or do they continue normally after that?
>
> That's the entire stack trace.
So, there are 2 cases of the stacktrace from inside the kretprobe handler.
1) Call stack_trace_save() in the handler. This will unwind stack from the
handler's context. This is the case of the ftrace dynamic events.
2) Call stack_trace_save_regs(regs) in the handler with the pt_regs passed
by the kretprobe. This is the case of ebpf.
For the case 1, these patches can be dropped because ORC can unwind the
stack with UNWIND_HINT_FUNC. For the case 2, regs->ip must be set to the
correct (return) address so that ORC can find the correct entry from that
ip.
Thank you,
--
Masami Hiramatsu <[email protected]>
On Fri, Jun 18, 2021 at 09:33:13AM +0900, Masami Hiramatsu wrote:
> On Thu, 17 Jun 2021 12:46:19 -0700
> Andrii Nakryiko <[email protected]> wrote:
>
> > On Thu, Jun 17, 2021 at 12:26 PM Josh Poimboeuf <[email protected]> wrote:
> > >
> > > On Thu, Jun 17, 2021 at 11:31:03AM -0700, Andrii Nakryiko wrote:
> > > > On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > > > > > >
> > > > > > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > > > > > reading regs->ip after all.
> > > > > > > > >
> > > > > > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > > > > > >
> > > > > > > > OK, I also confirmed that dropping those patche does not make any change
> > > > > > > > on the stacktrace.
> > > > > > > > Let me update the series without those.
> > > > > > >
> > > > > > > Oops, Andrii, can you also test the kernel without this patch?
> > > > > > > (you don't need to drop patch 6-8)
> > > > > >
> > > > > > Hi Masami,
> > > > > >
> > > > > > Dropping this patch and leaving all the other in place breaks stack
> > > > > > traces from kretprobes for BPF. I double checked with and without this
> > > > > > patch. Without this patch we are back to having broken stack traces. I
> > > > > > see either
> > > > > >
> > > > > > kretprobe_trampoline+0x0
> > > > > >
> > > > > > or
> > > > > >
> > > > > > ftrace_trampoline+0xc8
> > > > > > kretprobe_trampoline+0x0
> > >
> > > Do the stack traces end there? Or do they continue normally after that?
> >
> > That's the entire stack trace.
>
> So, there are 2 cases of the stacktrace from inside the kretprobe handler.
>
> 1) Call stack_trace_save() in the handler. This will unwind stack from the
> handler's context. This is the case of the ftrace dynamic events.
>
> 2) Call stack_trace_save_regs(regs) in the handler with the pt_regs passed
> by the kretprobe. This is the case of ebpf.
>
> For the case 1, these patches can be dropped because ORC can unwind the
> stack with UNWIND_HINT_FUNC. For the case 2, regs->ip must be set to the
> correct (return) address so that ORC can find the correct entry from that
> ip.
Agreed! I get it now. Thanks :-)
--
Josh
On Fri, Jun 18, 2021 at 08:58:11AM +0900, Masami Hiramatsu wrote:
> On Thu, 17 Jun 2021 13:21:59 -0500
> Josh Poimboeuf <[email protected]> wrote:
>
> > On Thu, Jun 17, 2021 at 10:45:41AM -0700, Andrii Nakryiko wrote:
> > > > > > > I know I suggested this patch, but I believe it would only be useful in
> > > > > > > combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I
> > > > > > > think that would be tricky to pull off correctly. Instead, we have
> > > > > > > UNWIND_HINT_FUNC, which is working fine.
> > > > > > >
> > > > > > > So I'd suggest dropping this patch, as the unwinder isn't actually
> > > > > > > reading regs->ip after all.
> > > > > >
> > > > > > ... and I guess this means patches 6-8 are no longer necessary.
> > > > >
> > > > > OK, I also confirmed that dropping those patche does not make any change
> > > > > on the stacktrace.
> > > > > Let me update the series without those.
> > > >
> > > > Oops, Andrii, can you also test the kernel without this patch?
> > > > (you don't need to drop patch 6-8)
> > >
> > > Hi Masami,
> > >
> > > Dropping this patch and leaving all the other in place breaks stack
> > > traces from kretprobes for BPF. I double checked with and without this
> > > patch. Without this patch we are back to having broken stack traces. I
> > > see either
> > >
> > > kretprobe_trampoline+0x0
> > >
> > > or
> > >
> > > ftrace_trampoline+0xc8
> > > kretprobe_trampoline+0x0
>
> Thanks for confirmation.
>
> > >
> > > Is there any problem if you leave this patch as is?
> >
> > Hm, I must be missing something then. The patch is probably fine to
> > keep, we just may need to improve the commit log so that it makes sense
> > to me.
>
> Yeah, I need to update the commit message so that this will help
> the stacktrace from kretprobe's pt_regs, which will be used in bpf.
Yes, I presume it's because when bpf unwinds from the kretprobe regs,
the unwinder starts from regs->ip, which is otherwise undefined because
it's skipped by SAVE_REGS_STRING.
--
Josh