[PATCH] ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support

[PATCH] ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support

[Jinjie Ruan]

Currently, kprobe on ARM32 can not use the '$argx' syntax available on
other architecture. So implement regs_get_kernel_argument() and add
HAVE_FUNCTION_ARG_ACCESS_API support.

Before there is a following error:
	echo 'p:kprobe_submit_bio submit_bio bio=$arg1' > kprobe_events
	sh: write error: Invalid argument

After:
	# echo 'p:kprobe_submit_bio submit_bio bio=$arg1' > kprobe_events
	# echo  1 > events/kprobes/enable
	# echo  1 > events/kprobes/kprobe_submit_bio/enable
	# echo 0 > tracing_on
	# echo > trace
	# echo 1 > tracing_on
	# cat trace
		kworker/u19:0-36      [002] d....    54.175322: kprobe_submit_bio: (submit_bio+0x0/0xf8) bio=0xc24e6000

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 arch/arm/Kconfig              |  1 +
 arch/arm/include/asm/ptrace.h | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ee5115252aac..4ed504139763 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -102,6 +102,7 @@ config ARM
 	select HAVE_EXIT_THREAD
 	select HAVE_GUP_FAST if ARM_LPAE
 	select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
+	select HAVE_FUNCTION_ARG_ACCESS_API
 	select HAVE_FUNCTION_ERROR_INJECTION
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACER if !XIP_KERNEL
diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 6eb311fb2da0..da5dd4cd0324 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -52,6 +52,28 @@ struct svc_pt_regs {
 #define fast_interrupts_enabled(regs) \
 	(!((regs)->ARM_cpsr & PSR_F_BIT))
 
+/**
+ * regs_get_kernel_argument() - get Nth function argument in kernel
+ * @regs:	pt_regs of that context
+ * @n:		function argument number (start from 0)
+ *
+ * regs_get_argument() returns @n th argument of the function call.
+ *
+ * Note that this chooses the most likely register mapping. In very rare
+ * cases this may not return correct data, for example, if one of the
+ * function parameters is 16 bytes or bigger. In such cases, we cannot
+ * get access the parameter correctly and the register assignment of
+ * subsequent parameters will be shifted.
+ */
+static inline unsigned long regs_get_kernel_argument(struct pt_regs *regs,
+						     unsigned int n)
+{
+#define NR_REG_ARGUMENTS 4
+	if (n < NR_REG_ARGUMENTS)
+		return regs->uregs[n];
+	return 0;
+}
+
 /* Are the current registers suitable for user mode?
  * (used to maintain security in signal handlers)
  */
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support

[Russell King (Oracle)]

On Mon, Jun 03, 2024 at 03:37:51PM +0800, Jinjie Ruan wrote:
> Currently, kprobe on ARM32 can not use the '$argx' syntax available on
> other architecture. So implement regs_get_kernel_argument() and add
> HAVE_FUNCTION_ARG_ACCESS_API support.

This may work in the simple case, but it just doesn't work in the
general case, where a function accepts 64-bit arguments. For example,
for EABI and a function taking a 64-bit argument followed by a 32-bit
argument:

	R0/R1 = 64-bit argument
	R2 = 32-bit argument

Now consider 32-bit argument followed by 64-bit argument:

	R0 = 32-bit argument
	R1 = unused
	R2/R3 = 64-bit argument

Note that the mapping isn't argN = RN.

Also, given that "unsigned long" is 32-bit on 32-bit Arm, one can't
return a 64-bit argument through this interface. Even if one typed
the function as u64, it still wouldn't work because the caller
assigns the return value to an unsigned long. This seems to be an
issue throughout the kernel tracing - it isn't written to support
64-bit arguments on 32-bit architectures. See, for example,
fetch_store_raw(), where the unsigned long gets cast to a u64.
It'll still only have 32-bits of significant value.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support

[Jinjie Ruan]

On 2024/6/3 16:39, Russell King (Oracle) wrote:
> On Mon, Jun 03, 2024 at 03:37:51PM +0800, Jinjie Ruan wrote:
>> Currently, kprobe on ARM32 can not use the '$argx' syntax available on
>> other architecture. So implement regs_get_kernel_argument() and add
>> HAVE_FUNCTION_ARG_ACCESS_API support.
> 
> This may work in the simple case, but it just doesn't work in the
> general case, where a function accepts 64-bit arguments. For example,
> for EABI and a function taking a 64-bit argument followed by a 32-bit
> argument:
> 
> 	R0/R1 = 64-bit argument
> 	R2 = 32-bit argument
> 
> Now consider 32-bit argument followed by 64-bit argument:
> 
> 	R0 = 32-bit argument
> 	R1 = unused
> 	R2/R3 = 64-bit argument

I agree with you, the current implementation considers a very simple
case, where all parameters are 32-bit.

From "Procedure Call Standard for the Arm® Architecture", the
"6.1.1.1 Handling values larger than 32 bits" describes it this way:

A double-word sized type is passed in two consecutive registers (e.g.,
r0 and r1, or r2 and r3).

> 
> Note that the mapping isn't argN = RN.
> 
> Also, given that "unsigned long" is 32-bit on 32-bit Arm, one can't
> return a 64-bit argument through this interface. Even if one typed
> the function as u64, it still wouldn't work because the caller
> assigns the return value to an unsigned long. This seems to be an
> issue throughout the kernel tracing - it isn't written to support

How about updating this interface to solve this problem? Let
regs_get_kernel_argument() return u64.

> 64-bit arguments on 32-bit architectures. See, for example,
> fetch_store_raw(), where the unsigned long gets cast to a u64.
> It'll still only have 32-bits of significant value.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support

[Russell King (Oracle)]

On Tue, Jun 04, 2024 at 09:36:04AM +0800, Jinjie Ruan wrote:
> On 2024/6/3 16:39, Russell King (Oracle) wrote:
> > On Mon, Jun 03, 2024 at 03:37:51PM +0800, Jinjie Ruan wrote:
> >> Currently, kprobe on ARM32 can not use the '$argx' syntax available on
> >> other architecture. So implement regs_get_kernel_argument() and add
> >> HAVE_FUNCTION_ARG_ACCESS_API support.
> > 
> > This may work in the simple case, but it just doesn't work in the
> > general case, where a function accepts 64-bit arguments. For example,
> > for EABI and a function taking a 64-bit argument followed by a 32-bit
> > argument:
> > 
> > 	R0/R1 = 64-bit argument
> > 	R2 = 32-bit argument
> > 
> > Now consider 32-bit argument followed by 64-bit argument:
> > 
> > 	R0 = 32-bit argument
> > 	R1 = unused
> > 	R2/R3 = 64-bit argument
> 
> I agree with you, the current implementation considers a very simple
> case, where all parameters are 32-bit.
> 
> From "Procedure Call Standard for the Arm® Architecture", the
> "6.1.1.1 Handling values larger than 32 bits" describes it this way:
> 
> A double-word sized type is passed in two consecutive registers (e.g.,
> r0 and r1, or r2 and r3).
> 
> > 
> > Note that the mapping isn't argN = RN.
> > 
> > Also, given that "unsigned long" is 32-bit on 32-bit Arm, one can't
> > return a 64-bit argument through this interface. Even if one typed
> > the function as u64, it still wouldn't work because the caller
> > assigns the return value to an unsigned long. This seems to be an
> > issue throughout the kernel tracing - it isn't written to support
> 
> How about updating this interface to solve this problem? Let
> regs_get_kernel_argument() return u64.

That doesn't solve the first problem. The issue is that once we enable
this, it becomes userspace ABI, and any changes to it then become
regressions.

So no, I'm not going to have it enabled in mainline in a half-broken
state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support

[Jinjie Ruan]

On 2024/6/4 16:14, Russell King (Oracle) wrote:
> On Tue, Jun 04, 2024 at 09:36:04AM +0800, Jinjie Ruan wrote:
>> On 2024/6/3 16:39, Russell King (Oracle) wrote:
>>> On Mon, Jun 03, 2024 at 03:37:51PM +0800, Jinjie Ruan wrote:
>>>> Currently, kprobe on ARM32 can not use the '$argx' syntax available on
>>>> other architecture. So implement regs_get_kernel_argument() and add
>>>> HAVE_FUNCTION_ARG_ACCESS_API support.
>>>
>>> This may work in the simple case, but it just doesn't work in the
>>> general case, where a function accepts 64-bit arguments. For example,
>>> for EABI and a function taking a 64-bit argument followed by a 32-bit
>>> argument:
>>>
>>> 	R0/R1 = 64-bit argument
>>> 	R2 = 32-bit argument
>>>
>>> Now consider 32-bit argument followed by 64-bit argument:
>>>
>>> 	R0 = 32-bit argument
>>> 	R1 = unused
>>> 	R2/R3 = 64-bit argument
>>
>> I agree with you, the current implementation considers a very simple
>> case, where all parameters are 32-bit.
>>
>> From "Procedure Call Standard for the Arm® Architecture", the
>> "6.1.1.1 Handling values larger than 32 bits" describes it this way:
>>
>> A double-word sized type is passed in two consecutive registers (e.g.,
>> r0 and r1, or r2 and r3).
>>
>>>
>>> Note that the mapping isn't argN = RN.
>>>
>>> Also, given that "unsigned long" is 32-bit on 32-bit Arm, one can't
>>> return a 64-bit argument through this interface. Even if one typed
>>> the function as u64, it still wouldn't work because the caller
>>> assigns the return value to an unsigned long. This seems to be an
>>> issue throughout the kernel tracing - it isn't written to support
>>
>> How about updating this interface to solve this problem? Let
>> regs_get_kernel_argument() return u64.
> 
> That doesn't solve the first problem. The issue is that once we enable
> this, it becomes userspace ABI, and any changes to it then become
> regressions.

Thank you! I understand what you mean. For the existing "$arg" API
interface, ARM32 cannot determine whether an input parameter of the
instrumentation function is a 32-bit or 64-bit parameter, and there is
no way to control the user not to instrument the function with complex
parameters, so this config cannot be enabled.

> 
> So no, I'm not going to have it enabled in mainline in a half-broken
> state.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel