linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Questions about kprobe handler
@ 2022-11-17  1:07 Tiezhu Yang
  2022-11-17 13:09 ` Masami Hiramatsu
  0 siblings, 1 reply; 6+ messages in thread
From: Tiezhu Yang @ 2022-11-17  1:07 UTC (permalink / raw)
  To: Naveen N. Rao, Anil S Keshavamurthy, David S. Miller, Masami Hiramatsu
  Cc: Huacai Chen, Jinyang He, linux-kernel

Hi KPROBES maintainers,

There are some differences of kprobe handler implementations on various
archs, the implementations are almost same on arm64, riscv, csky, the
code logic is clear and understandable. But on mips and loongarch (not
upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
the check "if (addr->word != breakpoint_insn.word)", is it necessary?
Can we just return directly? Please take a look, thank you.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
		p = get_kprobe(addr);
		if (p) {
			...
		} else if (addr->word != breakpoint_insn.word) {
			/*
			 * The breakpoint instruction was removed by
			 * another cpu right after we hit, no further
			 * handling of this interrupt is appropriate
			 */
			ret = 1;
		}
https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L262
	p = get_kprobe(addr);
	if (p) {
		...
	} else {
		if (addr->word != breakpoint_insn.word) {
			/*
			 * 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.
			 */
			preempt_enable_no_resched();
			return 1;
		}
	}

(1) arm64
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/probes/kprobes.c#n309
static void __kprobes kprobe_handler(struct pt_regs *regs)
{
	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.
			 */
			if (!p->pre_handler || !p->pre_handler(p, regs)) {
				setup_singlestep(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.
	 */
}

(2) riscv
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/kernel/probes/kprobes.c#n269
bool __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs)
{
	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 true;
		} 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.
			 */
			if (!p->pre_handler || !p->pre_handler(p, regs))
				setup_singlestep(p, regs, kcb, 0);
			else
				reset_current_kprobe();
		}
		return true;
	}

	/*
	 * 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.
	 */
	return false;
}

(3) csky
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/csky/kernel/probes/kprobes.c#n311
int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs)
{
	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 1;
		} 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.
			 */
			if (!p->pre_handler || !p->pre_handler(p, regs))
				setup_singlestep(p, regs, kcb, 0);
			else
				reset_current_kprobe();
		}
		return 1;
	}

	/*
	 * 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.
	 */
	return 0;
}

(4) mips
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n279
static int kprobe_handler(struct pt_regs *regs)
{
	struct kprobe *p;
	int ret = 0;
	kprobe_opcode_t *addr;
	struct kprobe_ctlblk *kcb;

	addr = (kprobe_opcode_t *) regs->cp0_epc;

	/*
	 * We don't want to be preempted for the entire
	 * duration of kprobe processing
	 */
	preempt_disable();
	kcb = get_kprobe_ctlblk();

	/* Check we're not actually recursing */
	if (kprobe_running()) {
		p = get_kprobe(addr);
		if (p) {
			if (kcb->kprobe_status == KPROBE_HIT_SS &&
			    p->ainsn.insn->word == breakpoint_insn.word) {
				regs->cp0_status &= ~ST0_IE;
				regs->cp0_status |= kcb->kprobe_saved_SR;
				goto no_kprobe;
			}
			/*
			 * We have reentered the kprobe_handler(), since
			 * another probe was hit while within the handler.
			 * We here save the original kprobes variables and
			 * just single step on the instruction of the new probe
			 * without calling any user handlers.
			 */
			save_previous_kprobe(kcb);
			set_current_kprobe(p, regs, kcb);
			kprobes_inc_nmissed_count(p);
			prepare_singlestep(p, regs, kcb);
			kcb->kprobe_status = KPROBE_REENTER;
			if (kcb->flags & SKIP_DELAYSLOT) {
				resume_execution(p, regs, kcb);
				restore_previous_kprobe(kcb);
				preempt_enable_no_resched();
			}
			return 1;
		} else if (addr->word != breakpoint_insn.word) {
			/*
			 * The breakpoint instruction was removed by
			 * another cpu right after we hit, no further
			 * handling of this interrupt is appropriate
			 */
			ret = 1;
		}
		goto no_kprobe;
	}
...
}

(5) loongarch
https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L228
static int __kprobes kprobe_handler(struct pt_regs *regs)
{
	struct kprobe *p;
	kprobe_opcode_t *addr;
	struct kprobe_ctlblk *kcb;

	addr = (kprobe_opcode_t *) regs->csr_era;

	/*
	 * We don't want to be preempted for the entire
	 * duration of kprobe processing
	 */
	preempt_disable();
	kcb = get_kprobe_ctlblk();

	p = get_kprobe(addr);
	if (p) {
		if (kprobe_running()) {
			if (reenter_kprobe(p, regs, kcb))
				return 1;
		} else {
			set_current_kprobe(p, regs, kcb);
			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
			if (p->pre_handler && p->pre_handler(p, regs)) {
			/* handler has already set things up, so skip ss setup */
				reset_current_kprobe();
				preempt_enable_no_resched();
				return 1;
			} else {
				setup_singlestep(p, regs, kcb, 0);
				return 1;
			}
		}
	} else {
		if (addr->word != breakpoint_insn.word) {
			/*
			 * 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.
			 */
			preempt_enable_no_resched();
			return 1;
		}
	}

	preempt_enable_no_resched();

	return 0;
}

Thanks,
Tiezhu


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Questions about kprobe handler
  2022-11-17  1:07 Questions about kprobe handler Tiezhu Yang
@ 2022-11-17 13:09 ` Masami Hiramatsu
  2022-11-23 15:24   ` Jinyang He
  0 siblings, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2022-11-17 13:09 UTC (permalink / raw)
  To: Tiezhu Yang
  Cc: Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
	Huacai Chen, Jinyang He, linux-kernel

On Thu, 17 Nov 2022 09:07:37 +0800
Tiezhu Yang <yangtiezhu@loongson.cn> wrote:

> Hi KPROBES maintainers,
> 
> There are some differences of kprobe handler implementations on various
> archs, the implementations are almost same on arm64, riscv, csky, the
> code logic is clear and understandable. But on mips and loongarch (not
> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
> Can we just return directly? Please take a look, thank you.

Good question!

This means that when the software breakpoint was hit on that CPU, but
before calling kprobe handler function, the other CPU can remove that
kprobe from hash table, becahse the hash table is not locked.
In that case, the get_kprobe(addr) will return NULL, and the software
breakpoint instruction is already removed (replaced with the original
instruction). Thus it is safe to go back. But this is originally
implemented for x86, which doesn't need stop_machine() to modify the
code. On the other hand, if an architecture which needs stop_machine()
to modify code, the above scenario never happen. In that case, you
don't need this "if" case.

Thank you,

> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
> 		p = get_kprobe(addr);
> 		if (p) {
> 			...
> 		} else if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * The breakpoint instruction was removed by
> 			 * another cpu right after we hit, no further
> 			 * handling of this interrupt is appropriate
> 			 */
> 			ret = 1;
> 		}
> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L262
> 	p = get_kprobe(addr);
> 	if (p) {
> 		...
> 	} else {
> 		if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * 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.
> 			 */
> 			preempt_enable_no_resched();
> 			return 1;
> 		}
> 	}
> 
> (1) arm64
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/probes/kprobes.c#n309
> static void __kprobes kprobe_handler(struct pt_regs *regs)
> {
> 	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.
> 			 */
> 			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> 				setup_singlestep(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.
> 	 */
> }
> 
> (2) riscv
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/kernel/probes/kprobes.c#n269
> bool __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs)
> {
> 	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 true;
> 		} 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.
> 			 */
> 			if (!p->pre_handler || !p->pre_handler(p, regs))
> 				setup_singlestep(p, regs, kcb, 0);
> 			else
> 				reset_current_kprobe();
> 		}
> 		return true;
> 	}
> 
> 	/*
> 	 * 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.
> 	 */
> 	return false;
> }
> 
> (3) csky
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/csky/kernel/probes/kprobes.c#n311
> int __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs)
> {
> 	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 1;
> 		} 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.
> 			 */
> 			if (!p->pre_handler || !p->pre_handler(p, regs))
> 				setup_singlestep(p, regs, kcb, 0);
> 			else
> 				reset_current_kprobe();
> 		}
> 		return 1;
> 	}
> 
> 	/*
> 	 * 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.
> 	 */
> 	return 0;
> }
> 
> (4) mips
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n279
> static int kprobe_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p;
> 	int ret = 0;
> 	kprobe_opcode_t *addr;
> 	struct kprobe_ctlblk *kcb;
> 
> 	addr = (kprobe_opcode_t *) regs->cp0_epc;
> 
> 	/*
> 	 * We don't want to be preempted for the entire
> 	 * duration of kprobe processing
> 	 */
> 	preempt_disable();
> 	kcb = get_kprobe_ctlblk();
> 
> 	/* Check we're not actually recursing */
> 	if (kprobe_running()) {
> 		p = get_kprobe(addr);
> 		if (p) {
> 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
> 			    p->ainsn.insn->word == breakpoint_insn.word) {
> 				regs->cp0_status &= ~ST0_IE;
> 				regs->cp0_status |= kcb->kprobe_saved_SR;
> 				goto no_kprobe;
> 			}
> 			/*
> 			 * We have reentered the kprobe_handler(), since
> 			 * another probe was hit while within the handler.
> 			 * We here save the original kprobes variables and
> 			 * just single step on the instruction of the new probe
> 			 * without calling any user handlers.
> 			 */
> 			save_previous_kprobe(kcb);
> 			set_current_kprobe(p, regs, kcb);
> 			kprobes_inc_nmissed_count(p);
> 			prepare_singlestep(p, regs, kcb);
> 			kcb->kprobe_status = KPROBE_REENTER;
> 			if (kcb->flags & SKIP_DELAYSLOT) {
> 				resume_execution(p, regs, kcb);
> 				restore_previous_kprobe(kcb);
> 				preempt_enable_no_resched();
> 			}
> 			return 1;
> 		} else if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * The breakpoint instruction was removed by
> 			 * another cpu right after we hit, no further
> 			 * handling of this interrupt is appropriate
> 			 */
> 			ret = 1;
> 		}
> 		goto no_kprobe;
> 	}
> ...
> }
> 
> (5) loongarch
> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L228
> static int __kprobes kprobe_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p;
> 	kprobe_opcode_t *addr;
> 	struct kprobe_ctlblk *kcb;
> 
> 	addr = (kprobe_opcode_t *) regs->csr_era;
> 
> 	/*
> 	 * We don't want to be preempted for the entire
> 	 * duration of kprobe processing
> 	 */
> 	preempt_disable();
> 	kcb = get_kprobe_ctlblk();
> 
> 	p = get_kprobe(addr);
> 	if (p) {
> 		if (kprobe_running()) {
> 			if (reenter_kprobe(p, regs, kcb))
> 				return 1;
> 		} else {
> 			set_current_kprobe(p, regs, kcb);
> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> 			if (p->pre_handler && p->pre_handler(p, regs)) {
> 			/* handler has already set things up, so skip ss setup */
> 				reset_current_kprobe();
> 				preempt_enable_no_resched();
> 				return 1;
> 			} else {
> 				setup_singlestep(p, regs, kcb, 0);
> 				return 1;
> 			}
> 		}
> 	} else {
> 		if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * 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.
> 			 */
> 			preempt_enable_no_resched();
> 			return 1;
> 		}
> 	}
> 
> 	preempt_enable_no_resched();
> 
> 	return 0;
> }
> 
> Thanks,
> Tiezhu
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Questions about kprobe handler
  2022-11-17 13:09 ` Masami Hiramatsu
@ 2022-11-23 15:24   ` Jinyang He
  2022-11-24 11:44     ` Naveen N. Rao
  2022-11-24 14:13     ` Masami Hiramatsu
  0 siblings, 2 replies; 6+ messages in thread
From: Jinyang He @ 2022-11-23 15:24 UTC (permalink / raw)
  To: Masami Hiramatsu (Google), Tiezhu Yang
  Cc: Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
	Huacai Chen, linux-kernel

在 2022/11/17 21:09, Masami Hiramatsu (Google) 写道:

> On Thu, 17 Nov 2022 09:07:37 +0800
> Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>> Hi KPROBES maintainers,
>>
>> There are some differences of kprobe handler implementations on various
>> archs, the implementations are almost same on arm64, riscv, csky, the
>> code logic is clear and understandable. But on mips and loongarch (not
>> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
>> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
>> Can we just return directly? Please take a look, thank you.
> Good question!
>
> This means that when the software breakpoint was hit on that CPU, but
> before calling kprobe handler function, the other CPU can remove that
> kprobe from hash table, becahse the hash table is not locked.
> In that case, the get_kprobe(addr) will return NULL, and the software
> breakpoint instruction is already removed (replaced with the original
> instruction). Thus it is safe to go back. But this is originally
> implemented for x86, which doesn't need stop_machine() to modify the
> code. On the other hand, if an architecture which needs stop_machine()
> to modify code, the above scenario never happen. In that case, you
> don't need this "if" case.
>
> Thank you,
>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
>> 		p = get_kprobe(addr);
>> 		if (p) {
>> 			...
>> 		} else if (addr->word != breakpoint_insn.word) {

Hi,


Sorry for the late reply, but I think there should be some public
comments so that I can get the authoritative response, as offline
communication with Tiezhu is always one-sided.

I think the branch you answered here is " if (p)... " rather than
"else if (addr->word != breakpoint_insn.word)". It is right if we
not use stop_machine here we need this branch. In fact, Tiezhu
and Huacai, the maintainer of LoongArch are more concerned
about the latter why we need compare with the breakpoint_insn.

The reason I gave as follows, and I show mips code here,

     p = get_kprobe(addr);
     if (!p) {
         if (addr->word != breakpoint_insn.word) {
             /*
              * 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.
              */
             ret = 1;
         }
         /* Not one of ours: let kernel handle it */
         goto no_kprobe;
     }

...
int kprobe_exceptions_notify(struct notifier_block *self,
                        unsigned long val, void *data)
{
     struct die_args *args = (struct die_args *)data;
     int ret = NOTIFY_DONE;

     switch (val) {
     case DIE_BREAK:
         if (kprobe_handler(args->regs))
             ret = NOTIFY_STOP;
         break;
...

The !p means this insn has been moved, and then in most cases the COND
"addr->word != breakpoint_insn.word" is true, we return 1 so that the return
value in kprobe_exceptions_notify will be changed to NOTIFY_STOP.
The mips use soft breakpoint like "break hint". How if the original insn
is same as breakpoint_insn? That is a few case when COND is false. In that
case, it means we should handle it by other handlers and doesn't change 
ret to
keep NOTIFY_DONE as kprobe_exceptions_notify return value.

Is this idea reasonable? Thanks!


Jinyang

>> 			/*
>> 			 * The breakpoint instruction was removed by
>> 			 * another cpu right after we hit, no further
>> 			 * handling of this interrupt is appropriate
>> 			 */
>> 			ret = 1;
>> 		}
>> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L262
>> 	p = get_kprobe(addr);
>> 	if (p) {
>> 		...
>> 	} else {
>> 		if (addr->word != breakpoint_insn.word) {
>> 			/*
>> 			 * 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.
>> 			 */
>> 			preempt_enable_no_resched();
>> 			return 1;
>> 		}
>> 	}
>>
>> (1) arm64
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/probes/kprobes.c#n309
>> static void __kprobes kprobe_handler(struct pt_regs *regs)
>> {
>> 	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.
>> 			 */
>> 			if (!p->pre_handler || !p->pre_handler(p, regs)) {
>> 				setup_singlestep(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.
>> 	 */
>> }
>>
>> (2) riscv
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/kernel/probes/kprobes.c#n269
>> bool __kprobes
>> kprobe_breakpoint_handler(struct pt_regs *regs)
>> {
>> 	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 true;
>> 		} 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.
>> 			 */
>> 			if (!p->pre_handler || !p->pre_handler(p, regs))
>> 				setup_singlestep(p, regs, kcb, 0);
>> 			else
>> 				reset_current_kprobe();
>> 		}
>> 		return true;
>> 	}
>>
>> 	/*
>> 	 * 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.
>> 	 */
>> 	return false;
>> }
>>
>> (3) csky
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/csky/kernel/probes/kprobes.c#n311
>> int __kprobes
>> kprobe_breakpoint_handler(struct pt_regs *regs)
>> {
>> 	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 1;
>> 		} 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.
>> 			 */
>> 			if (!p->pre_handler || !p->pre_handler(p, regs))
>> 				setup_singlestep(p, regs, kcb, 0);
>> 			else
>> 				reset_current_kprobe();
>> 		}
>> 		return 1;
>> 	}
>>
>> 	/*
>> 	 * 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.
>> 	 */
>> 	return 0;
>> }
>>
>> (4) mips
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n279
>> static int kprobe_handler(struct pt_regs *regs)
>> {
>> 	struct kprobe *p;
>> 	int ret = 0;
>> 	kprobe_opcode_t *addr;
>> 	struct kprobe_ctlblk *kcb;
>>
>> 	addr = (kprobe_opcode_t *) regs->cp0_epc;
>>
>> 	/*
>> 	 * We don't want to be preempted for the entire
>> 	 * duration of kprobe processing
>> 	 */
>> 	preempt_disable();
>> 	kcb = get_kprobe_ctlblk();
>>
>> 	/* Check we're not actually recursing */
>> 	if (kprobe_running()) {
>> 		p = get_kprobe(addr);
>> 		if (p) {
>> 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
>> 			    p->ainsn.insn->word == breakpoint_insn.word) {
>> 				regs->cp0_status &= ~ST0_IE;
>> 				regs->cp0_status |= kcb->kprobe_saved_SR;
>> 				goto no_kprobe;
>> 			}
>> 			/*
>> 			 * We have reentered the kprobe_handler(), since
>> 			 * another probe was hit while within the handler.
>> 			 * We here save the original kprobes variables and
>> 			 * just single step on the instruction of the new probe
>> 			 * without calling any user handlers.
>> 			 */
>> 			save_previous_kprobe(kcb);
>> 			set_current_kprobe(p, regs, kcb);
>> 			kprobes_inc_nmissed_count(p);
>> 			prepare_singlestep(p, regs, kcb);
>> 			kcb->kprobe_status = KPROBE_REENTER;
>> 			if (kcb->flags & SKIP_DELAYSLOT) {
>> 				resume_execution(p, regs, kcb);
>> 				restore_previous_kprobe(kcb);
>> 				preempt_enable_no_resched();
>> 			}
>> 			return 1;
>> 		} else if (addr->word != breakpoint_insn.word) {
>> 			/*
>> 			 * The breakpoint instruction was removed by
>> 			 * another cpu right after we hit, no further
>> 			 * handling of this interrupt is appropriate
>> 			 */
>> 			ret = 1;
>> 		}
>> 		goto no_kprobe;
>> 	}
>> ...
>> }
>>
>> (5) loongarch
>> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L228
>> static int __kprobes kprobe_handler(struct pt_regs *regs)
>> {
>> 	struct kprobe *p;
>> 	kprobe_opcode_t *addr;
>> 	struct kprobe_ctlblk *kcb;
>>
>> 	addr = (kprobe_opcode_t *) regs->csr_era;
>>
>> 	/*
>> 	 * We don't want to be preempted for the entire
>> 	 * duration of kprobe processing
>> 	 */
>> 	preempt_disable();
>> 	kcb = get_kprobe_ctlblk();
>>
>> 	p = get_kprobe(addr);
>> 	if (p) {
>> 		if (kprobe_running()) {
>> 			if (reenter_kprobe(p, regs, kcb))
>> 				return 1;
>> 		} else {
>> 			set_current_kprobe(p, regs, kcb);
>> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>> 			if (p->pre_handler && p->pre_handler(p, regs)) {
>> 			/* handler has already set things up, so skip ss setup */
>> 				reset_current_kprobe();
>> 				preempt_enable_no_resched();
>> 				return 1;
>> 			} else {
>> 				setup_singlestep(p, regs, kcb, 0);
>> 				return 1;
>> 			}
>> 		}
>> 	} else {
>> 		if (addr->word != breakpoint_insn.word) {
>> 			/*
>> 			 * 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.
>> 			 */
>> 			preempt_enable_no_resched();
>> 			return 1;
>> 		}
>> 	}
>>
>> 	preempt_enable_no_resched();
>>
>> 	return 0;
>> }
>>
>> Thanks,
>> Tiezhu
>>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Questions about kprobe handler
  2022-11-23 15:24   ` Jinyang He
