Message-ID: <20241105195208.GC29862@gate.crashing.org>
Date: Tue, 5 Nov 2024 13:52:08 -0600
From: Segher Boessenkool <segher@...nel.crashing.org>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Hari Bathini <hbathini@...ux.ibm.com>, Shuah Khan <shuah@...nel.org>,
        linux-kselftest@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        "Naveen N. Rao" <naveen@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        linux-trace-kernel@...r.kernel.org,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

Hi!

On Tue, Nov 05, 2024 at 06:17:51PM +0900, Masami Hiramatsu wrote:
> On Tue, 5 Nov 2024 02:20:18 -0600
> Segher Boessenkool <segher@...nel.crashing.org> wrote:
> > On Mon, Nov 04, 2024 at 11:06:23PM +0530, Hari Bathini wrote:
> > > Seems like a bit of misunderstanding there. Function entry here intends
> > > to mean the actual start of function code (function prologue) - after
> > > GEP and function profiling sequence (mflr r0; bl mcount).
> > 
> > What you call "function entry" here simply does not exist.  The compiler
> > can -- and ***WILL***, ***DOES*** -- mix up all of that.
> 
> Here is the "function entry" means the function address.

"Function entry point".  "Function entry" can mean whatever nebulous
thing done at the start of a function :-)

You're free to use your own terminology of course, but it help to use
standard names for standard things!

> Not the prologue.

But that is literally what Hari said, so it confused me.

> On some architecture, we are sure fixed sequences
> right after the function address for ftrace/security. For example,
> x86 has an `ENDBR` for security. Thus, even if we tend to put a
> probe on the "function entry", kprobes shifts the probe point
> forcibly skipping the `ENDBR`. So from the probe callback, the
> probed address does not look like the function address (shift
> the sizeof(ENDBR)).

On almmost all architectures and ABIs the prologues aren't so very
fixed, which is a good thing, because typically the compiler can
make things better in some way (typically faster or smaller code).  Or
other parts of the toolchain can, the loader or dynamic loader often.

> However, the ENDBR does nothing from the program point of view, we
> can still think of that address as the address of the function.
> That is the reason why I introduced arch_kprobe_on_func_entry().

Understood.

> For the other architecture, it might be misunderstood and
> could be miss-implemented. In that case, we should fix that.
> 
> >  In particular,
> > "function prologue" does not exist at all (on any architecture worth
> > its salt, including PowerPC), and all instructions you consider part of
> > a function prologue might end up anywhere.  The "profiling sequence" is
> > part of that btw, and that typically ends up *not* the first thing in
> > the function, not the first thing after the LEP (register saves are
> > earlier often, they are generated in that order in the first place,
> > but they can (and will) be moved if that schedules better).
> > 
> > > Function arguments can be accessed with kprobe only while setting a
> > > probe at an address the kernel treats as function start address.
> > 
> > That is a silly assumption to make.  There is no guarantee you can
> > access function arguments *at all*, we're not in 1975 anymore.  You
> > *need* to look at debug information if you want to deal with anything
> > about your high-level language program.  Looking at the machine code
> > can only tell you about the machine state, whatever is in registers
> > etc.
> 
> Yeah, understood. So the `$arg*` here does not guarantee to access
> arguments, but the best effort to do that. And it fully depends on

Is that GDB syntax?  Or what else?

> regs_get_kernel_argument(). Thus `$arg*` works only where the
> regs_get_kernel_argument() can return most likely function argument
> value from `pt_regs`. That is where we call "function entry" in
> this context.
> 
> And since it checks the function entry by arch_kprobe_on_func_entry()
> this test fails on powerpc because it returns true if the offset from
> the kallsyms symbol address is less than 8/16 bytes.
> 
> > > Note that the test case pass criteria here is setting probe to fail by
> > > providing an address (sym+offset) beyond the function start address.
> > > 
> > > And in this specific test case (with "vfs_read+8", where vfs_read is
> > > the symbol and '8' is the offset), the test case was failing on powerpc
> > > because setting the probe at 'sym+8' was succeeding, as anywhere between
> > > 'sym' to 'sym+16' is treated as function start address on powerpc:
> > 
> > Yeah, fragile tests sometimes break.  Changing a randomly chosen number
> > to some other randomly chosen number will not fix the problem (but you
> > can postpone having to deal with it, sure!)
> 
> Yeah, sorry about the test case. Actually `+8` is also not a good number
> for x86 too since we are not sure whether the address is an instruction
> boundary or not. In that case it may report another error and failed.

So what is it that the testcase really wants to test for?

Thanks for explaining the context somewhat, I know nothing (except all
details about ELFv2 and the other PowerPC ABIs :-) ), it helped :-)


Segher

Powered by blists - more mailing lists