libbpf: Fix the incorrect register read for syscalls on x86_64

Commit Message

Kenta Tada Dec. 21, 2021, 11:21 a.m. UTC

Currently, rcx is read as the fourth parameter of syscall on x86_64.
But x86_64 Linux System Call convention uses r10 actually.
This commit adds the wrapper for users who want to access to
syscall params to analyze the user space.

Signed-off-by: Kenta Tada <[e-mail]>
---
 tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Yonghong Song Dec. 21, 2021, 3:50 p.m. UTC | #1

On 12/21/21 3:21 AM, [e-mail] wrote:
> Currently, rcx is read as the fourth parameter of syscall on x86_64.
> But x86_64 Linux System Call convention uses r10 actually.
> This commit adds the wrapper for users who want to access to
> syscall params to analyze the user space.
> 
> Signed-off-by: Kenta Tada <[e-mail]>
> ---
>   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index db05a5937105..f6fcccd9b10c 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -67,10 +67,15 @@
>   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
>   
>   #define PT_REGS_PARM1(x) ((x)->di)
> +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
>   #define PT_REGS_PARM2(x) ((x)->si)
> +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
>   #define PT_REGS_PARM3(x) ((x)->dx)
> +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
>   #define PT_REGS_PARM4(x) ((x)->cx)
> +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */

I think this is correct. We have a bcc commit doing similar thing.
https://github.com/iovisor/bcc/commit/c23448e34ecd3cc9bfc19f0b43f4325f77c2e4cc#diff-c78ffb58f59e85eaba9bf9977b7202f3e50f17e2a9ee556c36a311f9a9ab5d6e

>   #define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
>   #define PT_REGS_RET(x) ((x)->sp)
>   #define PT_REGS_FP(x) ((x)->bp)
>   #define PT_REGS_RC(x) ((x)->ax)
> @@ -78,10 +83,15 @@
>   #define PT_REGS_IP(x) ((x)->ip)
>   
>   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
>   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
>   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
>   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* syscall uses r10 */
>   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
>   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
>   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
>   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax)
> @@ -117,10 +127,15 @@
>   #else
>   
>   #define PT_REGS_PARM1(x) ((x)->rdi)
> +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
>   #define PT_REGS_PARM2(x) ((x)->rsi)
> +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
>   #define PT_REGS_PARM3(x) ((x)->rdx)
> +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
>   #define PT_REGS_PARM4(x) ((x)->rcx)
> +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
>   #define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
>   #define PT_REGS_RET(x) ((x)->rsp)
>   #define PT_REGS_FP(x) ((x)->rbp)
>   #define PT_REGS_RC(x) ((x)->rax)
> @@ -128,10 +143,15 @@
>   #define PT_REGS_IP(x) ((x)->rip)
>   
>   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
>   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
>   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
>   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* syscall uses r10 */
>   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
>   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
>   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
>   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)

Looks like macros only available for x86_64. Can we make it also
available for other architectures so we won't introduce arch specific
codes into bpf program?

Also, could you add a selftest to use this macro, esp. for parameter 4?

Andrii Nakryiko Dec. 21, 2021, 10:58 p.m. UTC | #2

On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <[e-mail]> wrote:
>
>
>
> On 12/21/21 3:21 AM, [e-mail] wrote:
> > Currently, rcx is read as the fourth parameter of syscall on x86_64.
> > But x86_64 Linux System Call convention uses r10 actually.
> > This commit adds the wrapper for users who want to access to
> > syscall params to analyze the user space.
> >
> > Signed-off-by: Kenta Tada <[e-mail]>
> > ---
> >   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > index db05a5937105..f6fcccd9b10c 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -67,10 +67,15 @@
> >   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> >
> >   #define PT_REGS_PARM1(x) ((x)->di)
> > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> >   #define PT_REGS_PARM2(x) ((x)->si)
> > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> >   #define PT_REGS_PARM3(x) ((x)->dx)
> > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> >   #define PT_REGS_PARM4(x) ((x)->cx)
> > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
>
> I think this is correct. We have a bcc commit doing similar thing.
> https://github.com/iovisor/bcc/commit/c23448e34ecd3cc9bfc19f0b43f4325f77c2e4cc#diff-c78ffb58f59e85eaba9bf9977b7202f3e50f17e2a9ee556c36a311f9a9ab5d6e
>
> >   #define PT_REGS_PARM5(x) ((x)->r8)
> > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> >   #define PT_REGS_RET(x) ((x)->sp)
> >   #define PT_REGS_FP(x) ((x)->bp)
> >   #define PT_REGS_RC(x) ((x)->ax)
> > @@ -78,10 +83,15 @@
> >   #define PT_REGS_IP(x) ((x)->ip)
> >
> >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* syscall uses r10 */
> >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
> >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
> >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax)
> > @@ -117,10 +127,15 @@
> >   #else
> >
> >   #define PT_REGS_PARM1(x) ((x)->rdi)
> > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> >   #define PT_REGS_PARM2(x) ((x)->rsi)
> > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> >   #define PT_REGS_PARM3(x) ((x)->rdx)
> > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> >   #define PT_REGS_PARM4(x) ((x)->rcx)
> > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
> >   #define PT_REGS_PARM5(x) ((x)->r8)
> > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
> >   #define PT_REGS_RET(x) ((x)->rsp)
> >   #define PT_REGS_FP(x) ((x)->rbp)
> >   #define PT_REGS_RC(x) ((x)->rax)
> > @@ -128,10 +143,15 @@
> >   #define PT_REGS_IP(x) ((x)->rip)
> >
> >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* syscall uses r10 */
> >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
> >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
> >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)
>
> Looks like macros only available for x86_64. Can we make it also
> available for other architectures so we won't introduce arch specific
> codes into bpf program?

Yeah, but instead of copy/pasting it for each architecture, let's
define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only
arch with such inconsistency?) and then after all the architectures
defined their macro define

#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
...
#ifndef PT_REGS_PARM4_SYSCALL(x)
#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
#endif

That way we'll avoid all the extra "no-op" definitions.


>
> Also, could you add a selftest to use this macro, esp. for parameter 4?

+1

Kenta Tada Dec. 22, 2021, 5:51 a.m. UTC | #3

>> Looks like macros only available for x86_64. Can we make it also 
>> available for other architectures so we won't introduce arch specific 
>> codes into bpf program?
>
>Yeah, but instead of copy/pasting it for each architecture, let's define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only arch with such inconsistency?) and then after all the architectures defined their macro define

Currently, I think x86_64 is the only arch which causes this issue.
But I'll add #ifndef to not only the 4th parameter but also all parameters for future extensibility.

>>
>> Also, could you add a selftest to use this macro, esp. for parameter 4?

Sure.
I'll add the same macro to bpf_tracing.h in selftests.

Thanks!

-----Original Message-----
From: Andrii Nakryiko <[e-mail]> 
Sent: Wednesday, December 22, 2021 7:58 AM
To: Yonghong Song <[e-mail]>
Cc: Tada, Kenta (SGC) <[e-mail]>; Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
Subject: Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64

On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <[e-mail]> wrote:
>
>
>
> On 12/21/21 3:21 AM, [e-mail] wrote:
> > Currently, rcx is read as the fourth parameter of syscall on x86_64.
> > But x86_64 Linux System Call convention uses r10 actually.
> > This commit adds the wrapper for users who want to access to syscall 
> > params to analyze the user space.
> >
> > Signed-off-by: Kenta Tada <[e-mail]>
> > ---
> >   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/tools/lib/bpf/bpf_tracing.h 
> > b/tools/lib/bpf/bpf_tracing.h index db05a5937105..f6fcccd9b10c 
> > 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -67,10 +67,15 @@
> >   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> >
> >   #define PT_REGS_PARM1(x) ((x)->di)
> > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> >   #define PT_REGS_PARM2(x) ((x)->si)
> > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> >   #define PT_REGS_PARM3(x) ((x)->dx)
> > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> >   #define PT_REGS_PARM4(x) ((x)->cx)
> > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
>
> I think this is correct. We have a bcc commit doing similar thing.
> https://urldefense.com/v3/__https://github.com/iovisor/bcc/commit/c234
> 48e34ecd3cc9bfc19f0b43f4325f77c2e4cc*diff-c78ffb58f59e85eaba9bf9977b72
> 02f3e50f17e2a9ee556c36a311f9a9ab5d6e__;Iw!!JmoZiZGBv3RvKRSx!qa2pOQlUSd
> WfoxmZ7EuPvZpWAlK5IBVP1eC-2d3njBLe3yehAXyV_0wI9mGRF5Q$ [github[.]com]
>
> >   #define PT_REGS_PARM5(x) ((x)->r8)
> > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> >   #define PT_REGS_RET(x) ((x)->sp)
> >   #define PT_REGS_FP(x) ((x)->bp)
> >   #define PT_REGS_RC(x) ((x)->ax)
> > @@ -78,10 +83,15 @@
> >   #define PT_REGS_IP(x) ((x)->ip)
> >
> >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* 
> > +syscall uses r10 */
> >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
> >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
> >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax) @@ -117,10 
> > +127,15 @@
> >   #else
> >
> >   #define PT_REGS_PARM1(x) ((x)->rdi)
> > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> >   #define PT_REGS_PARM2(x) ((x)->rsi)
> > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> >   #define PT_REGS_PARM3(x) ((x)->rdx)
> > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> >   #define PT_REGS_PARM4(x) ((x)->rcx)
> > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
> >   #define PT_REGS_PARM5(x) ((x)->r8)
> > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
> >   #define PT_REGS_RET(x) ((x)->rsp)
> >   #define PT_REGS_FP(x) ((x)->rbp)
> >   #define PT_REGS_RC(x) ((x)->rax)
> > @@ -128,10 +143,15 @@
> >   #define PT_REGS_IP(x) ((x)->rip)
> >
> >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* 
> > +syscall uses r10 */
> >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
> >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
> >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)
>
> Looks like macros only available for x86_64. Can we make it also 
> available for other architectures so we won't introduce arch specific 
> codes into bpf program?

