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

[Kui-Feng Lee <kuifeng@fb.com>]

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