Re: [RFC PATCH bpf-next 0/7] bpf: Support struct value argument for trampoline base progs

[Yonghong Song <yhs@fb.com>]

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.

>    }
>    ......
>