在 2022/11/17 21:09, Masami Hiramatsu (Google) 写道:
On Thu, 17 Nov 2022 09:07:37 +0800
Tiezhu Yang <yangtiezhu@xxxxxxxxxxx> 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!