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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>