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

[Yonghong Song <yhs@fb.com>]

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);
...