[bpf-next,1/2] libbpf: Fix accessing first syscall argument on RV64

Commit Message

Pu Lehui Aug. 29, 2024, 1:34 p.m. UTC

From: Pu Lehui <[e-mail]>

On RV64, as Ilya mentioned before [0], the first syscall parameter should be
accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
test_lsm, etc. to fail on RV64. Let's fix it by using the struct pt_regs
style to provide access to it only through PT_REGS_PARM1_CORE_SYSCALL().

Link: https://lore.kernel.org/bpf/[e-mail] [0]
Signed-off-by: Pu Lehui <[e-mail]>
---
 tools/lib/bpf/bpf_tracing.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko Aug. 30, 2024, 7:34 p.m. UTC | #1

On Thu, Aug 29, 2024 at 6:32 AM Pu Lehui <[e-mail]> wrote:
>
> From: Pu Lehui <[e-mail]>
>
> On RV64, as Ilya mentioned before [0], the first syscall parameter should be
> accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
> otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
> test_lsm, etc. to fail on RV64. Let's fix it by using the struct pt_regs
> style to provide access to it only through PT_REGS_PARM1_CORE_SYSCALL().
>
> Link: https://lore.kernel.org/bpf/[e-mail] [0]
> Signed-off-by: Pu Lehui <[e-mail]>
> ---
>  tools/lib/bpf/bpf_tracing.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 9314fa95f04e..388f30cf7914 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -351,6 +351,10 @@ struct pt_regs___arm64 {
>   * https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#risc-v-calling-conventions
>   */
>
> +struct pt_regs___riscv {
> +       unsigned long orig_a0;
> +};
> +
>  /* riscv provides struct user_regs_struct instead of struct pt_regs to userspace */
>  #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
>  #define __PT_PARM1_REG a0
> @@ -362,12 +366,15 @@ struct pt_regs___arm64 {
>  #define __PT_PARM7_REG a6
>  #define __PT_PARM8_REG a7
>
> -#define __PT_PARM1_SYSCALL_REG __PT_PARM1_REG
> +#define __PT_PARM1_SYSCALL_REG orig_a0
>  #define __PT_PARM2_SYSCALL_REG __PT_PARM2_REG
>  #define __PT_PARM3_SYSCALL_REG __PT_PARM3_REG
>  #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
>  #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
>  #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
> +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
> +#define PT_REGS_PARM1_CORE_SYSCALL(x) \
> +       BPF_CORE_READ((const struct pt_regs___riscv *)(x), __PT_PARM1_SYSCALL_REG)

I feel like what we did for s390x is a bit suboptimal, so let's (try
to) improve that and then do the same for RV64.

What I mean is that PT_REGS_PARMn_SYSCALL macros are used to read
pt_regs coming directly from context, right? In that case we don't
need to pay the price of BPF_CORE_READ(), we can just access memory
directly (but we still need CO-RE relocation!).

So I think what we should do is

1) mark pt_regs___riscv {} with __attribute__((preserve_access_index))
so that normal field accesses are CO-RE-relocated
2) change PT_REGS_PARM1_SYSCALL(x) to be `((const
struct_pt_regs___riscv *)(x))->orig_a0`, which will directly read
memory
3) keep PT_REGS_PARM1_CORE_SYSCALL() as is


But having written all the above, I'm not sure whether we allow CO-RE
relocated direct context accesses (verifier might complain about
modifying ctx register offset or something). So can you please check
it either on s390 or RV64 and let me know? I'm not marking it as
"Changes Requested" for that reason, because that might not work and
we'll have to do BPF_CORE_READ().

>
>  #define __PT_RET_REG ra
>  #define __PT_FP_REG s0
> --
> 2.34.1
>

Pu Lehui Aug. 31, 2024, 6:16 a.m. UTC | #2

On 2024/8/31 3:34, Andrii Nakryiko wrote:
> On Thu, Aug 29, 2024 at 6:32 AM Pu Lehui <[e-mail]> wrote:
>>
[SNIP]
>>
>> -#define __PT_PARM1_SYSCALL_REG __PT_PARM1_REG
>> +#define __PT_PARM1_SYSCALL_REG orig_a0
>>   #define __PT_PARM2_SYSCALL_REG __PT_PARM2_REG
>>   #define __PT_PARM3_SYSCALL_REG __PT_PARM3_REG
>>   #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
>>   #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
>>   #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
>> +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
>> +#define PT_REGS_PARM1_CORE_SYSCALL(x) \
>> +       BPF_CORE_READ((const struct pt_regs___riscv *)(x), __PT_PARM1_SYSCALL_REG)
> 
> I feel like what we did for s390x is a bit suboptimal, so let's (try
> to) improve that and then do the same for RV64.
> 
> What I mean is that PT_REGS_PARMn_SYSCALL macros are used to read
> pt_regs coming directly from context, right? In that case we don't
> need to pay the price of BPF_CORE_READ(), we can just access memory
> directly (but we still need CO-RE relocation!).
> 
> So I think what we should do is
> 
> 1) mark pt_regs___riscv {} with __attribute__((preserve_access_index))
> so that normal field accesses are CO-RE-relocated
> 2) change PT_REGS_PARM1_SYSCALL(x) to be `((const
> struct_pt_regs___riscv *)(x))->orig_a0`, which will directly read
> memory
> 3) keep PT_REGS_PARM1_CORE_SYSCALL() as is
> 
> 
> But having written all the above, I'm not sure whether we allow CO-RE
> relocated direct context accesses (verifier might complain about
> modifying ctx register offset or something). So can you please check
> it either on s390 or RV64 and let me know? I'm not marking it as
> "Changes Requested" for that reason, because that might not work and
> we'll have to do BPF_CORE_READ().

Hi Andrii, thanks for your suggestion, it's really cool. I check that 
work for RV64, and send a new version:

https://lore.kernel.org/bpf/[e-mail]

> 
>>
>>   #define __PT_RET_REG ra
>>   #define __PT_FP_REG s0
>> --
>> 2.34.1
>>

Patch

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 9314fa95f04e..388f30cf7914 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -351,6 +351,10 @@  struct pt_regs___arm64 {
  * https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#risc-v-calling-conventions
  */
 
+struct pt_regs___riscv {
+	unsigned long orig_a0;
+};
+
 /* riscv provides struct user_regs_struct instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
 #define __PT_PARM1_REG a0
@@ -362,12 +366,15 @@  struct pt_regs___arm64 {
 #define __PT_PARM7_REG a6
 #define __PT_PARM8_REG a7
 
-#define __PT_PARM1_SYSCALL_REG __PT_PARM1_REG
+#define __PT_PARM1_SYSCALL_REG orig_a0
 #define __PT_PARM2_SYSCALL_REG __PT_PARM2_REG
 #define __PT_PARM3_SYSCALL_REG __PT_PARM3_REG
 #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
 #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
 #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
+#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_CORE_SYSCALL(x) \
+	BPF_CORE_READ((const struct pt_regs___riscv *)(x), __PT_PARM1_SYSCALL_REG)
 
 #define __PT_RET_REG ra
 #define __PT_FP_REG s0