Yeah, but instead of copy/pasting it for each architecture, let's define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only arch with such inconsistency?) and then after all the architectures defined their macro define

#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) ...
#ifndef PT_REGS_PARM4_SYSCALL(x)
#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x) #endif

That way we'll avoid all the extra "no-op" definitions.


>
> Also, could you add a selftest to use this macro, esp. for parameter 4?

+1

Kenta Tada Dec. 22, 2021, 6:19 a.m. UTC | #4

>>
>> Also, could you add a selftest to use this macro, esp. for parameter 4?

I misunderstood this comment.
I'll just add a test of this new macro.

-----Original Message-----
From: Tada, Kenta (SGC) 
Sent: Wednesday, December 22, 2021 2:52 PM
To: Andrii Nakryiko <[e-mail]>; Yonghong Song <[e-mail]>
Cc: Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
Subject: RE: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64

>> Looks like macros only available for x86_64. Can we make it also 
>> available for other architectures so we won't introduce arch specific 
>> codes into bpf program?
>
>Yeah, but instead of copy/pasting it for each architecture, let's 
>define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only 
>arch with such inconsistency?) and then after all the architectures 
>defined their macro define

Currently, I think x86_64 is the only arch which causes this issue.
But I'll add #ifndef to not only the 4th parameter but also all parameters for future extensibility.

>>
>> Also, could you add a selftest to use this macro, esp. for parameter 4?

Sure.
I'll add the same macro to bpf_tracing.h in selftests.

Thanks!

-----Original Message-----
From: Andrii Nakryiko <[e-mail]>
Sent: Wednesday, December 22, 2021 7:58 AM
To: Yonghong Song <[e-mail]>
Cc: Tada, Kenta (SGC) <[e-mail]>; Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
Subject: Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64

On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <[e-mail]> wrote:
>
>
>
> On 12/21/21 3:21 AM, [e-mail] wrote:
> > Currently, rcx is read as the fourth parameter of syscall on x86_64.
> > But x86_64 Linux System Call convention uses r10 actually.
> > This commit adds the wrapper for users who want to access to syscall 
> > params to analyze the user space.
> >
> > Signed-off-by: Kenta Tada <[e-mail]>
> > ---
> >   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/tools/lib/bpf/bpf_tracing.h 
> > b/tools/lib/bpf/bpf_tracing.h index db05a5937105..f6fcccd9b10c
> > 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -67,10 +67,15 @@
> >   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> >
> >   #define PT_REGS_PARM1(x) ((x)->di)
> > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> >   #define PT_REGS_PARM2(x) ((x)->si)
> > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> >   #define PT_REGS_PARM3(x) ((x)->dx)
> > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> >   #define PT_REGS_PARM4(x) ((x)->cx)
> > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
>
> I think this is correct. We have a bcc commit doing similar thing.
> https://urldefense.com/v3/__https://github.com/iovisor/bcc/commit/c234
> 48e34ecd3cc9bfc19f0b43f4325f77c2e4cc*diff-c78ffb58f59e85eaba9bf9977b72
> 02f3e50f17e2a9ee556c36a311f9a9ab5d6e__;Iw!!JmoZiZGBv3RvKRSx!qa2pOQlUSd
> WfoxmZ7EuPvZpWAlK5IBVP1eC-2d3njBLe3yehAXyV_0wI9mGRF5Q$ [github[.]com]
>
> >   #define PT_REGS_PARM5(x) ((x)->r8)
> > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> >   #define PT_REGS_RET(x) ((x)->sp)
> >   #define PT_REGS_FP(x) ((x)->bp)
> >   #define PT_REGS_RC(x) ((x)->ax)
> > @@ -78,10 +83,15 @@
> >   #define PT_REGS_IP(x) ((x)->ip)
> >
> >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* 
> > +syscall uses r10 */
> >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
> >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
> >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax) @@ -117,10
> > +127,15 @@
> >   #else
> >
> >   #define PT_REGS_PARM1(x) ((x)->rdi)
> > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> >   #define PT_REGS_PARM2(x) ((x)->rsi)
> > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> >   #define PT_REGS_PARM3(x) ((x)->rdx)
> > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> >   #define PT_REGS_PARM4(x) ((x)->rcx)
> > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
> >   #define PT_REGS_PARM5(x) ((x)->r8)
> > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
> >   #define PT_REGS_RET(x) ((x)->rsp)
> >   #define PT_REGS_FP(x) ((x)->rbp)
> >   #define PT_REGS_RC(x) ((x)->rax)
> > @@ -128,10 +143,15 @@
> >   #define PT_REGS_IP(x) ((x)->rip)
> >
> >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* 
> > +syscall uses r10 */
> >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
> >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
> >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)
>
> Looks like macros only available for x86_64. Can we make it also 
> available for other architectures so we won't introduce arch specific 
> codes into bpf program?

