Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580463 > unrolled thread

Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of  multiple kretprobes "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2017-02-14 11:40 +0100
    Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of  multiple kretprobes "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2017-02-14 14:50 +0100
      Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of  multiple kretprobes Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-14 17:10 +0100
        Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of  multiple kretprobes "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2017-02-14 17:50 +0100
          Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of  multiple kretprobes Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-15 01:00 +0100

#1580463 — Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> This is arm port of commit 737480a0d525 ("kprobes/x86:
> Fix the return address of multiple kretprobes").
> 
> Fix the return address of subsequent kretprobes when multiple
> kretprobes are set on the same function.
> 
> For example:
> 
>   # cd /sys/kernel/debug/tracing
>   # echo "r:event1 sys_symlink" > kprobe_events
>   # echo "r:event2 sys_symlink" >> kprobe_events
>   # echo 1 > events/kprobes/enable
>   # ln -s /tmp/foo /tmp/bar
> 
>  (without this patch)
> 
>   # cat trace | grep -v ^#
>               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
>               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> 
>  (with this patch)
> 
>   # cat trace | grep -v ^#
>               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
>               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> ---

I don't fully understand this function, but I've checked that the ARM
version now matches the x86 version (apart from the x86 specific
register fixup and some comments). So, FWIW

Acked-by: Jon Medhurst <tixy@linaro.org>

I ran the before and after test case in the commit log on ARM and
verified the result is correct. I also tried running the ARM kprobe
tests with these 3 fixes but the tests fail. However, they also fail
without any of these changes, so I'll investigate that further...
  
>  arch/arm/probes/kprobes/core.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index 84989ae..023800a 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -440,6 +440,7 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  	struct hlist_node *tmp;
>  	unsigned long flags, orig_ret_address = 0;
>  	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
> +	kprobe_opcode_t *correct_ret_addr = NULL;
>  
>  	INIT_HLIST_HEAD(&empty_rp);
>  	kretprobe_hash_lock(current, &head, &flags);
> @@ -462,14 +463,34 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  			/* another task is sharing our hash bucket */
>  			continue;
>  
> +		orig_ret_address = (unsigned long)ri->ret_addr;
> +
> +		if (orig_ret_address != trampoline_address)
> +			/*
> +			 * This is the real return address. Any other
> +			 * instances associated with this task are for
> +			 * other calls deeper on the call stack
> +			 */
> +			break;
> +	}
> +
> +	kretprobe_assert(ri, orig_ret_address, trampoline_address);
> +
> +	correct_ret_addr = ri->ret_addr;
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->task != current)
> +			/* another task is sharing our hash bucket */
> +			continue;
> +
> +		orig_ret_address = (unsigned long)ri->ret_addr;
>  		if (ri->rp && ri->rp->handler) {
>  			__this_cpu_write(current_kprobe, &ri->rp->kp);
>  			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
> +			ri->ret_addr = correct_ret_addr;
>  			ri->rp->handler(ri, regs);
>  			__this_cpu_write(current_kprobe, NULL);
>  		}
>  
> -		orig_ret_address = (unsigned long)ri->ret_addr;
>  		recycle_rp_inst(ri, &empty_rp);
>  
>  		if (orig_ret_address != trampoline_address)
> @@ -481,7 +502,6 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  			break;
>  	}
>  
> -	kretprobe_assert(ri, orig_ret_address, trampoline_address);
>  	kretprobe_hash_unlock(current, &flags);
>  
>  	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> 

[toc] | [next] | [standalone]

#1580551

On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > Fix the return address of multiple kretprobes").
> > 
> > Fix the return address of subsequent kretprobes when multiple
> > kretprobes are set on the same function.
> > 
> > For example:
> > 
> >   # cd /sys/kernel/debug/tracing
> >   # echo "r:event1 sys_symlink" > kprobe_events
> >   # echo "r:event2 sys_symlink" >> kprobe_events
> >   # echo 1 > events/kprobes/enable
> >   # ln -s /tmp/foo /tmp/bar
> > 
> >  (without this patch)
> > 
> >   # cat trace | grep -v ^#
> >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > 
> >  (with this patch)
> > 
> >   # cat trace | grep -v ^#
> >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > ---
> 
> I don't fully understand this function, but I've checked that the ARM
> version now matches the x86 version (apart from the x86 specific
> register fixup and some comments). So, FWIW
> 
> Acked-by: Jon Medhurst <tixy@linaro.org>
> 
> I ran the before and after test case in the commit log on ARM and
> verified the result is correct. I also tried running the ARM kprobe
> tests with these 3 fixes but the tests fail. However, they also fail
> without any of these changes, so I'll investigate that further...

Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")

This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
that on 4.10-rc4, with the three patches in this series, then the ARM
kprobes tests pass OK.

I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
a year, so can probably wait a little longer).

-- 
Tixy

[toc] | [prev] | [next] | [standalone]

#1580628

On Tue, 14 Feb 2017 13:47:07 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> > On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > > Fix the return address of multiple kretprobes").
> > > 
> > > Fix the return address of subsequent kretprobes when multiple
> > > kretprobes are set on the same function.
> > > 
> > > For example:
> > > 
> > >   # cd /sys/kernel/debug/tracing
> > >   # echo "r:event1 sys_symlink" > kprobe_events
> > >   # echo "r:event2 sys_symlink" >> kprobe_events
> > >   # echo 1 > events/kprobes/enable
> > >   # ln -s /tmp/foo /tmp/bar
> > > 
> > >  (without this patch)
> > > 
> > >   # cat trace | grep -v ^#
> > >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> > >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > 
> > >  (with this patch)
> > > 
> > >   # cat trace | grep -v ^#
> > >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > 
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > > ---
> > 
> > I don't fully understand this function, but I've checked that the ARM
> > version now matches the x86 version (apart from the x86 specific
> > register fixup and some comments). So, FWIW
> > 
> > Acked-by: Jon Medhurst <tixy@linaro.org>
> > 
> > I ran the before and after test case in the commit log on ARM and
> > verified the result is correct. I also tried running the ARM kprobe
> > tests with these 3 fixes but the tests fail. However, they also fail
> > without any of these changes, so I'll investigate that further...
> 
> Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
> ("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")
> 
> This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
> that on 4.10-rc4, with the three patches in this series, then the ARM
> kprobes tests pass OK.
> 
> I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
> a year, so can probably wait a little longer).

Hmm, I'm running arm kernel on qemu, which maybe the reason why
the test case passed in my environment, since my kconfig also sets
CONFIG_DEBUG_RODATA=y.

BTW, would you see that any kprobe_events didn't work with
CONFIG_DEBUG_RODATA=y? (what the failure messages were?)

Thank you,

> 
> -- 
> Tixy
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1580658

On Wed, 2017-02-15 at 01:01 +0900, Masami Hiramatsu wrote:
> On Tue, 14 Feb 2017 13:47:07 +0000
> "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
> 
> > On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> > > On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > > > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > > > Fix the return address of multiple kretprobes").
> > > > 
> > > > Fix the return address of subsequent kretprobes when multiple
> > > > kretprobes are set on the same function.
> > > > 
> > > > For example:
> > > > 
> > > >   # cd /sys/kernel/debug/tracing
> > > >   # echo "r:event1 sys_symlink" > kprobe_events
> > > >   # echo "r:event2 sys_symlink" >> kprobe_events
> > > >   # echo 1 > events/kprobes/enable
> > > >   # ln -s /tmp/foo /tmp/bar
> > > > 
> > > >  (without this patch)
> > > > 
> > > >   # cat trace | grep -v ^#
> > > >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> > > >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > 
> > > >  (with this patch)
> > > > 
> > > >   # cat trace | grep -v ^#
> > > >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > 
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > > > ---
> > > 
> > > I don't fully understand this function, but I've checked that the ARM
> > > version now matches the x86 version (apart from the x86 specific
> > > register fixup and some comments). So, FWIW
> > > 
> > > Acked-by: Jon Medhurst <tixy@linaro.org>
> > > 
> > > I ran the before and after test case in the commit log on ARM and
> > > verified the result is correct. I also tried running the ARM kprobe
> > > tests with these 3 fixes but the tests fail. However, they also fail
> > > without any of these changes, so I'll investigate that further...
> > 
> > Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
> > ("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")
> > 
> > This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
> > that on 4.10-rc4, with the three patches in this series, then the ARM
> > kprobes tests pass OK.
> > 
> > I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
> > a year, so can probably wait a little longer).
> 
> Hmm, I'm running arm kernel on qemu, which maybe the reason why
> the test case passed in my environment, since my kconfig also sets
> CONFIG_DEBUG_RODATA=y.
> 
> BTW, would you see that any kprobe_events didn't work with
> CONFIG_DEBUG_RODATA=y? (what the failure messages were?)

The tests I'm running are the ARM specific tests that are enabled by
CONFIG_ARM_KPROBES_TEST=y. I'm running the tests on real multicore ARM
hardware (Versatile Express with a TC2 CoreTile)

For me, sometimes the first test gave:

    Beginning kprobe tests...
    Probe ARM code
        kprobe
    FAIL: test regs not OK

Other times, for the specific instruction emulation tests they return

   FAIL: test_before_handler not run

Not sure how much of the diagnostic appear without setting the tests to
be verbose, which I do with:

  sed -e 's/VERBOSE 0/VERBOSE 1/' -i arch/arm/probes/kprobes/test-core.h

Whilst writing a reply, I looked at the test code in
arch/arm/probes/kprobes/test-core.c (which I wrote some years ago) and
there is possibly a clue staring at us in the comments at the top of the
file...

 *
 * The above would expand to assembler looking something like:
 *
 *	@ TESTCASE_START
 *	bl	__kprobes_test_case_start
 *	.pushsection .rodata
 *	"10:
 *	.ascii "mov r0, r7"	@ text title for test case
 *	.byte	0
 *	.popsection
 *	@ start of inline data...
 *	.word	10b		@ pointer to title in .rodata