@ 2022-11-24 11:44     ` Naveen N. Rao
  2022-11-24 15:03       ` Masami Hiramatsu
  2022-11-24 14:13     ` Masami Hiramatsu
  1 sibling, 1 reply; 6+ messages in thread
From: Naveen N. Rao @ 2022-11-24 11:44 UTC (permalink / raw)
  To: Jinyang He, Masami Hiramatsu (Google), Tiezhu Yang
  Cc: Anil S Keshavamurthy, Huacai Chen, David S. Miller, linux-kernel,
	linuxppc-dev

Jinyang He wrote:
> 在 2022/11/17 21:09, Masami Hiramatsu (Google) 写道:
> 
>> On Thu, 17 Nov 2022 09:07:37 +0800
>> Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>>
>>> Hi KPROBES maintainers,
>>>
>>> There are some differences of kprobe handler implementations on various
>>> archs, the implementations are almost same on arm64, riscv, csky, the
>>> code logic is clear and understandable. But on mips and loongarch (not
>>> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
>>> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
>>> Can we just return directly? Please take a look, thank you.

Are you seeing any problem with that?

>> Good question!
>>
>> This means that when the software breakpoint was hit on that CPU, but
>> before calling kprobe handler function, the other CPU can remove that
>> kprobe from hash table, becahse the hash table is not locked.
>> In that case, the get_kprobe(addr) will return NULL, and the software
>> breakpoint instruction is already removed (replaced with the original
>> instruction). Thus it is safe to go back. But this is originally
>> implemented for x86, which doesn't need stop_machine() to modify the
>> code. On the other hand, if an architecture which needs stop_machine()
>> to modify code, the above scenario never happen. In that case, you
>> don't need this "if" case.

This has been a problematic area on powerpc. See, for instance, commits:
- 9ed5df69b79a22 ("powerpc/kprobes: Use probe_address() to read instructions")
- 21f8b2fa3ca5b0 ("powerpc/kprobes: Ignore traps that happened in real mode")

I think we should close this race, perhaps by instroducing another 
synchronize_rcu() in unregister_kprobe(), allowing architectures using 
stop_machine() to override that. That would be cleaner.


>>
>> Thank you,
>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
>>> 		p = get_kprobe(addr);
>>> 		if (p) {
>>> 			...
>>> 		} else if (addr->word != breakpoint_insn.word) {
> 
> Hi,
> 
> 
> Sorry for the late reply, but I think there should be some public
> comments so that I can get the authoritative response, as offline
> communication with Tiezhu is always one-sided.
> 
> I think the branch you answered here is " if (p)... " rather than
> "else if (addr->word != breakpoint_insn.word)". It is right if we
> not use stop_machine here we need this branch. In fact, Tiezhu
> and Huacai, the maintainer of LoongArch are more concerned
> about the latter why we need compare with the breakpoint_insn.

Masami answered why we need the else part comparing the instruction at 
addr with the breakpoint instruction. It is to check if the breakpoint 
instruction has been removed since we hit it, but before the processor 
reached the kprobe handler.

> 
> The reason I gave as follows, and I show mips code here,
> 
>      p = get_kprobe(addr);
>      if (!p) {
>          if (addr->word != breakpoint_insn.word) {
>              /*
>               * 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.
>               */
>              ret = 1;
>          }
>          /* Not one of ours: let kernel handle it */
>          goto no_kprobe;
>      }
> 
> ...
> int kprobe_exceptions_notify(struct notifier_block *self,
>                         unsigned long val, void *data)
> {
>      struct die_args *args = (struct die_args *)data;
>      int ret = NOTIFY_DONE;
> 
>      switch (val) {
>      case DIE_BREAK:
>          if (kprobe_handler(args->regs))
>              ret = NOTIFY_STOP;
>          break;
> ...
> 
> The !p means this insn has been moved, and then in most cases the COND

!p means the kprobe has been removed - there may or may not be another 
breakpoint instruction at that address.

> "addr->word != breakpoint_insn.word" is true, we return 1 so that the return
> value in kprobe_exceptions_notify will be changed to NOTIFY_STOP.

We do this since we now know that there is no breakpoint at this 
address, so we should not continue.

> The mips use soft breakpoint like "break hint". How if the original insn
> is same as breakpoint_insn? That is a few case when COND is false. In that
> case, it means we should handle it by other handlers and doesn't change 
> ret to
> keep NOTIFY_DONE as kprobe_exceptions_notify return value.

If there is a breakpoint instruction at that address, we should let the 
kernel continue processing the breakpoint.

> 
> Is this idea reasonable? Thanks!

As another data point, you may want to look at kprobe_handler() in 
arch/powerpc/kernel/kprobes.c . We also handle cases where there is a 
different type of breakpoint instruction.


- Naveen


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Questions about kprobe handler
  2022-11-23 15:24   ` Jinyang He
  2022-11-24 11:44     ` Naveen N. Rao
@ 2022-11-24 14:13     ` Masami Hiramatsu
  1 sibling, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2022-11-24 14:13 UTC (permalink / raw)
  To: Jinyang He
  Cc: Tiezhu Yang, Naveen N. Rao, Anil S Keshavamurthy,
	David S. Miller, Huacai Chen, linux-kernel