Yeah, but instead of copy/pasting it for each architecture, let's define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only arch with such inconsistency?) and then after all the architectures defined their macro define

#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) ...
#ifndef PT_REGS_PARM4_SYSCALL(x)
#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x) #endif

That way we'll avoid all the extra "no-op" definitions.


>
> Also, could you add a selftest to use this macro, esp. for parameter 4?

+1

Andrii Nakryiko Dec. 22, 2021, 6:47 a.m. UTC | #5

On Tue, Dec 21, 2021 at 10:20 PM <[e-mail]> wrote:
>
> >>
> >> Also, could you add a selftest to use this macro, esp. for parameter 4?
>
> I misunderstood this comment.
> I'll just add a test of this new macro.
>
> -----Original Message-----
> From: Tada, Kenta (SGC)
> Sent: Wednesday, December 22, 2021 2:52 PM
> To: Andrii Nakryiko <[e-mail]>; Yonghong Song <[e-mail]>
> Cc: Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
> Subject: RE: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64
>
> >> Looks like macros only available for x86_64. Can we make it also
> >> available for other architectures so we won't introduce arch specific
> >> codes into bpf program?
> >
> >Yeah, but instead of copy/pasting it for each architecture, let's
> >define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only
> >arch with such inconsistency?) and then after all the architectures
> >defined their macro define
>
> Currently, I think x86_64 is the only arch which causes this issue.
> But I'll add #ifndef to not only the 4th parameter but also all parameters for future extensibility.

Let's not add unnecessary #ifndefs. If we get a case for other args,
we'll add #ifndefs as necessary.

But after looking at your and Hengqi's patches in the last few days,
I've looked at the current state of bpf_tracing.h and felt like there
is too much repetition. So I've refactored it to not require as many
similar macro definitions. I'm going to finish it up and submit it
tomorrow, so please hold on a bit with your additions until then so
that you can base it off the refactored bpf_tracing.h. Thanks.

>
> >>
> >> Also, could you add a selftest to use this macro, esp. for parameter 4?
>
> Sure.
> I'll add the same macro to bpf_tracing.h in selftests.
>
> Thanks!
>
> -----Original Message-----
> From: Andrii Nakryiko <[e-mail]>
> Sent: Wednesday, December 22, 2021 7:58 AM
> To: Yonghong Song <[e-mail]>
> Cc: Tada, Kenta (SGC) <[e-mail]>; Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
> Subject: Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64
>
> On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <[e-mail]> wrote:
> >
> >
> >
> > On 12/21/21 3:21 AM, [e-mail] wrote:
> > > Currently, rcx is read as the fourth parameter of syscall on x86_64.
> > > But x86_64 Linux System Call convention uses r10 actually.
> > > This commit adds the wrapper for users who want to access to syscall
> > > params to analyze the user space.
> > >
> > > Signed-off-by: Kenta Tada <[e-mail]>
> > > ---
> > >   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/bpf_tracing.h
> > > b/tools/lib/bpf/bpf_tracing.h index db05a5937105..f6fcccd9b10c
> > > 100644
> > > --- a/tools/lib/bpf/bpf_tracing.h
> > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > @@ -67,10 +67,15 @@
> > >   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > >
> > >   #define PT_REGS_PARM1(x) ((x)->di)
> > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > >   #define PT_REGS_PARM2(x) ((x)->si)
> > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > >   #define PT_REGS_PARM3(x) ((x)->dx)
> > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > >   #define PT_REGS_PARM4(x) ((x)->cx)
> > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
> >
> > I think this is correct. We have a bcc commit doing similar thing.
> > https://urldefense.com/v3/__https://github.com/iovisor/bcc/commit/c234
> > 48e34ecd3cc9bfc19f0b43f4325f77c2e4cc*diff-c78ffb58f59e85eaba9bf9977b72
> > 02f3e50f17e2a9ee556c36a311f9a9ab5d6e__;Iw!!JmoZiZGBv3RvKRSx!qa2pOQlUSd
> > WfoxmZ7EuPvZpWAlK5IBVP1eC-2d3njBLe3yehAXyV_0wI9mGRF5Q$ [github[.]com]
> >
> > >   #define PT_REGS_PARM5(x) ((x)->r8)
> > > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> > >   #define PT_REGS_RET(x) ((x)->sp)
> > >   #define PT_REGS_FP(x) ((x)->bp)
> > >   #define PT_REGS_RC(x) ((x)->ax)
> > > @@ -78,10 +83,15 @@
> > >   #define PT_REGS_IP(x) ((x)->ip)
> > >
> > >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /*
> > > +syscall uses r10 */
> > >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
> > >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
> > >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax) @@ -117,10
> > > +127,15 @@
> > >   #else
> > >
> > >   #define PT_REGS_PARM1(x) ((x)->rdi)
> > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > >   #define PT_REGS_PARM2(x) ((x)->rsi)
> > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > >   #define PT_REGS_PARM3(x) ((x)->rdx)
> > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > >   #define PT_REGS_PARM4(x) ((x)->rcx)
> > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
> > >   #define PT_REGS_PARM5(x) ((x)->r8)
> > > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
> > >   #define PT_REGS_RET(x) ((x)->rsp)
> > >   #define PT_REGS_FP(x) ((x)->rbp)
> > >   #define PT_REGS_RC(x) ((x)->rax)
> > > @@ -128,10 +143,15 @@
> > >   #define PT_REGS_IP(x) ((x)->rip)
> > >
> > >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /*
> > > +syscall uses r10 */
> > >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
> > >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
> > >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)
> >
> > Looks like macros only available for x86_64. Can we make it also
> > available for other architectures so we won't introduce arch specific
> > codes into bpf program?
>
> Yeah, but instead of copy/pasting it for each architecture, let's define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only arch with such inconsistency?) and then after all the architectures defined their macro define
>
> #define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) ...
> #ifndef PT_REGS_PARM4_SYSCALL(x)
> #define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x) #endif
>
> That way we'll avoid all the extra "no-op" definitions.
>
>
> >
> > Also, could you add a selftest to use this macro, esp. for parameter 4?
>
> +1

