[PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Hari Bathini]

For ppc64le, depending on the kernel configuration used, offset 16
from function start address can also be considered function entry.
Update the test case to accommodate such configurations.

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---
 .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc    | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
index a16c6a6f6055..c03b94cc5784 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
@@ -111,7 +111,11 @@ check_error 'p vfs_read $arg* ^$arg*'		# DOUBLE_ARGS
 if !grep -q 'kernel return probes support:' README; then
 check_error 'r vfs_read ^$arg*'			# NOFENTRY_ARGS
 fi
+if [ "$(uname -m)" = "ppc64le" ]; then
+check_error 'p vfs_read+20 ^$arg*'		# NOFENTRY_ARGS
+else
 check_error 'p vfs_read+8 ^$arg*'		# NOFENTRY_ARGS
+fi
 check_error 'p vfs_read ^hoge'			# NO_BTFARG
 check_error 'p kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
 check_error 'r kfree ^$retval'			# NO_RETVAL
-- 
2.47.0

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Segher Boessenkool]

Hi!

On Sat, Nov 02, 2024 at 12:49:25AM +0530, Hari Bathini wrote:
> For ppc64le, depending on the kernel configuration used, offset 16
> from function start address can also be considered function entry.
> Update the test case to accommodate such configurations.

(This is true for all ELfv2, not just LE.  For the kernel that is about
the same).

The LEP and GEP can differ by zero, one, two, four, eight, or sixteen
insns (where an insn is four bytes).  Four insns is common, yes, but
maybe you can support all?  See the function symbol's st_other field
to see what the offset is:
0, 1: zero insns, zero bytes
N = 2..6: 1 << (N-2) insns, i.e. 1<<N bytes
7: reserved

(This is the top 3 bits of st_other, the other bits have other meanings).

Four insns is common, yes, but by no means the only possibility.


Segher

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Hari Bathini]

On 02/11/24 2:29 am, Segher Boessenkool wrote:
> Hi!
> 
> On Sat, Nov 02, 2024 at 12:49:25AM +0530, Hari Bathini wrote:
>> For ppc64le, depending on the kernel configuration used, offset 16
>> from function start address can also be considered function entry.
>> Update the test case to accommodate such configurations.
> 
> (This is true for all ELfv2, not just LE.  For the kernel that is about
> the same).
> 
> The LEP and GEP can differ by zero, one, two, four, eight, or sixteen
> insns (where an insn is four bytes).  Four insns is common, yes, but
> maybe you can support all?  See the function symbol's st_other field
> to see what the offset is:
> 0, 1: zero insns, zero bytes
> N = 2..6: 1 << (N-2) insns, i.e. 1<<N bytes
> 7: reserved
> 
> (This is the top 3 bits of st_other, the other bits have other meanings).
> 
> Four insns is common, yes, but by no means the only possibility.

Hi Segher,

Querying for function arguments is supported on kprobes only at function
entry. This is a negative test case where the offset is intentionally
set beyond function entry while querying for function arguments.
I guess, simply setting the offset to 20 (vfs_read is anyway
going to be beyond 5 instructions) instead of 8 for powerpc would
make all platforms and ABI variants happy?

Thanks
Hari

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Segher Boessenkool]

Hi!

On Mon, Nov 04, 2024 at 02:51:57PM +0530, Hari Bathini wrote:
> On 02/11/24 2:29 am, Segher Boessenkool wrote:
> >On Sat, Nov 02, 2024 at 12:49:25AM +0530, Hari Bathini wrote:
> >>For ppc64le, depending on the kernel configuration used, offset 16
> >>from function start address can also be considered function entry.
> >>Update the test case to accommodate such configurations.
> >
> >(This is true for all ELfv2, not just LE.  For the kernel that is about
> >the same).
> >
> >The LEP and GEP can differ by zero, one, two, four, eight, or sixteen
> >insns (where an insn is four bytes).  Four insns is common, yes, but
> >maybe you can support all?  See the function symbol's st_other field
> >to see what the offset is:
> >0, 1: zero insns, zero bytes
> >N = 2..6: 1 << (N-2) insns, i.e. 1<<N bytes
> >7: reserved
> >
> >(This is the top 3 bits of st_other, the other bits have other meanings).
> >
> >Four insns is common, yes, but by no means the only possibility.
> 
> Hi Segher,
> 
> Querying for function arguments is supported on kprobes only at function
> entry. This is a negative test case where the offset is intentionally
> set beyond function entry while querying for function arguments.
> I guess, simply setting the offset to 20 (vfs_read is anyway
> going to be beyond 5 instructions) instead of 8 for powerpc would
> make all platforms and ABI variants happy?

