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. > } > ...... >