Kenta Tada Dec. 22, 2021, 6:57 a.m. UTC | #6

>Let's not add unnecessary #ifndefs. If we get a case for other args, we'll add #ifndefs as necessary.

OK. I agree with you.

>But after looking at your and Hengqi's patches in the last few days, I've looked at the current state of bpf_tracing.h and felt like there is too much repetition. So I've refactored it to not require as many similar macro definitions. I'm going to finish it up and submit it tomorrow, so please hold on a bit with your additions until then so that you can base it off the refactored bpf_tracing.h. Thanks.

I'll wait for the refactored bpf_tracing.h
Thanks for all your help.

-----Original Message-----
From: Andrii Nakryiko <[e-mail]> 
Sent: Wednesday, December 22, 2021 3:47 PM
To: Tada, Kenta (SGC) <[e-mail]>
Cc: Yonghong Song <[e-mail]>; Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
Subject: Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64

On Tue, Dec 21, 2021 at 10:20 PM <[e-mail]> wrote:
>
> >>
> >> Also, could you add a selftest to use this macro, esp. for parameter 4?
>
> I misunderstood this comment.
> I'll just add a test of this new macro.
>
> -----Original Message-----
> From: Tada, Kenta (SGC)
> Sent: Wednesday, December 22, 2021 2:52 PM
> To: Andrii Nakryiko <[e-mail]>; Yonghong Song 
> <[e-mail]>
> Cc: Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; 
> Alexei Starovoitov <[e-mail]>; Daniel Borkmann 
> <[e-mail]>; Martin Lau <[e-mail]>; Song Liu 
> <[e-mail]>; john fastabend <[e-mail]>; KP 
> Singh <[e-mail]>
> Subject: RE: [PATCH] libbpf: Fix the incorrect register read for 
> syscalls on x86_64
>
> >> Looks like macros only available for x86_64. Can we make it also 
> >> available for other architectures so we won't introduce arch 
> >> specific codes into bpf program?
> >
> >Yeah, but instead of copy/pasting it for each architecture, let's 
> >define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only 
> >arch with such inconsistency?) and then after all the architectures 
> >defined their macro define
>
> Currently, I think x86_64 is the only arch which causes this issue.
> But I'll add #ifndef to not only the 4th parameter but also all parameters for future extensibility.

Let's not add unnecessary #ifndefs. If we get a case for other args, we'll add #ifndefs as necessary.

But after looking at your and Hengqi's patches in the last few days, I've looked at the current state of bpf_tracing.h and felt like there is too much repetition. So I've refactored it to not require as many similar macro definitions. I'm going to finish it up and submit it tomorrow, so please hold on a bit with your additions until then so that you can base it off the refactored bpf_tracing.h. Thanks.

