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:   Mon, 16 Jan 2023 14:02:04 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add
 multiple consecutive probes in a function
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     Akanksha J N <akanksha@linux.ibm.com>,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-trace-kernel@vger.kernel.org, rostedt@goodmis.org,
        shuah@kernel.org
References: <20230112095600.37665-1-akanksha@linux.ibm.com>
        <1673529279.3c5f8oes3z.naveen@linux.ibm.com>
        <20230113005153.c6ca2f75b9d12627eb63308a@kernel.org>
        <1673601511.tq30r5phea.naveen@linux.ibm.com>
        <20230114002126.a37640f815b74e9e78259a9f@kernel.org>
In-Reply-To: <20230114002126.a37640f815b74e9e78259a9f@kernel.org>
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1673856229.a7tekgas75.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

Masami Hiramatsu wrote:
> Hi Naveen,
>=20
> On Fri, 13 Jan 2023 14:59:51 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> Masami Hiramatsu wrote:
>> > On Thu, 12 Jan 2023 18:51:14 +0530
>> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> >=20
>> >> Akanksha J N wrote:
>> >> > Commit 97f88a3d723162 ("powerpc/kprobes: Fix null pointer reference=
 in
>> >> > arch_prepare_kprobe()") fixed a recent kernel oops that was caused =
as
>> >> > ftrace-based kprobe does not generate kprobe::ainsn::insn and it ge=
ts
>> >> > set to NULL.
>> >> > Extend multiple kprobes test to add kprobes on first 256 bytes with=
in a
>> >> > function, to be able to test potential issues with kprobes on
>> >> > successive instructions.
>> >=20
>> > What is the purpose of that test? If you intended to add a kprobe even=
ts
>> > with some offset so that it becomes ftrace-based kprobe, it should be
>> > a different test case, because
>>=20
>> This is a follow up to:
>> http://lore.kernel.org/1664530538.ke6dp49pwh.naveen@linux.ibm.com
>>=20
>> The intent is to add consecutive probes covering KPROBES_ON_FTRACE,=20
>> vanilla trap-based kprobes as well as optprobes to ensure all of those=20
>> and their interactions are good.
>=20
> Hmm, that should be implemented for each architecture with specific
> knowledge instead of random offset, so that we can ensure the kprobe
> is on ftrace/optimized or using trap. Also, it should check the
> debugfs/kprobes/list file.

...

>=20
>>=20
>> >=20
>> >  - This is a test case for checking multiple (at least 256) kprobe eve=
nts
>> >   can be defined and enabled.
>> >=20
>> >  - If you want to check the ftrace-based kprobe, it should be near the
>> >    function entry, maybe within 16 bytes or so.
>> >=20
>> >  - Also, you don't need to enable it at once (and should not for this =
case).
>> >=20
>> >> > The '|| true' is added with the echo statement to ignore errors tha=
t are
>> >> > caused by trying to add kprobes to non probeable lines and continue=
 with
>> >> > the test.
>> >=20
>> > Can you add another test case for that? (and send it to the MLs which =
Cc'd
>> > to this mail)
>> > e.g.=20
>> >=20
>> >    for i in `seq 0 16`; do
>> >      echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue
>> >      echo 1 > events/kprobes/testprobe/enable
>> >      ( echo "forked" )
>> >      echo 0 > events/kprobes/testprobe/enable
>> >      echo > kprobe_events
>> >    done
>>=20
>> The current test to add multiple kprobes within a function also falls=20
>> under the purview of multiple_kprobes.tc, but it can be split into a=20
>> separate multiple_kprobes_func.tc if you think that will be better.
>>=20
>=20
> Yes, please make it separate, this test case is for checking whether
> the ftrace can define/enable/disable multiple kprobe events. Not for
> checking kprobe with different types, nor checking interactions among
> different types of kprobes.
>=20
> (BTW, if you want to test optprobe on x86, you can not put the probes
>  within the jump instruction (+5 bytes). It will unoptimize existing
>  optimized kprobe in that case)

Ok, I can see why we won't be able to optimize any of the probes on x86=20
with this approach. But, we should be able to do so on powerpc and arm,=20
the only other architectures supporting OPTPROBES at this time. For x86,=20
we may have to extend the test to check kprobes/list.

Crucially, I think trying to place a probe at each byte can still=20
exercize interactions across KPROBES_ON_FTRACE and normal kprobes, so=20
this test is still a good start. In addition, we get to ensure that=20
kprobes infrastructure is rejecting placing probes at non-instruction=20
boundaries.

>=20
> And do you really need to run "multiple" kprobes at once?
> I think what you need is 'kprobe_opt_types.tc'.

Yes, enabling those probes is a good stress test to ensure we are only=20
accepting valid probe locations.

multiple_kprobe_types.tc ? :)


Thanks,
Naveen