I have no idea.  What is this "offset" anyway?

This is just the ELFv2 ABI.  No platform can make up its own thing at
all (well, none decided to be gratuitously incompatible, so far).  And
there are no "ABI variants"!

You're just making assumptions here that are based on nothing else but
observations of what is done most of the time.  That might work for a
while -- maybe a long while even! -- but it can easily break down.


Segher

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Hari Bathini]

On 04/11/24 3:14 pm, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Nov 04, 2024 at 02:51:57PM +0530, Hari Bathini wrote:
>> On 02/11/24 2:29 am, Segher Boessenkool wrote:
>>> On Sat, Nov 02, 2024 at 12:49:25AM +0530, Hari Bathini wrote:
>>>> For ppc64le, depending on the kernel configuration used, offset 16
>>> >from function start address can also be considered function entry.
>>>> Update the test case to accommodate such configurations.
>>>
>>> (This is true for all ELfv2, not just LE.  For the kernel that is about
>>> the same).
>>>
>>> The LEP and GEP can differ by zero, one, two, four, eight, or sixteen
>>> insns (where an insn is four bytes).  Four insns is common, yes, but
>>> maybe you can support all?  See the function symbol's st_other field
>>> to see what the offset is:
>>> 0, 1: zero insns, zero bytes
>>> N = 2..6: 1 << (N-2) insns, i.e. 1<<N bytes
>>> 7: reserved
>>>
>>> (This is the top 3 bits of st_other, the other bits have other meanings).
>>>
>>> Four insns is common, yes, but by no means the only possibility.
>>
>> Hi Segher,
>>
>> Querying for function arguments is supported on kprobes only at function
>> entry. This is a negative test case where the offset is intentionally
>> set beyond function entry while querying for function arguments.
>> I guess, simply setting the offset to 20 (vfs_read is anyway
>> going to be beyond 5 instructions) instead of 8 for powerpc would
>> make all platforms and ABI variants happy?
> 
> I have no idea.  What is this "offset" anyway?

offset (in bytes) from function start address..

> 
> This is just the ELFv2 ABI.  No platform can make up its own thing at
> all (well, none decided to be gratuitously incompatible, so far).  And
> there are no "ABI variants"!

The test case applies for ABIv1 & ABIv2. All ppc32 & ppc64 platforms..

> 
> You're just making assumptions here that are based on nothing else but
> observations of what is done most of the time.  That might work for a
> while -- maybe a long while even! -- but it can easily break down.

Hmmm.. I understand that you want the test case to read st_other field
but would you rather suggest an offset of 64?
Is a GEP of 8/16 instructions going to be true anytime soon or is it
true already for some cases? The reason I ask that is some kprobe/ftrace
code in the kernel might need a bit of re-look if that is the case.

Thanks
Hari

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Segher Boessenkool]

Hi!

