Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 17:14:27 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
Subject: Re: Questions about kprobe handler
To:     Jinyang He <hejinyang@loongson.cn>,
        "Masami Hiramatsu (Google)" <mhiramat@kernel.org>,
        Tiezhu Yang <yangtiezhu@loongson.cn>
Cc:     Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
        Huacai Chen <chenhuacai@loongson.cn>,
        "David S. Miller" <davem@davemloft.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linuxppc-dev@lists.ozlabs.org
References: <9b6eb4db-83fe-e6d9-a580-1a11aace84b0@loongson.cn>
        <20221117220915.8b233ec82dc10a84753150b4@kernel.org>
        <7e4143dc-c444-e497-43bb-ac0ba18b6691@loongson.cn>
In-Reply-To: <7e4143dc-c444-e497-43bb-ac0ba18b6691@loongson.cn>
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1669288280.gcxbuppl5k.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

Jinyang He wrote:
> =E5=9C=A8 2022/11/17 21:09, Masami Hiramatsu (Google) =E5=86=99=E9=81=93:
>=20
>> 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 !=3D 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 instruction=
s")
- 21f8b2fa3ca5b0 ("powerpc/kprobes: Ignore traps that happened in real mode=
")

I think we should close this race, perhaps by instroducing another=20
synchronize_rcu() in unregister_kprobe(), allowing architectures using=20
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 =3D get_kprobe(addr);
>>> 		if (p) {
>>> 			...
>>> 		} else if (addr->word !=3D breakpoint_insn.word) {
>=20
> Hi,
>=20
>=20
> 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.
>=20
> I think the branch you answered here is " if (p)... " rather than
> "else if (addr->word !=3D 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=20
addr with the breakpoint instruction. It is to check if the breakpoint=20
instruction has been removed since we hit it, but before the processor=20
reached the kprobe handler.

>=20
> The reason I gave as follows, and I show mips code here,
>=20
>  =C2=A0=C2=A0 =C2=A0p =3D get_kprobe(addr);
>  =C2=A0=C2=A0 =C2=A0if (!p) {
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 if (addr->word !=3D breakpoint_ins=
n.word) {
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 /*
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0* The bre=
akpoint instruction was removed right
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0* after w=
e hit it.=C2=A0 Another cpu has removed
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0* either =
a probepoint or a debugger breakpoint
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0* at this=
 address.=C2=A0 In either case, no further
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0* handlin=
g of this interrupt is appropriate.
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0*/
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ret =3D 1;
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 }
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 /* Not one of ours: let kernel han=
dle it */
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 goto no_kprobe;
>  =C2=A0=C2=A0 =C2=A0}
>=20
> ...
> int kprobe_exceptions_notify(struct notifier_block *self,
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=
=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long val, void *data)
> {
>  =C2=A0=C2=A0 =C2=A0struct die_args *args =3D (struct die_args *)data;
>  =C2=A0=C2=A0 =C2=A0int ret =3D NOTIFY_DONE;
>=20
>  =C2=A0=C2=A0 =C2=A0switch (val) {
>  =C2=A0=C2=A0 =C2=A0case DIE_BREAK:
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 if (kprobe_handler(args->regs))
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ret =3D NOTIFY_=
STOP;
>  =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 break;
> ...
>=20
> 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=20
breakpoint instruction at that address.

> "addr->word !=3D breakpoint_insn.word" is true, we return 1 so that the r=
eturn
> 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=20
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=20
> ret to
> keep NOTIFY_DONE as kprobe_exceptions_notify return value.

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

>=20
> Is this idea reasonable? Thanks!

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


- Naveen