Message-Id: <20181115004141.5ed772834fc6bdf3467f244e@kernel.org>
Date:   Thu, 15 Nov 2018 00:41:41 -0800
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Patrick Staehlin <me@...ki.ch>
Cc:     linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Palmer Dabbelt <palmer@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Alan Kao <alankao@...estech.com>, Zong Li <zong@...estech.com>,
        Ingo Molnar <mingo@...nel.org>,
        Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Catalin Marinas <catalin.marinas@....com>,
        zhong jiang <zhongjiang@...wei.com>,
        Anders Roxell <anders.roxell@...aro.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jim Wilson <jimw@...ive.com>,
        Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        Souptick Joarder <jrdr.linux@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [RFC/RFT 2/2] RISC-V: kprobes/kretprobe support

On Wed, 14 Nov 2018 21:52:57 +0100
Patrick Staehlin <me@...ki.ch> wrote:

> Hi Masami,
> 
> thank you for your remarks.
> 
> On 14.11.18 09:37, Masami Hiramatsu wrote>
> > Thank you very much for implementing kprobes on RISC-V :)
> > 
> > On Tue, 13 Nov 2018 20:58:04 +0100
> > Patrick Stählin <me@...ki.ch> wrote:
> > 
> >> First implementation, adapted from arm64. The C.ADDISP16 instruction
> >> gets simulated and the kprobes-handler called by inserting a C.EBREAK
> >> instruction.
> >>
> >> C.ADDISP16 was chosen as it sets-up the stack frame for functions.
> >> Some work has been done to support probes on non-compressed
> >> instructions but there is no support yet for decoding those.
> > 
> > Does this only support C.ADDISP16? No other insns are supported?
> > Supporting 1 insn is too few I think.
> 
> At the moment, yes. I'm waiting for some input from somebody with deeper
> insights into the RISC-V architecture than me before implementing more
> instructions, should solution I've chosen be woefully inadequate.

I think starting with a few emulatable set of instruction support is good
to me. But maybe we need more... 1 instruction is too limited.


> > Can RISC-V do single stepping? If not, we need to prepare emulator
> > as match as possible, or for ALU instructions, we can run it on
> > buffer and hook it.
> 
> The debug-specification is still a draft but there are some softcores
> that implement it. But even if it was finalized I don't think this will
> be made a mandatory extension so we need to simulate/emulate a good part
> of the instruction set anyway.

OK.

[...]
> >> diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
> >> new file mode 100644
> >> index 000000000000..2d8f46f4c2e7
> >> --- /dev/null
> >> +++ b/arch/riscv/kernel/probes/decode-insn.c
> >> @@ -0,0 +1,38 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/kprobes.h>
> >> +#include <linux/module.h>
> >> +#include <linux/kallsyms.h>
> >> +#include <asm/sections.h>
> >> +
> >> +#include "decode-insn.h"
> >> +#include "simulate-insn.h"
> >> +
> >> +#define C_ADDISP16_MASK 0x6F83
> >> +#define C_ADDISP16_VAL  0x6101
> >> +
> >> +/* Return:
> >> + *   INSN_REJECTED     If instruction is one not allowed to kprobe,
> >> + *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
> >> + */
> >> +enum probe_insn __kprobes
> >> +riscv_probe_decode_insn(kprobe_opcode_t *addr, struct arch_probe_insn *api)
> > 
> > Please don't use __kprobes anymore. That is old stlye, instead, please
> > use NOKPROBE_SYMBOL() or nokprobe_inline for static-inline function.
> > (NOKPROBE_SYMBOL() will make the symbol non-inline always)
> 
> OK, should I make a note to change that in the arm64 port as well in a
> separate patch?

I think you don't need such note for this annotation. It should be fixed on arm64
too. (Sorry, that is my lazyness..)