On Mon, Nov 04, 2024 at 03:40:26PM +0530, Hari Bathini wrote:
> On 04/11/24 3:14 pm, Segher Boessenkool wrote:
> >On Mon, Nov 04, 2024 at 02:51:57PM +0530, Hari Bathini wrote:
> >>On 02/11/24 2:29 am, Segher Boessenkool wrote:
> >>>On Sat, Nov 02, 2024 at 12:49:25AM +0530, Hari Bathini wrote:
> >>>>For ppc64le, depending on the kernel configuration used, offset 16
> >>>>from function start address can also be considered function entry.
> >>>>Update the test case to accommodate such configurations.
> >>>
> >>>(This is true for all ELfv2, not just LE.  For the kernel that is about
> >>>the same).
> >>>
> >>>The LEP and GEP can differ by zero, one, two, four, eight, or sixteen
> >>>insns (where an insn is four bytes).  Four insns is common, yes, but
> >>>maybe you can support all?  See the function symbol's st_other field
> >>>to see what the offset is:
> >>>0, 1: zero insns, zero bytes
> >>>N = 2..6: 1 << (N-2) insns, i.e. 1<<N bytes
> >>>7: reserved
> >>>
> >>>(This is the top 3 bits of st_other, the other bits have other meanings).
> >>>
> >>>Four insns is common, yes, but by no means the only possibility.
> >>
> >>Hi Segher,
> >>
> >>Querying for function arguments is supported on kprobes only at function
> >>entry. This is a negative test case where the offset is intentionally
> >>set beyond function entry while querying for function arguments.
> >>I guess, simply setting the offset to 20 (vfs_read is anyway
> >>going to be beyond 5 instructions) instead of 8 for powerpc would
> >>make all platforms and ABI variants happy?
> >
> >I have no idea.  What is this "offset" anyway?
> 
> offset (in bytes) from function start address..

But what is there?

> >This is just the ELFv2 ABI.  No platform can make up its own thing at
> >all (well, none decided to be gratuitously incompatible, so far).  And
> >there are no "ABI variants"!
> 
> The test case applies for ABIv1 & ABIv2. All ppc32 & ppc64 platforms..

Hrm.  So you allow essentially random entry points on other ABIs to
work?

> >You're just making assumptions here that are based on nothing else but
> >observations of what is done most of the time.  That might work for a
> >while -- maybe a long while even! -- but it can easily break down.
> 
> Hmmm.. I understand that you want the test case to read st_other field
> but would you rather suggest an offset of 64?

I have no idea what "offset" means here.

> Is a GEP of 8/16 instructions going to be true anytime soon or is it
> true already for some cases? The reason I ask that is some kprobe/ftrace
> code in the kernel might need a bit of re-look if that is the case.

An entry point has no instructions at all.  Oh, you mean the code at
the GEP.

The LEP can already be all the allowed distances after the GEP.  And
the .localentry GAS directive already supports all those distances
always.  Not a lot of code written in assembler does use that, and
certainly GCC does not use a lot of the freedom it has here, but it
could (and so could assembler programmers).  Typically people will want
to make the code here as short as possible, and there are restrictions
on what is *allowed* to be done here anyway (ld, the link editor, can
change this code after all!), so it is not too likely you will ever see
big code at the GEP often, but times change, etc.


Segher

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Steven Rostedt]

On Mon, 4 Nov 2024 04:36:15 -0600
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > >>Querying for function arguments is supported on kprobes only at function
> > >>entry. This is a negative test case where the offset is intentionally
> > >>set beyond function entry while querying for function arguments.
> > >>I guess, simply setting the offset to 20 (vfs_read is anyway
> > >>going to be beyond 5 instructions) instead of 8 for powerpc would
> > >>make all platforms and ABI variants happy?  
> > >
> > >I have no idea.  What is this "offset" anyway?  
> > 
> > offset (in bytes) from function start address..  
> 
> But what is there?

Function start address is what kallsyms returns. That is:

  grep function /proc/kallsyms

> 
> > >This is just the ELFv2 ABI.  No platform can make up its own thing at
> > >all (well, none decided to be gratuitously incompatible, so far).  And
> > >there are no "ABI variants"!  
> > 
> > The test case applies for ABIv1 & ABIv2. All ppc32 & ppc64 platforms..  
> 
> Hrm.  So you allow essentially random entry points on other ABIs to
> work?
> 
> > >You're just making assumptions here that are based on nothing else but
> > >observations of what is done most of the time.  That might work for a
> > >while -- maybe a long while even! -- but it can easily break down.  
> > 
> > Hmmm.. I understand that you want the test case to read st_other field
> > but would you rather suggest an offset of 64?  
> 
> I have no idea what "offset" means here.

The offset is the number of bytes from the address that is returned by
kallsyms.


> 
> > Is a GEP of 8/16 instructions going to be true anytime soon or is it
> > true already for some cases? The reason I ask that is some kprobe/ftrace
> > code in the kernel might need a bit of re-look if that is the case.  
> 
> An entry point has no instructions at all.  Oh, you mean the code at
> the GEP.
> 
> The LEP can already be all the allowed distances after the GEP.  And
> the .localentry GAS directive already supports all those distances
> always.  Not a lot of code written in assembler does use that, and
> certainly GCC does not use a lot of the freedom it has here, but it
> could (and so could assembler programmers).  Typically people will want
> to make the code here as short as possible, and there are restrictions
> on what is *allowed* to be done here anyway (ld, the link editor, can
> change this code after all!), so it is not too likely you will ever see
> big code at the GEP often, but times change, etc.


This is all determined by the kernel. It's considered a function entry by
the function:

   arch_kprobe_on_func_entry()

Which on PowerPC has:

static bool arch_kprobe_on_func_entry(unsigned long offset)
{
#ifdef CONFIG_PPC64_ELF_ABI_V2
#ifdef CONFIG_KPROBES_ON_FTRACE
        return offset <= 16;
#else
        return offset <= 8;
#endif
#else
        return !offset;
#endif  
}

So, being greater than 16 on powerpc with config CONFIG_PPC64_ELF_ABI_V2
set, would work. If that function changes, then the test needs to change.

-- Steve

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Hari Bathini]

Hi Segher,

On 04/11/24 4:06 pm, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Nov 04, 2024 at 03:40:26PM +0530, Hari Bathini wrote:
>> On 04/11/24 3:14 pm, Segher Boessenkool wrote:
>>> On Mon, Nov 04, 2024 at 02:51:57PM +0530, Hari Bathini wrote:
>>>> On 02/11/24 2:29 am, Segher Boessenkool wrote:
>>>>> On Sat, Nov 02, 2024 at 12:49:25AM +0530, Hari Bathini wrote:
>>>>>> For ppc64le, depending on the kernel configuration used, offset 16
>>>>> >from function start address can also be considered function entry.
>>>>>> Update the test case to accommodate such configurations.
>>>>>
>>>>> (This is true for all ELfv2, not just LE.  For the kernel that is about
>>>>> the same).
>>>>>
>>>>> The LEP and GEP can differ by zero, one, two, four, eight, or sixteen
>>>>> insns (where an insn is four bytes).  Four insns is common, yes, but
>>>>> maybe you can support all?  See the function symbol's st_other field
>>>>> to see what the offset is:
>>>>> 0, 1: zero insns, zero bytes
>>>>> N = 2..6: 1 << (N-2) insns, i.e. 1<<N bytes
>>>>> 7: reserved
>>>>>
>>>>> (This is the top 3 bits of st_other, the other bits have other meanings).
>>>>>
>>>>> Four insns is common, yes, but by no means the only possibility.
>>>>
>>>> Hi Segher,
>>>>
>>>> Querying for function arguments is supported on kprobes only at function
>>>> entry. This is a negative test case where the offset is intentionally
>>>> set beyond function entry while querying for function arguments.
>>>> I guess, simply setting the offset to 20 (vfs_read is anyway
>>>> going to be beyond 5 instructions) instead of 8 for powerpc would
>>>> make all platforms and ABI variants happy?
>>>
>>> I have no idea.  What is this "offset" anyway?
>>
>> offset (in bytes) from function start address..
> 
> But what is there?
> 
>>> This is just the ELFv2 ABI.  No platform can make up its own thing at
>>> all (well, none decided to be gratuitously incompatible, so far).  And
>>> there are no "ABI variants"!
>>
>> The test case applies for ABIv1 & ABIv2. All ppc32 & ppc64 platforms..
> 
> Hrm.  So you allow essentially random entry points on other ABIs to
> work?
> 
>>> You're just making assumptions here that are based on nothing else but
>>> observations of what is done most of the time.  That might work for a
>>> while -- maybe a long while even! -- but it can easily break down.
>>
>> Hmmm.. I understand that you want the test case to read st_other field
>> but would you rather suggest an offset of 64?
> 
> I have no idea what "offset" means here.
> 
>> Is a GEP of 8/16 instructions going to be true anytime soon or is it
>> true already for some cases? The reason I ask that is some kprobe/ftrace
>> code in the kernel might need a bit of re-look if that is the case.
> 
> An entry point has no instructions at all.  Oh, you mean the code at
> the GEP.
> 
> The LEP can already be all the allowed distances after the GEP.  And
> the .localentry GAS directive already supports all those distances
> always.  Not a lot of code written in assembler does use that, and
> certainly GCC does not use a lot of the freedom it has here, but it
> could (and so could assembler programmers).  Typically people will want
> to make the code here as short as possible, and there are restrictions
> on what is *allowed* to be done here anyway (ld, the link editor, can
> change this code after all!), so it is not too likely you will ever see
> big code at the GEP often, but times change, etc.

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).

Function arguments can be accessed with kprobe only while setting a
probe at an address the kernel treats as function start address.
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:

  
https://github.com/torvalds/linux/blob/master/arch/powerpc/kernel/kprobes.c#L108


So, the fix here essentially is to provide an address that is at least
an insn or two beyond function start address. As GEP is 8 bytes and
function profile sequence is 8 bytes, sym+20 is beyond function start
address on ppc64le. In fact, sym+20 should work for other platforms
too as sym+20 not treated as function start address on any platform
on powerpc as of today, and that is all the test case cares about...

Thanks
Hari

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Segher Boessenkool]

Hi!

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.  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.

> 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!)


Segher

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Masami Hiramatsu]

Hi,

On Tue, 5 Nov 2024 02:20:18 -0600
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!
> 
> 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.
Not the prologue. 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)).

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().

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
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.

Thank you,

> 
> 
> Segher


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Segher Boessenkool]

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@kernel.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

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Hari Bathini]

On 06/11/24 1:22 am, Segher Boessenkool wrote:
> 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@kernel.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.

Sorry about that. I should have said.. maybe prologue or whatever
nebulous thing at the start of a function :-)

Basically, the address provided to test case can be any insn in the
function code expect what the kernel considers function entry address..

Thanks
Hari

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Masami Hiramatsu]

On Sat,  2 Nov 2024 00:49:25 +0530
Hari Bathini <hbathini@linux.ibm.com> wrote:

> For ppc64le, depending on the kernel configuration used, offset 16
> from function start address can also be considered function entry.
> Update the test case to accommodate such configurations.
> 

Hi Hari, so have you met any error on this test case?
Can you share the error result too?

Thank you,

> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
>  .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc    | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> index a16c6a6f6055..c03b94cc5784 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> @@ -111,7 +111,11 @@ check_error 'p vfs_read $arg* ^$arg*'		# DOUBLE_ARGS
>  if !grep -q 'kernel return probes support:' README; then
>  check_error 'r vfs_read ^$arg*'			# NOFENTRY_ARGS
>  fi
> +if [ "$(uname -m)" = "ppc64le" ]; then
> +check_error 'p vfs_read+20 ^$arg*'		# NOFENTRY_ARGS
> +else
>  check_error 'p vfs_read+8 ^$arg*'		# NOFENTRY_ARGS
> +fi
>  check_error 'p vfs_read ^hoge'			# NO_BTFARG
>  check_error 'p kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
>  check_error 'r kfree ^$retval'			# NO_RETVAL
> -- 
> 2.47.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Hari Bathini]

On 03/11/24 10:27 am, Masami Hiramatsu (Google) wrote:
> On Sat,  2 Nov 2024 00:49:25 +0530
> Hari Bathini <hbathini@linux.ibm.com> wrote:
> 
>> For ppc64le, depending on the kernel configuration used, offset 16
>> from function start address can also be considered function entry.
>> Update the test case to accommodate such configurations.
>>
> 
> Hi Hari, so have you met any error on this test case?

Hi Masami,

vfs_read+8 is function entry on powerpc. So, the test case bails out at:
   "check_error 'p vfs_read+8 ^$arg*'		# NOFENTRY_ARGS"

as it allows setting kprobe "vfs_read+8 $arg*"

> Can you share the error result too?


End of the log file for reference:

"
Test command: p vfs_read $arg* $arg*
[2661828.483436] trace_kprobe: error: $arg* can be used only once in the 
parameters
   Command: p vfs_read $arg* $arg*
                             ^
Test command: p vfs_read+8 $arg*
"

Thanks
Hari

> 
> Thank you,
> 
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>>   .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc    | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
>> index a16c6a6f6055..c03b94cc5784 100644
>> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
>> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
>> @@ -111,7 +111,11 @@ check_error 'p vfs_read $arg* ^$arg*'		# DOUBLE_ARGS
>>   if !grep -q 'kernel return probes support:' README; then
>>   check_error 'r vfs_read ^$arg*'			# NOFENTRY_ARGS
>>   fi
>> +if [ "$(uname -m)" = "ppc64le" ]; then
>> +check_error 'p vfs_read+20 ^$arg*'		# NOFENTRY_ARGS
>> +else
>>   check_error 'p vfs_read+8 ^$arg*'		# NOFENTRY_ARGS
>> +fi
>>   check_error 'p vfs_read ^hoge'			# NO_BTFARG
>>   check_error 'p kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
>>   check_error 'r kfree ^$retval'			# NO_RETVAL
>> -- 
>> 2.47.0
>>
> 
> 

Re: [PATCH] selftests/ftrace: update kprobe syntax error test for ppc64le

[Masami Hiramatsu]

On Mon, 4 Nov 2024 15:02:12 +0530
Hari Bathini <hbathini@linux.ibm.com> wrote:

> 
> 
> On 03/11/24 10:27 am, Masami Hiramatsu (Google) wrote:
> > On Sat,  2 Nov 2024 00:49:25 +0530
> > Hari Bathini <hbathini@linux.ibm.com> wrote:
> > 
> >> For ppc64le, depending on the kernel configuration used, offset 16
> >> from function start address can also be considered function entry.
> >> Update the test case to accommodate such configurations.
> >>
> > 
> > Hi Hari, so have you met any error on this test case?
> 
> Hi Masami,
> 
> vfs_read+8 is function entry on powerpc. So, the test case bails out at:
>    "check_error 'p vfs_read+8 ^$arg*'		# NOFENTRY_ARGS"
> 
> as it allows setting kprobe "vfs_read+8 $arg*"
> 
> > Can you share the error result too?
> 
> 
> End of the log file for reference:
> 
> "
> Test command: p vfs_read $arg* $arg*
> [2661828.483436] trace_kprobe: error: $arg* can be used only once in the 
> parameters
>    Command: p vfs_read $arg* $arg*
>                              ^
> Test command: p vfs_read+8 $arg*
> "

Ah, OK. so it should fail but passed. (and test failure)

Thank you,

> 
> Thanks
> Hari
> 
> > 
> > Thank you,
> > 
> >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> >> ---
> >>   .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc    | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> >> index a16c6a6f6055..c03b94cc5784 100644
> >> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> >> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> >> @@ -111,7 +111,11 @@ check_error 'p vfs_read $arg* ^$arg*'		# DOUBLE_ARGS
> >>   if !grep -q 'kernel return probes support:' README; then
> >>   check_error 'r vfs_read ^$arg*'			# NOFENTRY_ARGS
> >>   fi
> >> +if [ "$(uname -m)" = "ppc64le" ]; then
> >> +check_error 'p vfs_read+20 ^$arg*'		# NOFENTRY_ARGS
> >> +else
> >>   check_error 'p vfs_read+8 ^$arg*'		# NOFENTRY_ARGS
> >> +fi
> >>   check_error 'p vfs_read ^hoge'			# NO_BTFARG
> >>   check_error 'p kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
> >>   check_error 'r kfree ^$retval'			# NO_RETVAL
> >> -- 
> >> 2.47.0
> >>
> > 
> > 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>