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, 03 Mar 2022 17:41:22 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH v2 15/39] x86/ibt,kprobes: Fix more +0 assumptions
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     alexei.starovoitov@gmail.com, alyssa.milburn@intel.com,
        andrew.cooper3@citrix.com, hjl.tools@gmail.com,
        joao@overdrivepizza.com, jpoimboe@redhat.com,
        keescook@chromium.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, mbenes@suse.cz,
        Masami Hiramatsu <mhiramat@kernel.org>,
        ndesaulniers@google.com, rostedt@goodmis.org,
        samitolvanen@google.com, x86@kernel.org
References: <20220224151322.892372059@infradead.org>
        <20220228150705.aab2d654b973109bab070ffe@kernel.org>
        <20220228232513.GH11184@worktop.programming.kicks-ass.net>
        <20220301114905.e11146ad69d6e01998101c3b@kernel.org>
        <Yh3ZQQv8GjtqgUF4@hirez.programming.kicks-ass.net>
        <1646154463.4r1sh4kjf0.naveen@linux.ibm.com>
        <20220301191245.GI11184@worktop.programming.kicks-ass.net>
        <20220301200547.GK11184@worktop.programming.kicks-ass.net>
        <1646236764.vx04n8yp12.naveen@linux.ibm.com>
        <Yh/GXaqzDNfp93Jd@hirez.programming.kicks-ass.net>
        <Yh/H+IyKnnC0w5TN@hirez.programming.kicks-ass.net>
In-Reply-To: <Yh/H+IyKnnC0w5TN@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1646305566.4wf34z8v3g.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Peter Zijlstra wrote:
> On Wed, Mar 02, 2022 at 08:32:45PM +0100, Peter Zijlstra wrote:
>> I wonder if you also want to tighten up on_func_entry? Wouldn't the
>> above suggest something like:

Good question ;)
I noticed this yesterday, but held off on making changes so that I can=20
think this through.

>>=20
>> kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long addr, unsigned lo=
ng offset,
>> 					 bool *on_func_entry)
>> {
>> #ifdef PPC64_ELF_ABI_V2
>> 	unsigned long entry =3D ppc_function_entry((void *)addr) - addr;
>> 	*on_func_entry =3D !offset || offset =3D=3D entry;
>> 	if (*on_func_entry)
>> 		offset =3D entry;
>> #else
>> 	*on_func_entry =3D !offset;
>> #endif
>> 	return (void *)(addr + offset);
>> }

This is more accurate and probably something we should do in the long=20
term. The main issue I see today is userspace, specifically perf (and=20
perhaps others too).

Historically, we have worked around the issue of probes on a function's=20
global entry point not working by having userspace adjust the offset at=20
which probes are placed. This works well if those object files have=20
either the symbol table, or debuginfo capturing if functions have a=20
separate local entry point. In the absence of those, we are left=20
guessing and we chose to just offset all probes at function entry by 8=20
(GEP almost always has the same two instructions) so that perf "just=20
works". This still works well for functions without a GEP since we=20
expect to see the two ftrace instructions at function entry, so we are=20
ok to probe after that. As an added bonus, this also allows uprobes to=20
work, for the most part.

On the kernel side, we only implemented logic to adjust probe address if=20
a function name was specified without an offset. This went for a toss=20
once perf probe moved to using _text as the base symbol for kprobes=20
though, and we weren't handling scenarios where addr+offset was=20
provided. With the changes in this series, we can now adjust kprobe=20
address across all those scenarios properly.

If we update perf to not pass an offset any more, then newer perf will=20
stop working on older kernels. If we make the logic to determine=20
function entry strict in the kernel, then we risk breaking existing=20
userspace.

I'm not sure how best to address this.

>=20
> One question though; the above seems to work for +0 or +8 (IIRC your
> instructions are 4 bytes each and the GEP is 2 instructions).
>=20
> But what do we want to happen for +4 ?

We don't want to change the behavior of probes at the second instruction=20
in GEP. The thinking is that it allows the rare scenario (if at all) of=20
wanting to catch indirect function calls, and/or cross-module function=20
calls -- especially since we now promote probes at GEP to LEP. I frankly=20
know of no such scenarios so far, but in any case, if the user is=20
specifying an offset, they better know what they are asking for :)

For the same reason, we should allow kretprobe at +4.


Thanks,
Naveen