>
> >>
> >> Also, could you add a selftest to use this macro, esp. for parameter 4?
>
> Sure.
> I'll add the same macro to bpf_tracing.h in selftests.
>
> Thanks!
>
> -----Original Message-----
> From: Andrii Nakryiko <[e-mail]>
> Sent: Wednesday, December 22, 2021 7:58 AM
> To: Yonghong Song <[e-mail]>
> Cc: Tada, Kenta (SGC) <[e-mail]>; Andrii Nakryiko 
> <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov 
> <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau 
> <[e-mail]>; Song Liu <[e-mail]>; john fastabend 
> <[e-mail]>; KP Singh <[e-mail]>
> Subject: Re: [PATCH] libbpf: Fix the incorrect register read for 
> syscalls on x86_64
>
> On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <[e-mail]> wrote:
> >
> >
> >
> > On 12/21/21 3:21 AM, [e-mail] wrote:
> > > Currently, rcx is read as the fourth parameter of syscall on x86_64.
> > > But x86_64 Linux System Call convention uses r10 actually.
> > > This commit adds the wrapper for users who want to access to 
> > > syscall params to analyze the user space.
> > >
> > > Signed-off-by: Kenta Tada <[e-mail]>
> > > ---
> > >   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/bpf_tracing.h 
> > > b/tools/lib/bpf/bpf_tracing.h index db05a5937105..f6fcccd9b10c
> > > 100644
> > > --- a/tools/lib/bpf/bpf_tracing.h
> > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > @@ -67,10 +67,15 @@
> > >   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > >
> > >   #define PT_REGS_PARM1(x) ((x)->di)
> > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > >   #define PT_REGS_PARM2(x) ((x)->si)
> > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > >   #define PT_REGS_PARM3(x) ((x)->dx)
> > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > >   #define PT_REGS_PARM4(x) ((x)->cx)
> > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 
> > > +*/
> >
> > I think this is correct. We have a bcc commit doing similar thing.
> > https://urldefense.com/v3/__https://github.com/iovisor/bcc/commit/c2
> > 34
> > 48e34ecd3cc9bfc19f0b43f4325f77c2e4cc*diff-c78ffb58f59e85eaba9bf9977b
> > 72 
> > 02f3e50f17e2a9ee556c36a311f9a9ab5d6e__;Iw!!JmoZiZGBv3RvKRSx!qa2pOQlU
> > Sd WfoxmZ7EuPvZpWAlK5IBVP1eC-2d3njBLe3yehAXyV_0wI9mGRF5Q$ 
> > [github[.]com]
> >
> > >   #define PT_REGS_PARM5(x) ((x)->r8)
> > > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> > >   #define PT_REGS_RET(x) ((x)->sp)
> > >   #define PT_REGS_FP(x) ((x)->bp)
> > >   #define PT_REGS_RC(x) ((x)->ax)
> > > @@ -78,10 +83,15 @@
> > >   #define PT_REGS_IP(x) ((x)->ip)
> > >
> > >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* 
> > > +syscall uses r10 */
> > >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
> > >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
> > >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax) @@ -117,10
> > > +127,15 @@
> > >   #else
> > >
> > >   #define PT_REGS_PARM1(x) ((x)->rdi)
> > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > >   #define PT_REGS_PARM2(x) ((x)->rsi)
> > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > >   #define PT_REGS_PARM3(x) ((x)->rdx)
> > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > >   #define PT_REGS_PARM4(x) ((x)->rcx)
> > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 
> > > +*/
> > >   #define PT_REGS_PARM5(x) ((x)->r8)
> > > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
> > >   #define PT_REGS_RET(x) ((x)->rsp)
> > >   #define PT_REGS_FP(x) ((x)->rbp)
> > >   #define PT_REGS_RC(x) ((x)->rax) @@ -128,10 +143,15 @@
> > >   #define PT_REGS_IP(x) ((x)->rip)
> > >
> > >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* 
> > > +syscall uses r10 */
> > >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
> > >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
> > >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)
> >
> > Looks like macros only available for x86_64. Can we make it also 
> > available for other architectures so we won't introduce arch 
> > specific codes into bpf program?
>
> Yeah, but instead of copy/pasting it for each architecture, let's 
> define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only 
> arch with such inconsistency?) and then after all the architectures 
> defined their macro define
>
> #define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) ...
> #ifndef PT_REGS_PARM4_SYSCALL(x)
> #define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x) #endif
>
> That way we'll avoid all the extra "no-op" definitions.
>
>
> >
> > Also, could you add a selftest to use this macro, esp. for parameter 4?
>
> +1

Andrii Nakryiko Jan. 6, 2022, 8:29 p.m. UTC | #7

On Tue, Dec 21, 2021 at 10:58 PM <[e-mail]> wrote:
>
> >Let's not add unnecessary #ifndefs. If we get a case for other args, we'll add #ifndefs as necessary.
>
> OK. I agree with you.
>
> >But after looking at your and Hengqi's patches in the last few days, I've looked at the current state of bpf_tracing.h and felt like there is too much repetition. So I've refactored it to not require as many similar macro definitions. I'm going to finish it up and submit it tomorrow, so please hold on a bit with your additions until then so that you can base it off the refactored bpf_tracing.h. Thanks.
>
> I'll wait for the refactored bpf_tracing.h
> Thanks for all your help.

The patches got merged. Please submit your fixes based on top of those.

And please don't top post, reply inline.

>
> -----Original Message-----
> From: Andrii Nakryiko <[e-mail]>
> Sent: Wednesday, December 22, 2021 3:47 PM
> To: Tada, Kenta (SGC) <[e-mail]>
> Cc: Yonghong Song <[e-mail]>; Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau <[e-mail]>; Song Liu <[e-mail]>; john fastabend <[e-mail]>; KP Singh <[e-mail]>
> Subject: Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64
>
> On Tue, Dec 21, 2021 at 10:20 PM <[e-mail]> wrote:
> >
> > >>
> > >> Also, could you add a selftest to use this macro, esp. for parameter 4?
> >
> > I misunderstood this comment.
> > I'll just add a test of this new macro.
> >
> > -----Original Message-----
> > From: Tada, Kenta (SGC)
> > Sent: Wednesday, December 22, 2021 2:52 PM
> > To: Andrii Nakryiko <[e-mail]>; Yonghong Song
> > <[e-mail]>
> > Cc: Andrii Nakryiko <[e-mail]>; bpf <[e-mail]>;
> > Alexei Starovoitov <[e-mail]>; Daniel Borkmann
> > <[e-mail]>; Martin Lau <[e-mail]>; Song Liu
> > <[e-mail]>; john fastabend <[e-mail]>; KP
> > Singh <[e-mail]>
> > Subject: RE: [PATCH] libbpf: Fix the incorrect register read for
> > syscalls on x86_64
> >
> > >> Looks like macros only available for x86_64. Can we make it also
> > >> available for other architectures so we won't introduce arch
> > >> specific codes into bpf program?
> > >
> > >Yeah, but instead of copy/pasting it for each architecture, let's
> > >define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only
> > >arch with such inconsistency?) and then after all the architectures
> > >defined their macro define
> >
> > Currently, I think x86_64 is the only arch which causes this issue.
> > But I'll add #ifndef to not only the 4th parameter but also all parameters for future extensibility.
>
> Let's not add unnecessary #ifndefs. If we get a case for other args, we'll add #ifndefs as necessary.
>
> But after looking at your and Hengqi's patches in the last few days, I've looked at the current state of bpf_tracing.h and felt like there is too much repetition. So I've refactored it to not require as many similar macro definitions. I'm going to finish it up and submit it tomorrow, so please hold on a bit with your additions until then so that you can base it off the refactored bpf_tracing.h. Thanks.
>
> >
> > >>
> > >> Also, could you add a selftest to use this macro, esp. for parameter 4?
> >
> > Sure.
> > I'll add the same macro to bpf_tracing.h in selftests.
> >
> > Thanks!
> >
> > -----Original Message-----
> > From: Andrii Nakryiko <[e-mail]>
> > Sent: Wednesday, December 22, 2021 7:58 AM
> > To: Yonghong Song <[e-mail]>
> > Cc: Tada, Kenta (SGC) <[e-mail]>; Andrii Nakryiko
> > <[e-mail]>; bpf <[e-mail]>; Alexei Starovoitov
> > <[e-mail]>; Daniel Borkmann <[e-mail]>; Martin Lau
> > <[e-mail]>; Song Liu <[e-mail]>; john fastabend
> > <[e-mail]>; KP Singh <[e-mail]>
> > Subject: Re: [PATCH] libbpf: Fix the incorrect register read for
> > syscalls on x86_64
> >
> > On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <[e-mail]> wrote:
> > >
> > >
> > >
> > > On 12/21/21 3:21 AM, [e-mail] wrote:
> > > > Currently, rcx is read as the fourth parameter of syscall on x86_64.
> > > > But x86_64 Linux System Call convention uses r10 actually.
> > > > This commit adds the wrapper for users who want to access to
> > > > syscall params to analyze the user space.
> > > >
> > > > Signed-off-by: Kenta Tada <[e-mail]>
> > > > ---
> > > >   tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++
> > > >   1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/tools/lib/bpf/bpf_tracing.h
> > > > b/tools/lib/bpf/bpf_tracing.h index db05a5937105..f6fcccd9b10c
> > > > 100644
> > > > --- a/tools/lib/bpf/bpf_tracing.h
> > > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > > @@ -67,10 +67,15 @@
> > > >   #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > > >
> > > >   #define PT_REGS_PARM1(x) ((x)->di)
> > > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > > >   #define PT_REGS_PARM2(x) ((x)->si)
> > > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > > >   #define PT_REGS_PARM3(x) ((x)->dx)
> > > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > > >   #define PT_REGS_PARM4(x) ((x)->cx)
> > > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10
> > > > +*/
> > >
> > > I think this is correct. We have a bcc commit doing similar thing.
> > > https://urldefense.com/v3/__https://github.com/iovisor/bcc/commit/c2
> > > 34
> > > 48e34ecd3cc9bfc19f0b43f4325f77c2e4cc*diff-c78ffb58f59e85eaba9bf9977b
> > > 72
> > > 02f3e50f17e2a9ee556c36a311f9a9ab5d6e__;Iw!!JmoZiZGBv3RvKRSx!qa2pOQlU
> > > Sd WfoxmZ7EuPvZpWAlK5IBVP1eC-2d3njBLe3yehAXyV_0wI9mGRF5Q$
> > > [github[.]com]
> > >
> > > >   #define PT_REGS_PARM5(x) ((x)->r8)
> > > > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> > > >   #define PT_REGS_RET(x) ((x)->sp)
> > > >   #define PT_REGS_FP(x) ((x)->bp)
> > > >   #define PT_REGS_RC(x) ((x)->ax)
> > > > @@ -78,10 +83,15 @@
> > > >   #define PT_REGS_IP(x) ((x)->ip)
> > > >
> > > >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
> > > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > > >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
> > > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > > >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
> > > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > > >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
> > > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /*
> > > > +syscall uses r10 */
> > > >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > > >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
> > > >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
> > > >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax) @@ -117,10
> > > > +127,15 @@
> > > >   #else
> > > >
> > > >   #define PT_REGS_PARM1(x) ((x)->rdi)
> > > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > > >   #define PT_REGS_PARM2(x) ((x)->rsi)
> > > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > > >   #define PT_REGS_PARM3(x) ((x)->rdx)
> > > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > > >   #define PT_REGS_PARM4(x) ((x)->rcx)
> > > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10
> > > > +*/
> > > >   #define PT_REGS_PARM5(x) ((x)->r8)
> > > > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
> > > >   #define PT_REGS_RET(x) ((x)->rsp)
> > > >   #define PT_REGS_FP(x) ((x)->rbp)
> > > >   #define PT_REGS_RC(x) ((x)->rax) @@ -128,10 +143,15 @@
> > > >   #define PT_REGS_IP(x) ((x)->rip)
> > > >
> > > >   #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
> > > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > > >   #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
> > > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > > >   #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
> > > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > > >   #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
> > > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /*
> > > > +syscall uses r10 */
> > > >   #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
> > > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > > >   #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
> > > >   #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
> > > >   #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)
> > >
> > > Looks like macros only available for x86_64. Can we make it also
> > > available for other architectures so we won't introduce arch
> > > specific codes into bpf program?
> >
> > Yeah, but instead of copy/pasting it for each architecture, let's
> > define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only
> > arch with such inconsistency?) and then after all the architectures
> > defined their macro define
> >
> > #define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) ...
> > #ifndef PT_REGS_PARM4_SYSCALL(x)
> > #define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x) #endif
> >
> > That way we'll avoid all the extra "no-op" definitions.
> >
> >
> > >
> > > Also, could you add a selftest to use this macro, esp. for parameter 4?
> >
> > +1