[...]
> >> +
> >> +static void __kprobes kprobe_handler(struct pt_regs *regs)
> >> +{
> > 
> > Does this handler run under local IRQ disabled? (for making sure)
> 
> Exceptions are being handled with locals IRQs _enabled_.

Oops that's crazy and cool :D
So I guess it just implemented as an indirect jump referring the exception vector.
Just out of curiosity, is that enough safe for interruption?

> As we've not
> implemented any simulated instructions to modify the instruction
> pointer, I think this is safe? Then again, I'm new to this, so please
> bear with me.

kprobes itself should be safe, since I introduced a jump optimization,
which doing similar thing.

However, if it is not IRQ disabled, you must preempt_disable() right
before using get_kprobe_ctlblk() because it is per-cpu.

> >> +	struct kprobe *p, *cur_kprobe;
> >> +	struct kprobe_ctlblk *kcb;
> >> +	unsigned long addr = instruction_pointer(regs);
> >> +
> >> +	kcb = get_kprobe_ctlblk();
> >> +	cur_kprobe = kprobe_running();
> >> +
> >> +	p = get_kprobe((kprobe_opcode_t *) addr);
> >> +
> >> +	if (p) {
> >> +		if (cur_kprobe) {
> >> +			if (reenter_kprobe(p, regs, kcb))
> >> +				return;
> >> +		} else {
> >> +			/* Probe hit */
> >> +			set_current_kprobe(p);
> >> +			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> >> +
> >> +			/*
> >> +			 * If we have no pre-handler or it returned 0, we
> >> +			 * continue with normal processing.  If we have a
> >> +			 * pre-handler and it returned non-zero, it will
> >> +			 * modify the execution path and no need to single
> >> +			 * stepping. Let's just reset current kprobe and exit.
> >> +			 *
> >> +			 * pre_handler can hit a breakpoint and can step thru
> >> +			 * before return, keep PSTATE D-flag enabled until
> >> +			 * pre_handler return back.
> > 
> > Is this true on RISC-V too?
> 
> It's not as we don't have a debug-unit at the moment. I'll remove the
> second part of the comment block.

OK.

> >> +			 */
> >> +			if (!p->pre_handler || !p->pre_handler(p, regs))
> >> +				simulate(p, regs, kcb, 0);
> >> +			else
> >> +				reset_current_kprobe();
> >> +		}
> >> +	}
> >> +	/*
> >> +	 * The breakpoint instruction was removed right
> >> +	 * after we hit it.  Another cpu has removed
> >> +	 * either a probepoint or a debugger breakpoint
> >> +	 * at this address.  In either case, no further
> >> +	 * handling of this interrupt is appropriate.
> >> +	 * Return back to original instruction, and continue.
> >> +	 */
> > 
> > This should return 0 if it doesn't handle anything, but if it handles c.break
> > should return 1.
> 
> I thought so too, but didn't insert one because of the comment (again,
> copied from arm64). If get_kprobe doesn't return a result, it could have
> been removed between the time the exception got raised or is the comment
> just wrong? On the other hand, solving it this way effectively means
> that we'll silently drop any other exceptions.

Hmm, in x86, original code, I checked the *addr whether there is a Breakpoint
instruction. If not, this means someone already removed it. If there is,
we just return to kernel's break handler and see what happens, since the breakpoint
instruction can be used from another subsystem. If no one handles it, kernel may
warn it finds stray breakpoint. (warning such case is kernel's work, not kprobes') 

I think I have to recheck the arm64's implementation again...


> >> +}
> >> +
> >> +int __kprobes
> >> +kprobe_breakpoint_handler(struct pt_regs *regs)
> >> +{
> >> +	kprobe_handler(regs);
> >> +	return 1;
> > 
> > Why don't you call kprobe_handler directly?
> 
> I should.
> 
> > 
> >> +}
> >> +
> >> +bool arch_within_kprobe_blacklist(unsigned long addr)
> >> +{
> >> +	if ((addr >= (unsigned long)__kprobes_text_start &&
> >> +	    addr < (unsigned long)__kprobes_text_end) ||
> >> +	    (addr >= (unsigned long)__entry_text_start &&
> >> +	    addr < (unsigned long)__entry_text_end) ||
> >> +	    !!search_exception_tables(addr))
> >> +		return true;
> >> +
> >> +	return false;
> >> +}
> >> +
> >> +void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
> >> +{
> >> +	struct kretprobe_instance *ri = NULL;
> >> +	struct hlist_head *head, empty_rp;
> >> +	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;
> >> +
> > 
> > 	It seems you don't setup instruction_pointer.
> 
> I don't think I get what you mean by that. The instruction pointer (or
> rather the return address register) gets set by kretprobe_trampoline.S
> to the address returned from this function.

I meant regs->sepc should be set as the address of the trampoline function.
There is a histrical reason, but that is expected behavior...

[...]
> >> diff --git a/arch/riscv/kernel/probes/simulate-insn.c b/arch/riscv/kernel/probes/simulate-insn.c
> >> new file mode 100644
> >> index 000000000000..5734d9bae22f
> >> --- /dev/null
> >> +++ b/arch/riscv/kernel/probes/simulate-insn.c
> >> @@ -0,0 +1,33 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +
> >> +#include <linux/bitops.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/kprobes.h>
> >> +
> >> +#include "simulate-insn.h"
> >> +
> >> +#define bit_at(value, bit)		((value) & (1 << (bit)))
> >> +#define move_bit_at(value, bit, to)	((bit_at(value, bit) >> bit) << to)
> >> +
> >> +void __kprobes
> >> +simulate_caddisp16(u32 opcode, long addr, struct pt_regs *regs)
> >> +{
> >> +	s16 imm;
> >> +
> >> +	/*
> >> +	 * Immediate value layout in c.addisp16:
> >> +	 * xxx9 xxxx x468 75xx
> >> +	 * 1  1    8    4    0
> >> +	 * 5  2
> >> +	 */
> >> +	imm = sign_extend32(
> >> +		move_bit_at(opcode, 12, 9) |
> >> +		move_bit_at(opcode, 6, 4) |
> >> +		move_bit_at(opcode, 5, 6) |
> >> +		move_bit_at(opcode, 4, 8) |
> >> +		move_bit_at(opcode, 3, 7) |
> >> +		move_bit_at(opcode, 2, 5),
> >> +		9);
> >> +
> >> +	regs->sp += imm;
> > 
> > What about updating regs->sepc?
> 
> sepc gets updated by instruction_pointer_set in kprobe_handler

post_kprobe_handler()? Anyway, I think it should be updated in
this function or simulate() since it is a part of instruction execution.

> >> +}
> >> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> >> new file mode 100644
> >> index 000000000000..dc2c06c30167
> >> --- /dev/null
> >> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> >> @@ -0,0 +1,8 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> +
> >> +#ifndef _RISCV_KERNEL_KPROBES_SIMULATE_INSN_H
> >> +#define _RISCV_KERNEL_KPROBES_SIMULATE_INSN_H
> >> +
> >> +void simulate_caddisp16(u32 opcode, long addr, struct pt_regs *regs);
> >> +
> >> +#endif /* _RISCV_KERNEL_KPROBES_SIMULATE_INSN_H */
> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> >> index 24a9333dda2c..d7113178d401 100644
> >> --- a/arch/riscv/kernel/traps.c
> >> +++ b/arch/riscv/kernel/traps.c
> >> @@ -18,6 +18,7 @@
> >>  #include <linux/sched/signal.h>
> >>  #include <linux/signal.h>
> >>  #include <linux/kdebug.h>
> >> +#include <linux/kprobes.h>
> >>  #include <linux/uaccess.h>
> >>  #include <linux/mm.h>
> >>  #include <linux/module.h>
> >> @@ -120,8 +121,14 @@ DO_ERROR_INFO(do_trap_ecall_m,
> >>  
> >>  asmlinkage void do_trap_break(struct pt_regs *regs)
> >>  {
> >> +	bool handler_found = false;
> >> +
> >> +#ifdef CONFIG_KPROBES
> >> +	if (kprobe_breakpoint_handler(regs))
> >> +		handler_found = 1;
> > 
> > Why don't you just return from here?
> 
> Following the pattern I've seen in other places, I can change that.

Yeah, I think it's simpler.

And I found that the kprobe_breakpoint_handler() was called without
checking !user_mode(regs). In that case, you should add the check in
front of kprobe_breakpoint_handler() call.

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists