Message ID | 20240603073751.2282024-1-ruanjinjie@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ARM: Add HAVE_FUNCTION_ARG_ACCESS_API support | expand |
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.
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. >
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.
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. >
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) */
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(+)