Patch

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index db05a5937105..f6fcccd9b10c 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -67,10 +67,15 @@ 
 #if defined(__KERNEL__) || defined(__VMLINUX_H__)
 
 #define PT_REGS_PARM1(x) ((x)->di)
+#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
 #define PT_REGS_PARM2(x) ((x)->si)
+#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
 #define PT_REGS_PARM3(x) ((x)->dx)
+#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
 #define PT_REGS_PARM4(x) ((x)->cx)
+#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
 #define PT_REGS_PARM5(x) ((x)->r8)
+#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
 #define PT_REGS_RET(x) ((x)->sp)
 #define PT_REGS_FP(x) ((x)->bp)
 #define PT_REGS_RC(x) ((x)->ax)
@@ -78,10 +83,15 @@ 
 #define PT_REGS_IP(x) ((x)->ip)
 
 #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di)
+#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
 #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si)
+#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
 #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx)
+#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
 #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx)
+#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* syscall uses r10 */
 #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
+#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
 #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp)
 #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp)
 #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax)
@@ -117,10 +127,15 @@ 
 #else
 
 #define PT_REGS_PARM1(x) ((x)->rdi)
+#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
 #define PT_REGS_PARM2(x) ((x)->rsi)
+#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
 #define PT_REGS_PARM3(x) ((x)->rdx)
+#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
 #define PT_REGS_PARM4(x) ((x)->rcx)
+#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */
 #define PT_REGS_PARM5(x) ((x)->r8)
+#define PT_REGS_PARM5(x) PT_REGS_PARM5(x)
 #define PT_REGS_RET(x) ((x)->rsp)
 #define PT_REGS_FP(x) ((x)->rbp)
 #define PT_REGS_RC(x) ((x)->rax)
@@ -128,10 +143,15 @@ 
 #define PT_REGS_IP(x) ((x)->rip)
 
 #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi)
+#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
 #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi)
+#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
 #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx)
+#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
 #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx)
+#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* syscall uses r10 */
 #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8)
+#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
 #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp)
 #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp)
 #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax)