| Message ID | 20220726171129.708371-1-yhs@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: Support struct value argument for trampoline base progs | expand |
On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote: > Currently struct arguments are not supported for trampoline based > progs. > One of major reason is that struct argument may pass by value which > may > use more than one registers. This breaks trampoline progs where > each argument is assumed to take one register. bcc community reported > the > issue ([1]) where struct argument is not supported for fentry > program. > typedef struct { > uid_t val; > } kuid_t; > typedef struct { > gid_t val; > } kgid_t; > int security_path_chown(struct path *path, kuid_t uid, kgid_t gid); > Inside Meta, we also have a use case to attach to tcp_setsockopt() > typedef struct { > union { > void *kernel; > void __user *user; > }; > bool is_kernel : 1; > } sockptr_t; > int tcp_setsockopt(struct sock *sk, int level, int optname, > sockptr_t optval, unsigned int optlen); > > This patch added struct value support for bpf tracing programs which > uses trampoline. struct argument size needs to be 16 or less so > it can fit in one or two registers. Based on analysis on llvm and > experiments, atruct argument size greater than 16 will be passed > as pointer to the struct. Is it possible to force llvm to always pass a pointer to a struct over 8 bytes (the size of single register) for the BPF traget?
On 7/28/22 8:46 AM, Kui-Feng Lee wrote: > On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote: >> Currently struct arguments are not supported for trampoline based >> progs. >> One of major reason is that struct argument may pass by value which >> may >> use more than one registers. This breaks trampoline progs where >> each argument is assumed to take one register. bcc community reported >> the >> issue ([1]) where struct argument is not supported for fentry >> program. >> typedef struct { >> uid_t val; >> } kuid_t; >> typedef struct { >> gid_t val; >> } kgid_t; >> int security_path_chown(struct path *path, kuid_t uid, kgid_t gid); >> Inside Meta, we also have a use case to attach to tcp_setsockopt() >> typedef struct { >> union { >> void *kernel; >> void __user *user; >> }; >> bool is_kernel : 1; >> } sockptr_t; >> int tcp_setsockopt(struct sock *sk, int level, int optname, >> sockptr_t optval, unsigned int optlen); >> >> This patch added struct value support for bpf tracing programs which >> uses trampoline. struct argument size needs to be 16 or less so >> it can fit in one or two registers. Based on analysis on llvm and >> experiments, atruct argument size greater than 16 will be passed >> as pointer to the struct. > > Is it possible to force llvm to always pass a pointer to a struct over > 8 bytes (the size of single register) for the BPF traget? This is already the case for bpf target. Any struct parameter (1 byte, 2 bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a reference. But this is not the case for most other architectures. For example, for x86_64, in most cases, struct size <= 16 will be passed with two registers instead of as a reference.
On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote: > > > On 7/28/22 8:46 AM, Kui-Feng Lee wrote: > > On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote: > > > Currently struct arguments are not supported for trampoline based > > > progs. > > > One of major reason is that struct argument may pass by value > > > which > > > may > > > use more than one registers. This breaks trampoline progs where > > > each argument is assumed to take one register. bcc community > > > reported > > > the > > > issue ([1]) where struct argument is not supported for fentry > > > program. > > > typedef struct { > > > uid_t val; > > > } kuid_t; > > > typedef struct { > > > gid_t val; > > > } kgid_t; > > > int security_path_chown(struct path *path, kuid_t uid, kgid_t > > > gid); > > > Inside Meta, we also have a use case to attach to > > > tcp_setsockopt() > > > typedef struct { > > > union { > > > void *kernel; > > > void __user *user; > > > }; > > > bool is_kernel : 1; > > > } sockptr_t; > > > int tcp_setsockopt(struct sock *sk, int level, int optname, > > > sockptr_t optval, unsigned int optlen); > > > > > > This patch added struct value support for bpf tracing programs > > > which > > > uses trampoline. struct argument size needs to be 16 or less so > > > it can fit in one or two registers. Based on analysis on llvm and > > > experiments, atruct argument size greater than 16 will be passed > > > as pointer to the struct. > > > > Is it possible to force llvm to always pass a pointer to a struct > > over > > 8 bytes (the size of single register) for the BPF traget? > > This is already the case for bpf target. Any struct parameter (1 > byte, 2 > bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a > reference. > > But this is not the case for most other architectures. For example, > for > x86_64, in most cases, struct size <= 16 will be passed with two > registers instead of as a reference. I ask this question because you modify the signature of a bpf program to a pointer to a struct in patch #4. Is that necessary if the compiler passes a struct paramter as a reference?
On 7/28/22 12:57 PM, Kui-Feng Lee wrote: > On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote: >> >> >> On 7/28/22 8:46 AM, Kui-Feng Lee wrote: >>> On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote: >>>> Currently struct arguments are not supported for trampoline based >>>> progs. >>>> One of major reason is that struct argument may pass by value >>>> which >>>> may >>>> use more than one registers. This breaks trampoline progs where >>>> each argument is assumed to take one register. bcc community >>>> reported >>>> the >>>> issue ([1]) where struct argument is not supported for fentry >>>> program. >>>> typedef struct { >>>> uid_t val; >>>> } kuid_t; >>>> typedef struct { >>>> gid_t val; >>>> } kgid_t; >>>> int security_path_chown(struct path *path, kuid_t uid, kgid_t >>>> gid); >>>> Inside Meta, we also have a use case to attach to >>>> tcp_setsockopt() >>>> typedef struct { >>>> union { >>>> void *kernel; >>>> void __user *user; >>>> }; >>>> bool is_kernel : 1; >>>> } sockptr_t; >>>> int tcp_setsockopt(struct sock *sk, int level, int optname, >>>> sockptr_t optval, unsigned int optlen); >>>> >>>> This patch added struct value support for bpf tracing programs >>>> which >>>> uses trampoline. struct argument size needs to be 16 or less so >>>> it can fit in one or two registers. Based on analysis on llvm and >>>> experiments, atruct argument size greater than 16 will be passed >>>> as pointer to the struct. >>> >>> Is it possible to force llvm to always pass a pointer to a struct >>> over >>> 8 bytes (the size of single register) for the BPF traget? >> >> This is already the case for bpf target. Any struct parameter (1 >> byte, 2 >> bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a >> reference. >> >> But this is not the case for most other architectures. For example, >> for >> x86_64, in most cases, struct size <= 16 will be passed with two >> registers instead of as a reference. > > I ask this question because you modify the signature of a bpf program > to a pointer to a struct in patch #4. Is that necessary if the > compiler passes a struct paramter as a reference? Note that The true bpf program signature is only one. long bpf_prog(<ctx_type> *ctx) BPF_PROG is a macro for user friendly purpose. For example, +int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a, int b, int c) after macro expansion: int test_struct_arg_1(unsigned long long *ctx); static __attribute__((always_inline)) typeof(test_struct_arg_1(0)) ____test_struct_arg_1( unsigned long long *ctx, struct bpf_testmod_struct_arg_2 *a, int b, int c); ...
On Thu, 2022-07-28 at 16:30 -0700, Yonghong Song wrote: > > > On 7/28/22 12:57 PM, Kui-Feng Lee wrote: > > On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote: > > > > > > > > > On 7/28/22 8:46 AM, Kui-Feng Lee wrote: > > > > On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote: > > > > > Currently struct arguments are not supported for trampoline > > > > > based > > > > > progs. > > > > > One of major reason is that struct argument may pass by value > > > > > which > > > > > may > > > > > use more than one registers. This breaks trampoline progs > > > > > where > > > > > each argument is assumed to take one register. bcc community > > > > > reported > > > > > the > > > > > issue ([1]) where struct argument is not supported for fentry > > > > > program. > > > > > typedef struct { > > > > > uid_t val; > > > > > } kuid_t; > > > > > typedef struct { > > > > > gid_t val; > > > > > } kgid_t; > > > > > int security_path_chown(struct path *path, kuid_t uid, > > > > > kgid_t > > > > > gid); > > > > > Inside Meta, we also have a use case to attach to > > > > > tcp_setsockopt() > > > > > typedef struct { > > > > > union { > > > > > void *kernel; > > > > > void __user *user; > > > > > }; > > > > > bool is_kernel : 1; > > > > > } sockptr_t; > > > > > int tcp_setsockopt(struct sock *sk, int level, int > > > > > optname, > > > > > sockptr_t optval, unsigned int > > > > > optlen); > > > > > > > > > > This patch added struct value support for bpf tracing > > > > > programs > > > > > which > > > > > uses trampoline. struct argument size needs to be 16 or less > > > > > so > > > > > it can fit in one or two registers. Based on analysis on llvm > > > > > and > > > > > experiments, atruct argument size greater than 16 will be > > > > > passed > > > > > as pointer to the struct. > > > > > > > > Is it possible to force llvm to always pass a pointer to a > > > > struct > > > > over > > > > 8 bytes (the size of single register) for the BPF traget? > > > > > > This is already the case for bpf target. Any struct parameter (1 > > > byte, 2 > > > bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a > > > reference. > > > > > > But this is not the case for most other architectures. For > > > example, > > > for > > > x86_64, in most cases, struct size <= 16 will be passed with two > > > registers instead of as a reference. > > > > I ask this question because you modify the signature of a bpf > > program > > to a pointer to a struct in patch #4. Is that necessary if the > > compiler passes a struct paramter as a reference? > > Note that The true bpf program signature is only one. > long bpf_prog(<ctx_type> *ctx) > BPF_PROG is a macro for user friendly purpose. > > For example, > +int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a, > int > b, int c) > > after macro expansion: > int test_struct_arg_1(unsigned long long *ctx); > static __attribute__((always_inline)) > typeof(test_struct_arg_1(0)) ____test_struct_arg_1( > unsigned long long *ctx, > struct bpf_testmod_struct_arg_2 *a, int b, int c); > ... If cast the pointer of __test_struct_arg_1() to the type (int (*)(void*, void*, void*, void*)), test_struct_arg_1() can pass all arguments to __tst_struct_arg_1() in the type (void *) without changing the types of struct arguments to the pointer to a struct since all struct types will be passed as a reference for the BPF target. For example, the macro above can be expanded into the following block. ...... int test_struct_arg_1(unsigned long long *ctx) { ...... return ((typeof(name(0)) (*)(void*, void*, void*, void*))&____test_struct_arg_1)(ctx[0], ctx[1], ctx[2], ctx[3]); ...... } ......
On 7/29/22 11:04 AM, Kui-Feng Lee wrote: > On Thu, 2022-07-28 at 16:30 -0700, Yonghong Song wrote: >> >> >> On 7/28/22 12:57 PM, Kui-Feng Lee wrote: >>> On Thu, 2022-07-28 at 10:46 -0700, Yonghong Song wrote: >>>> >>>> >>>> On 7/28/22 8:46 AM, Kui-Feng Lee wrote: >>>>> On Tue, 2022-07-26 at 10:11 -0700, Yonghong Song wrote: >>>>>> Currently struct arguments are not supported for trampoline >>>>>> based >>>>>> progs. >>>>>> One of major reason is that struct argument may pass by value >>>>>> which >>>>>> may >>>>>> use more than one registers. This breaks trampoline progs >>>>>> where >>>>>> each argument is assumed to take one register. bcc community >>>>>> reported >>>>>> the >>>>>> issue ([1]) where struct argument is not supported for fentry >>>>>> program. >>>>>> typedef struct { >>>>>> uid_t val; >>>>>> } kuid_t; >>>>>> typedef struct { >>>>>> gid_t val; >>>>>> } kgid_t; >>>>>> int security_path_chown(struct path *path, kuid_t uid, >>>>>> kgid_t >>>>>> gid); >>>>>> Inside Meta, we also have a use case to attach to >>>>>> tcp_setsockopt() >>>>>> typedef struct { >>>>>> union { >>>>>> void *kernel; >>>>>> void __user *user; >>>>>> }; >>>>>> bool is_kernel : 1; >>>>>> } sockptr_t; >>>>>> int tcp_setsockopt(struct sock *sk, int level, int >>>>>> optname, >>>>>> sockptr_t optval, unsigned int >>>>>> optlen); >>>>>> >>>>>> This patch added struct value support for bpf tracing >>>>>> programs >>>>>> which >>>>>> uses trampoline. struct argument size needs to be 16 or less >>>>>> so >>>>>> it can fit in one or two registers. Based on analysis on llvm >>>>>> and >>>>>> experiments, atruct argument size greater than 16 will be >>>>>> passed >>>>>> as pointer to the struct. >>>>> >>>>> Is it possible to force llvm to always pass a pointer to a >>>>> struct >>>>> over >>>>> 8 bytes (the size of single register) for the BPF traget? >>>> >>>> This is already the case for bpf target. Any struct parameter (1 >>>> byte, 2 >>>> bytes, ..., 8 types, ..., 16 bytes, ...) will be passed as a >>>> reference. >>>> >>>> But this is not the case for most other architectures. For >>>> example, >>>> for >>>> x86_64, in most cases, struct size <= 16 will be passed with two >>>> registers instead of as a reference. >>> >>> I ask this question because you modify the signature of a bpf >>> program >>> to a pointer to a struct in patch #4. Is that necessary if the >>> compiler passes a struct paramter as a reference? >> >> Note that The true bpf program signature is only one. >> long bpf_prog(<ctx_type> *ctx) >> BPF_PROG is a macro for user friendly purpose. >> >> For example, >> +int BPF_PROG(test_struct_arg_1, struct bpf_testmod_struct_arg_2 *a, >> int >> b, int c) >> >> after macro expansion: >> int test_struct_arg_1(unsigned long long *ctx); >> static __attribute__((always_inline)) >> typeof(test_struct_arg_1(0)) ____test_struct_arg_1( >> unsigned long long *ctx, >> struct bpf_testmod_struct_arg_2 *a, int b, int c); >> ... > > If cast the pointer of __test_struct_arg_1() to the type (int > (*)(void*, void*, void*, void*)), test_struct_arg_1() can pass all > arguments to __tst_struct_arg_1() in the type (void *) without changing > the types of struct arguments to the pointer to a struct since all > struct types will be passed as a reference for the BPF target. > > For example, the macro above can be expanded into the following block. > ...... > int test_struct_arg_1(unsigned long long *ctx) { > ...... > return ((typeof(name(0)) (*)(void*, void*, void*, > void*))&____test_struct_arg_1)(ctx[0], ctx[1], ctx[2], ctx[3]); > ...... This doesn't really work. Here ctx[0]/ctx[1]/... has type 'unsigned long long' since ctx has type 'unsigned long long *'. But if later parameter type is 'struct bpf_testmod_struct_arg_2' in the function prototype and definition. Then implicitly we will have a type conversion like (struct bpf_testmod_struct_arg_2)(unsigned long long)ctx[...] and the above won't work as you cannot convert a 'unsigned long long' to a struct. > } > ...... >
Currently struct arguments are not supported for trampoline based progs. One of major reason is that struct argument may pass by value which may use more than one registers. This breaks trampoline progs where each argument is assumed to take one register. bcc community reported the issue ([1]) where struct argument is not supported for fentry program. typedef struct { uid_t val; } kuid_t; typedef struct { gid_t val; } kgid_t; int security_path_chown(struct path *path, kuid_t uid, kgid_t gid); Inside Meta, we also have a use case to attach to tcp_setsockopt() typedef struct { union { void *kernel; void __user *user; }; bool is_kernel : 1; } sockptr_t; int tcp_setsockopt(struct sock *sk, int level, int optname, sockptr_t optval, unsigned int optlen); This patch added struct value support for bpf tracing programs which uses trampoline. struct argument size needs to be 16 or less so it can fit in one or two registers. Based on analysis on llvm and experiments, atruct argument size greater than 16 will be passed as pointer to the struct. Please see patch #4 and selftests for specific examples. I labelled the patch as RFC so I can get some comments before proceeding. The patch set is not complete as: (1). it does not support struct arguments which are passed in as pointers. (2). there might be some corner cases where on x86_64 even 16 bytes may pass by pointers. This needs further investigation. (3). tests are imcomplete, no fexit or fmod tests. [1] https://github.com/iovisor/bcc/issues/3657 [2] https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.cpp Yonghong Song (7): bpf: Always return corresponding btf_type in __get_type_size() bpf: Add struct argument info in btf_func_model bpf: x86: Rename stack_size to regs_off in {save,restore}_regs() bpf: x86: Support in-register struct arguments bpf: arm64: No support of struct value argument bpf: Populate struct value info in btf_func_model selftests/bpf: Add struct value tests with fentry programs. arch/arm64/net/bpf_jit_comp.c | 4 + arch/x86/net/bpf_jit_comp.c | 173 ++++++++++++++++-- include/linux/bpf.h | 9 + kernel/bpf/btf.c | 54 +++++- .../selftests/bpf/bpf_testmod/bpf_testmod.c | 37 ++++ .../selftests/bpf/prog_tests/tracing_struct.c | 51 ++++++ .../selftests/bpf/progs/tracing_struct.c | 64 +++++++ 7 files changed, 365 insertions(+), 27 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_struct.c create mode 100644 tools/testing/selftests/bpf/progs/tracing_struct.c