[-tip,v7,09/13] kprobes: Setup instruction pointer in __kretprobe_trampoline_handler

Commit Message

Masami Hiramatsu (Google) May 27, 2021, 6:40 a.m. UTC

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 <[e-mail]>
Signed-off-by: Masami Hiramatsu <[e-mail]>
Tested-by: Andrii Nakryik <[e-mail]>
---
 Changes in v3:
  - Cast the correct_ret_addr to unsigned long.
---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

Comments

Josh Poimboeuf June 17, 2021, 4:39 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Masami Hiramatsu <[e-mail]>
> Tested-by: Andrii Nakryik <[e-mail]>
> ---
>  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 Poimboeuf June 17, 2021, 4:40 a.m. UTC | #2

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 <[e-mail]>
> > Signed-off-by: Masami Hiramatsu <[e-mail]>
> > Tested-by: Andrii Nakryik <[e-mail]>
> > ---
> >  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.

Masami Hiramatsu (Google) June 17, 2021, 2:40 p.m. UTC | #3

On Wed, 16 Jun 2021 23:40:32 -0500
Josh Poimboeuf <[e-mail]> 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 <[e-mail]>
> > > Signed-off-by: Masami Hiramatsu <[e-mail]>
> > > Tested-by: Andrii Nakryik <[e-mail]>
> > > ---
> > >  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 (Google) June 17, 2021, 3:02 p.m. UTC | #4

On Thu, 17 Jun 2021 23:40:01 +0900
Masami Hiramatsu <[e-mail]> wrote:

> On Wed, 16 Jun 2021 23:40:32 -0500
> Josh Poimboeuf <[e-mail]> 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 <[e-mail]>
> > > > Signed-off-by: Masami Hiramatsu <[e-mail]>
> > > > Tested-by: Andrii Nakryik <[e-mail]>
> > > > ---
> > > >  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 <[e-mail]>

Andrii Nakryiko June 17, 2021, 5:45 p.m. UTC | #5

On Thu, Jun 17, 2021 at 8:02 AM Masami Hiramatsu <[e-mail]> wrote:
>
> On Thu, 17 Jun 2021 23:40:01 +0900
> Masami Hiramatsu <[e-mail]> wrote:
>
> > On Wed, 16 Jun 2021 23:40:32 -0500
> > Josh Poimboeuf <[e-mail]> 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 <[e-mail]>
> > > > > Signed-off-by: Masami Hiramatsu <[e-mail]>
> > > > > Tested-by: Andrii Nakryik <[e-mail]>
> > > > > ---
> > > > >  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 <[e-mail]>
>
>
> --
> Masami Hiramatsu <[e-mail]>

Josh Poimboeuf June 17, 2021, 6:21 p.m. UTC | #6

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_*)?

Andrii Nakryiko June 17, 2021, 6:31 p.m. UTC | #7

On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[e-mail]> 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
>

Josh Poimboeuf June 17, 2021, 7:26 p.m. UTC | #8

On Thu, Jun 17, 2021 at 11:31:03AM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 17, 2021 at 11:22 AM Josh Poimboeuf <[e-mail]> 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?

Andrii Nakryiko June 17, 2021, 7:46 p.m. UTC | #9

On Thu, Jun 17, 2021 at 12:26 PM Josh Poimboeuf <[e-mail]> 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 <[e-mail]> 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
>

Masami Hiramatsu (Google) June 17, 2021, 11:58 p.m. UTC | #10

On Thu, 17 Jun 2021 13:21:59 -0500
Josh Poimboeuf <[e-mail]> 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 (Google) June 18, 2021, 12:33 a.m. UTC | #11

On Thu, 17 Jun 2021 12:46:19 -0700
Andrii Nakryiko <[e-mail]> wrote:

> On Thu, Jun 17, 2021 at 12:26 PM Josh Poimboeuf <[e-mail]> 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 <[e-mail]> 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,

Josh Poimboeuf June 18, 2021, 12:58 a.m. UTC | #12

On Fri, Jun 18, 2021 at 08:58:11AM +0900, Masami Hiramatsu wrote:
> On Thu, 17 Jun 2021 13:21:59 -0500
> Josh Poimboeuf <[e-mail]> 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 Poimboeuf June 18, 2021, 1:03 a.m. UTC | #13

On Fri, Jun 18, 2021 at 09:33:13AM +0900, Masami Hiramatsu wrote:
> On Thu, 17 Jun 2021 12:46:19 -0700
> Andrii Nakryiko <[e-mail]> wrote:
> 
> > On Thu, Jun 17, 2021 at 12:26 PM Josh Poimboeuf <[e-mail]> 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 <[e-mail]> 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 :-)

Patch

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) {