section

Note the ".pushsection .rodata" (though I don't see an immediate obvious
reason why that would cause a problem. It certainly seems likely that
the problem is with the ARM test code rather than actual kprobe
implementation itself.

Like I said, this issue has been there for a year or more, so I wasn't
planning on spending time on it for a few more days yet whilst I get on
with other urgent matters.

-- 
Tixy


Basically, m

[toc] | [prev] | [next] | [standalone]

#1580951

On Tue, 14 Feb 2017 16:39:50 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Wed, 2017-02-15 at 01:01 +0900, Masami Hiramatsu wrote:
> > On Tue, 14 Feb 2017 13:47:07 +0000
> > "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
> > 
> > > On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> > > > On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > > > > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > > > > Fix the return address of multiple kretprobes").
> > > > > 
> > > > > Fix the return address of subsequent kretprobes when multiple
> > > > > kretprobes are set on the same function.
> > > > > 
> > > > > For example:
> > > > > 
> > > > >   # cd /sys/kernel/debug/tracing
> > > > >   # echo "r:event1 sys_symlink" > kprobe_events
> > > > >   # echo "r:event2 sys_symlink" >> kprobe_events
> > > > >   # echo 1 > events/kprobes/enable
> > > > >   # ln -s /tmp/foo /tmp/bar
> > > > > 
> > > > >  (without this patch)
> > > > > 
> > > > >   # cat trace | grep -v ^#
> > > > >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> > > > >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > > 
> > > > >  (with this patch)
> > > > > 
> > > > >   # cat trace | grep -v ^#
> > > > >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > > 
> > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > > > > ---
> > > > 
> > > > I don't fully understand this function, but I've checked that the ARM
> > > > version now matches the x86 version (apart from the x86 specific
> > > > register fixup and some comments). So, FWIW
> > > > 
> > > > Acked-by: Jon Medhurst <tixy@linaro.org>
> > > > 
> > > > I ran the before and after test case in the commit log on ARM and
> > > > verified the result is correct. I also tried running the ARM kprobe
> > > > tests with these 3 fixes but the tests fail. However, they also fail
> > > > without any of these changes, so I'll investigate that further...
> > > 
> > > Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
> > > ("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")
> > > 
> > > This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
> > > that on 4.10-rc4, with the three patches in this series, then the ARM
> > > kprobes tests pass OK.
> > > 
> > > I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
> > > a year, so can probably wait a little longer).
> > 
> > Hmm, I'm running arm kernel on qemu, which maybe the reason why
> > the test case passed in my environment, since my kconfig also sets
> > CONFIG_DEBUG_RODATA=y.
> > 
> > BTW, would you see that any kprobe_events didn't work with
> > CONFIG_DEBUG_RODATA=y? (what the failure messages were?)
> 
> The tests I'm running are the ARM specific tests that are enabled by
> CONFIG_ARM_KPROBES_TEST=y. I'm running the tests on real multicore ARM
> hardware (Versatile Express with a TC2 CoreTile)

Ah, I didn't enabled it. I'll also try to run it.

> 
> For me, sometimes the first test gave:
> 
>     Beginning kprobe tests...
>     Probe ARM code
>         kprobe
>     FAIL: test regs not OK
> 
> Other times, for the specific instruction emulation tests they return
> 
>    FAIL: test_before_handler not run
> 
> Not sure how much of the diagnostic appear without setting the tests to
> be verbose, which I do with:
> 
>   sed -e 's/VERBOSE 0/VERBOSE 1/' -i arch/arm/probes/kprobes/test-core.h
> 
> Whilst writing a reply, I looked at the test code in
> arch/arm/probes/kprobes/test-core.c (which I wrote some years ago) and
> there is possibly a clue staring at us in the comments at the top of the
> file...
> 
>  *
>  * The above would expand to assembler looking something like:
>  *
>  *	@ TESTCASE_START
>  *	bl	__kprobes_test_case_start
>  *	.pushsection .rodata
>  *	"10:
>  *	.ascii "mov r0, r7"	@ text title for test case
>  *	.byte	0
>  *	.popsection
>  *	@ start of inline data...
>  *	.word	10b		@ pointer to title in .rodata
> section
> 
> Note the ".pushsection .rodata" (though I don't see an immediate obvious
> reason why that would cause a problem. It certainly seems likely that
> the problem is with the ARM test code rather than actual kprobe
> implementation itself.

OK, then I'll just update the series according your comment.

> 
> Like I said, this issue has been there for a year or more, so I wasn't
> planning on spending time on it for a few more days yet whilst I get on
> with other urgent matters.

OK, I'll also investigate it.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web