On Wed, 23 Nov 2022 23:24:36 +0800
Jinyang He <hejinyang@loongson.cn> wrote:

> 在 2022/11/17 21:09, Masami Hiramatsu (Google) 写道:
> 
> > On Thu, 17 Nov 2022 09:07:37 +0800
> > Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> >
> >> Hi KPROBES maintainers,
> >>
> >> There are some differences of kprobe handler implementations on various
> >> archs, the implementations are almost same on arm64, riscv, csky, the
> >> code logic is clear and understandable. But on mips and loongarch (not
> >> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
> >> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
> >> Can we just return directly? Please take a look, thank you.
> > Good question!
> >
> > This means that when the software breakpoint was hit on that CPU, but
> > before calling kprobe handler function, the other CPU can remove that
> > kprobe from hash table, becahse the hash table is not locked.
> > In that case, the get_kprobe(addr) will return NULL, and the software
> > breakpoint instruction is already removed (replaced with the original
> > instruction). Thus it is safe to go back. But this is originally
> > implemented for x86, which doesn't need stop_machine() to modify the
> > code. On the other hand, if an architecture which needs stop_machine()
> > to modify code, the above scenario never happen. In that case, you
> > don't need this "if" case.
> >
> > Thank you,
> >
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
> >> 		p = get_kprobe(addr);
> >> 		if (p) {
> >> 			...
> >> 		} else if (addr->word != breakpoint_insn.word) {
> 
> Hi,
> 
> 
> Sorry for the late reply, but I think there should be some public
> comments so that I can get the authoritative response, as offline
> communication with Tiezhu is always one-sided.
> 
> I think the branch you answered here is " if (p)... " rather than
> "else if (addr->word != breakpoint_insn.word)". It is right if we
> not use stop_machine here we need this branch. In fact, Tiezhu
> and Huacai, the maintainer of LoongArch are more concerned
> about the latter why we need compare with the breakpoint_insn.
> 
> The reason I gave as follows, and I show mips code here,
> 
>      p = get_kprobe(addr);
>      if (!p) {
>          if (addr->word != breakpoint_insn.word) {
>              /*
>               * 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.
>               */
>              ret = 1;
>          }
>          /* Not one of ours: let kernel handle it */
>          goto no_kprobe;
>      }
> 
> ...
> int kprobe_exceptions_notify(struct notifier_block *self,
>                         unsigned long val, void *data)
> {
>      struct die_args *args = (struct die_args *)data;
>      int ret = NOTIFY_DONE;
> 
>      switch (val) {
>      case DIE_BREAK:
>          if (kprobe_handler(args->regs))
>              ret = NOTIFY_STOP;
>          break;
> ...
> 
> The !p means this insn has been moved, and then in most cases the COND
> "addr->word != breakpoint_insn.word" is true, we return 1 so that the return
> value in kprobe_exceptions_notify will be changed to NOTIFY_STOP.
> The mips use soft breakpoint like "break hint". How if the original insn
> is same as breakpoint_insn? That is a few case when COND is false. In that
> case, it means we should handle it by other handlers and doesn't change 
> ret to
> keep NOTIFY_DONE as kprobe_exceptions_notify return value.
> 
> Is this idea reasonable? Thanks!

Ah, in that case, yes, you should not return 1. Since the original code is
based on x86, which only has "int3" which is a unique instruction, it is
reasonable to check the breakpoint instruction. But it depends on the
architecture implementation.
BTW, even on x86, I think this case should not be checked by the kprobes.
It should be checked in the arch dependent software break handler code
right before detecting "stray software breakpoint", because this can happen
in other cases.

Thanks for reporting. The condition must be removed on MIPS.


> 
> 
> Jinyang
> 
> >> 			/*
> >> 			 * The breakpoint instruction was removed by
> >> 			 * another cpu right after we hit, no further
> >> 			 * handling of this interrupt is appropriate
> >> 			 */
> >> 			ret = 1;
> >> 		}
> >> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L262
> >> 	p = get_kprobe(addr);
> >> 	if (p) {
> >> 		...
> >> 	} else {
> >> 		if (addr->word != breakpoint_insn.word) {
> >> 			/*
> >> 			 * 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.
> >> 			 */
> >> 			preempt_enable_no_resched();
> >> 			return 1;
> >> 		}
> >> 	}
> >>
> >> (1) arm64
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/probes/kprobes.c#n309
> >> static void __kprobes kprobe_handler(struct pt_regs *regs)
> >> {
> >> 	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.
> >> 			 */
> >> 			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> >> 				setup_singlestep(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.
> >> 	 */
> >> }
> >>
> >> (2) riscv
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/kernel/probes/kprobes.c#n269
> >> bool __kprobes
> >> kprobe_breakpoint_handler(struct pt_regs *regs)
> >> {
> >> 	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 true;
> >> 		} 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.
> >> 			 */
> >> 			if (!p->pre_handler || !p->pre_handler(p, regs))
> >> 				setup_singlestep(p, regs, kcb, 0);
> >> 			else
> >> 				reset_current_kprobe();
> >> 		}
> >> 		return true;
> >> 	}
> >>
> >> 	/*
> >> 	 * 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.
> >> 	 */
> >> 	return false;
> >> }
> >>
> >> (3) csky
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/csky/kernel/probes/kprobes.c#n311
> >> int __kprobes
> >> kprobe_breakpoint_handler(struct pt_regs *regs)
> >> {
> >> 	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 1;
> >> 		} 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.
> >> 			 */
> >> 			if (!p->pre_handler || !p->pre_handler(p, regs))
> >> 				setup_singlestep(p, regs, kcb, 0);
> >> 			else
> >> 				reset_current_kprobe();
> >> 		}
> >> 		return 1;
> >> 	}
> >>
> >> 	/*
> >> 	 * 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.
> >> 	 */
> >> 	return 0;
> >> }
> >>
> >> (4) mips
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n279
> >> static int kprobe_handler(struct pt_regs *regs)
> >> {
> >> 	struct kprobe *p;
> >> 	int ret = 0;
> >> 	kprobe_opcode_t *addr;
> >> 	struct kprobe_ctlblk *kcb;
> >>
> >> 	addr = (kprobe_opcode_t *) regs->cp0_epc;
> >>
> >> 	/*
> >> 	 * We don't want to be preempted for the entire
> >> 	 * duration of kprobe processing
> >> 	 */
> >> 	preempt_disable();
> >> 	kcb = get_kprobe_ctlblk();
> >>
> >> 	/* Check we're not actually recursing */
> >> 	if (kprobe_running()) {
> >> 		p = get_kprobe(addr);
> >> 		if (p) {
> >> 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
> >> 			    p->ainsn.insn->word == breakpoint_insn.word) {
> >> 				regs->cp0_status &= ~ST0_IE;
> >> 				regs->cp0_status |= kcb->kprobe_saved_SR;
> >> 				goto no_kprobe;
> >> 			}
> >> 			/*
> >> 			 * We have reentered the kprobe_handler(), since
> >> 			 * another probe was hit while within the handler.
> >> 			 * We here save the original kprobes variables and
> >> 			 * just single step on the instruction of the new probe
> >> 			 * without calling any user handlers.
> >> 			 */
> >> 			save_previous_kprobe(kcb);
> >> 			set_current_kprobe(p, regs, kcb);
> >> 			kprobes_inc_nmissed_count(p);
> >> 			prepare_singlestep(p, regs, kcb);
> >> 			kcb->kprobe_status = KPROBE_REENTER;
> >> 			if (kcb->flags & SKIP_DELAYSLOT) {
> >> 				resume_execution(p, regs, kcb);
> >> 				restore_previous_kprobe(kcb);
> >> 				preempt_enable_no_resched();
> >> 			}
> >> 			return 1;
> >> 		} else if (addr->word != breakpoint_insn.word) {
> >> 			/*
> >> 			 * The breakpoint instruction was removed by
> >> 			 * another cpu right after we hit, no further
> >> 			 * handling of this interrupt is appropriate
> >> 			 */
> >> 			ret = 1;
> >> 		}
> >> 		goto no_kprobe;
> >> 	}
> >> ...
> >> }
> >>
> >> (5) loongarch
> >> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L228
> >> static int __kprobes kprobe_handler(struct pt_regs *regs)
> >> {
> >> 	struct kprobe *p;
> >> 	kprobe_opcode_t *addr;
> >> 	struct kprobe_ctlblk *kcb;
> >>
> >> 	addr = (kprobe_opcode_t *) regs->csr_era;
> >>
> >> 	/*
> >> 	 * We don't want to be preempted for the entire
> >> 	 * duration of kprobe processing
> >> 	 */
> >> 	preempt_disable();
> >> 	kcb = get_kprobe_ctlblk();
> >>
> >> 	p = get_kprobe(addr);
> >> 	if (p) {
> >> 		if (kprobe_running()) {
> >> 			if (reenter_kprobe(p, regs, kcb))
> >> 				return 1;
> >> 		} else {
> >> 			set_current_kprobe(p, regs, kcb);
> >> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> >> 			if (p->pre_handler && p->pre_handler(p, regs)) {
> >> 			/* handler has already set things up, so skip ss setup */
> >> 				reset_current_kprobe();
> >> 				preempt_enable_no_resched();
> >> 				return 1;
> >> 			} else {
> >> 				setup_singlestep(p, regs, kcb, 0);
> >> 				return 1;
> >> 			}
> >> 		}
> >> 	} else {
> >> 		if (addr->word != breakpoint_insn.word) {
> >> 			/*
> >> 			 * 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.
> >> 			 */
> >> 			preempt_enable_no_resched();
> >> 			return 1;
> >> 		}
> >> 	}
> >>
> >> 	preempt_enable_no_resched();
> >>
> >> 	return 0;
> >> }
> >>
> >> Thanks,
> >> Tiezhu
> >>
> >
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Questions about kprobe handler
  2022-11-24 11:44     ` Naveen N. Rao
@ 2022-11-24 15:03       ` Masami Hiramatsu
  0 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2022-11-24 15:03 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Jinyang He, Tiezhu Yang, Anil S Keshavamurthy, Huacai Chen,
	David S. Miller, linux-kernel, linuxppc-dev

On Thu, 24 Nov 2022 17:14:27 +0530
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com> wrote:

> Jinyang He wrote:
> > 在 2022/11/17 21:09, Masami Hiramatsu (Google) 写道:
> > 
> >> On Thu, 17 Nov 2022 09:07:37 +0800
> >> Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> >>
> >>> Hi KPROBES maintainers,
> >>>
> >>> There are some differences of kprobe handler implementations on various
> >>> archs, the implementations are almost same on arm64, riscv, csky, the
> >>> code logic is clear and understandable. But on mips and loongarch (not
> >>> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
> >>> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
> >>> Can we just return directly? Please take a look, thank you.
> 
> Are you seeing any problem with that?
> 
> >> Good question!
> >>
> >> This means that when the software breakpoint was hit on that CPU, but
> >> before calling kprobe handler function, the other CPU can remove that
> >> kprobe from hash table, becahse the hash table is not locked.
> >> In that case, the get_kprobe(addr) will return NULL, and the software
> >> breakpoint instruction is already removed (replaced with the original
> >> instruction). Thus it is safe to go back. But this is originally
> >> implemented for x86, which doesn't need stop_machine() to modify the
> >> code. On the other hand, if an architecture which needs stop_machine()
> >> to modify code, the above scenario never happen. In that case, you
> >> don't need this "if" case.
> 
> This has been a problematic area on powerpc. See, for instance, commits:
> - 9ed5df69b79a22 ("powerpc/kprobes: Use probe_address() to read instructions")
> - 21f8b2fa3ca5b0 ("powerpc/kprobes: Ignore traps that happened in real mode")
> 
> I think we should close this race, perhaps by instroducing another 
> synchronize_rcu() in unregister_kprobe(), allowing architectures using 
> stop_machine() to override that. That would be cleaner.
> 
> 
> >>
> >> Thank you,
> >>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
> >>> 		p = get_kprobe(addr);
> >>> 		if (p) {
> >>> 			...
> >>> 		} else if (addr->word != breakpoint_insn.word) {
> > 
> > Hi,
> > 
> > 
> > Sorry for the late reply, but I think there should be some public
> > comments so that I can get the authoritative response, as offline
> > communication with Tiezhu is always one-sided.
> > 
> > I think the branch you answered here is " if (p)... " rather than
> > "else if (addr->word != breakpoint_insn.word)". It is right if we
> > not use stop_machine here we need this branch. In fact, Tiezhu
> > and Huacai, the maintainer of LoongArch are more concerned
> > about the latter why we need compare with the breakpoint_insn.
> 
> Masami answered why we need the else part comparing the instruction at 
> addr with the breakpoint instruction. It is to check if the breakpoint 
> instruction has been removed since we hit it, but before the processor 
> reached the kprobe handler.
> 
> > 
> > The reason I gave as follows, and I show mips code here,
> > 
> >      p = get_kprobe(addr);
> >      if (!p) {
> >          if (addr->word != breakpoint_insn.word) {
> >              /*
> >               * 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.
> >               */
> >              ret = 1;
> >          }
> >          /* Not one of ours: let kernel handle it */
> >          goto no_kprobe;
> >      }
> > 
> > ...
> > int kprobe_exceptions_notify(struct notifier_block *self,
> >                         unsigned long val, void *data)
> > {
> >      struct die_args *args = (struct die_args *)data;
> >      int ret = NOTIFY_DONE;
> > 
> >      switch (val) {
> >      case DIE_BREAK:
> >          if (kprobe_handler(args->regs))
> >              ret = NOTIFY_STOP;
> >          break;
> > ...
> > 
> > The !p means this insn has been moved, and then in most cases the COND
> 
> !p means the kprobe has been removed - there may or may not be another 
> breakpoint instruction at that address.
> 
> > "addr->word != breakpoint_insn.word" is true, we return 1 so that the return
> > value in kprobe_exceptions_notify will be changed to NOTIFY_STOP.
> 
> We do this since we now know that there is no breakpoint at this 
> address, so we should not continue.

I think this depends on how the architecture handles the breakpoint.
It may be shared with the other users and in that case, kprobes can not
determine it can stop notifier chain because another notifier callback
is able to handle such cases.

> 
> > The mips use soft breakpoint like "break hint". How if the original insn
> > is same as breakpoint_insn? That is a few case when COND is false. In that
> > case, it means we should handle it by other handlers and doesn't change 
> > ret to
> > keep NOTIFY_DONE as kprobe_exceptions_notify return value.
> 
> If there is a breakpoint instruction at that address, we should let the 
> kernel continue processing the breakpoint.

Even if there is not a breakpoint instruction at that address at that point,
the breakpoint handler is called means there *was* a breakpoint, which can
be managed by another user. Thus I think it is not correct way to check
it in kprobe_int3_andler on x86.

> 
> > 
> > Is this idea reasonable? Thanks!
> 
> As another data point, you may want to look at kprobe_handler() in 
> arch/powerpc/kernel/kprobes.c . We also handle cases where there is a 
> different type of breakpoint instruction.

Yeah, that looks good example for the arch which uses a specific
breakpoint for kprobes. So good for MIPS too.
Unfortunately it doesn't work for x86 since it shares same int3
instruction among all breakpoint users.

Thank you,

> 
> 
> - Naveen
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-24 15:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-17  1:07 Questions about kprobe handler Tiezhu Yang
2022-11-17 13:09 ` Masami Hiramatsu
2022-11-23 15:24   ` Jinyang He
2022-11-24 11:44     ` Naveen N. Rao
2022-11-24 15:03       ` Masami Hiramatsu
2022-11-24 14:13     ` Masami Hiramatsu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).