[PATCH] bpf: Allow small structs to be type of function argument

[PATCH] bpf: Allow small structs to be type of function argument

[Jiri Olsa]

This way we can have trampoline on function
that has arguments with types like:

  kuid_t uid
  kgid_t gid

which unwind into small structs like:

  typedef struct {
        uid_t val;
  } kuid_t;

  typedef struct {
        gid_t val;
  } kgid_t;

And we can use them in bpftrace like:
(assuming d_path changes are in)

  # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
  Attaching 1 probe...
  uid 0, gid 0
  uid 1000, gid 1000
  ...

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 58c9af1d4808..f8fee5833684 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
 	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
 }
 
+/* type is struct and its size is within 8 bytes
+ * and it can be value of function argument
+ */
+static bool btf_type_is_struct_arg(const struct btf_type *t)
+{
+	return btf_type_is_struct(t) && (t->size <= sizeof(u64));
+}
+
 static bool __btf_type_is_struct(const struct btf_type *t)
 {
 	return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
@@ -3768,7 +3776,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	/* skip modifiers */
 	while (btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
-	if (btf_type_is_int(t) || btf_type_is_enum(t))
+	if (btf_type_is_int(t) || btf_type_is_enum(t) || btf_type_is_struct_arg(t))
 		/* accessing a scalar */
 		return true;
 	if (!btf_type_is_ptr(t)) {
@@ -4161,6 +4169,8 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
 		return sizeof(void *);
 	if (btf_type_is_int(t) || btf_type_is_enum(t))
 		return t->size;
+	if (btf_type_is_struct_arg(t))
+		return t->size;
 	*bad_type = t;
 	return -EINVAL;
 }
-- 
2.25.4

RE: [PATCH] bpf: Allow small structs to be type of function argument

[John Fastabend]

Jiri Olsa wrote:
> This way we can have trampoline on function
> that has arguments with types like:
> 
>   kuid_t uid
>   kgid_t gid
> 
> which unwind into small structs like:
> 
>   typedef struct {
>         uid_t val;
>   } kuid_t;
> 
>   typedef struct {
>         gid_t val;
>   } kgid_t;
> 
> And we can use them in bpftrace like:
> (assuming d_path changes are in)
> 
>   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
>   Attaching 1 probe...
>   uid 0, gid 0
>   uid 1000, gid 1000
>   ...
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/btf.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 58c9af1d4808..f8fee5833684 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
>  	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
>  }
>  
> +/* type is struct and its size is within 8 bytes
> + * and it can be value of function argument
> + */
> +static bool btf_type_is_struct_arg(const struct btf_type *t)
> +{
> +	return btf_type_is_struct(t) && (t->size <= sizeof(u64));

Can you comment on why sizeof(u64) here? The int types can be larger
than 64 for example and don't have a similar check, maybe the should
as well?

Here is an example from some made up program I ran through clang and
bpftool.

[2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED

We also have btf_type_int_is_regular to decide if the int is of some
"regular" size but I don't see it used in these paths.

> +}
> +
>  static bool __btf_type_is_struct(const struct btf_type *t)
>  {
>  	return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
> @@ -3768,7 +3776,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  	/* skip modifiers */
>  	while (btf_type_is_modifier(t))
>  		t = btf_type_by_id(btf, t->type);
> -	if (btf_type_is_int(t) || btf_type_is_enum(t))
> +	if (btf_type_is_int(t) || btf_type_is_enum(t) || btf_type_is_struct_arg(t))
>  		/* accessing a scalar */
>  		return true;
>  	if (!btf_type_is_ptr(t)) {
> @@ -4161,6 +4169,8 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>  		return sizeof(void *);
>  	if (btf_type_is_int(t) || btf_type_is_enum(t))
>  		return t->size;
> +	if (btf_type_is_struct_arg(t))
> +		return t->size;
>  	*bad_type = t;
>  	return -EINVAL;
>  }
> -- 
> 2.25.4
> 

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Jiri Olsa]

On Wed, Jun 17, 2020 at 04:20:54PM -0700, John Fastabend wrote:
> Jiri Olsa wrote:
> > This way we can have trampoline on function
> > that has arguments with types like:
> > 
> >   kuid_t uid
> >   kgid_t gid
> > 
> > which unwind into small structs like:
> > 
> >   typedef struct {
> >         uid_t val;
> >   } kuid_t;
> > 
> >   typedef struct {
> >         gid_t val;
> >   } kgid_t;
> > 
> > And we can use them in bpftrace like:
> > (assuming d_path changes are in)
> > 
> >   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
> >   Attaching 1 probe...
> >   uid 0, gid 0
> >   uid 1000, gid 1000
> >   ...
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/btf.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 58c9af1d4808..f8fee5833684 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
> >  	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> >  }
> >  
> > +/* type is struct and its size is within 8 bytes
> > + * and it can be value of function argument
> > + */
> > +static bool btf_type_is_struct_arg(const struct btf_type *t)
> > +{
> > +	return btf_type_is_struct(t) && (t->size <= sizeof(u64));
> 
> Can you comment on why sizeof(u64) here? The int types can be larger
> than 64 for example and don't have a similar check, maybe the should
> as well?
> 
> Here is an example from some made up program I ran through clang and
> bpftool.
> 
> [2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED
> 
> We also have btf_type_int_is_regular to decide if the int is of some
> "regular" size but I don't see it used in these paths.

so this small structs are passed as scalars via function arguments,
so the size limit is to fit teir value into register size which holds
the argument

I'm not sure how 128bit numbers are passed to function as argument,
but I think we can treat them separately if there's a need

jirka

> 
> > +}
> > +
> >  static bool __btf_type_is_struct(const struct btf_type *t)
> >  {
> >  	return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
> > @@ -3768,7 +3776,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >  	/* skip modifiers */
> >  	while (btf_type_is_modifier(t))
> >  		t = btf_type_by_id(btf, t->type);
> > -	if (btf_type_is_int(t) || btf_type_is_enum(t))
> > +	if (btf_type_is_int(t) || btf_type_is_enum(t) || btf_type_is_struct_arg(t))
> >  		/* accessing a scalar */
> >  		return true;
> >  	if (!btf_type_is_ptr(t)) {
> > @@ -4161,6 +4169,8 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
> >  		return sizeof(void *);
> >  	if (btf_type_is_int(t) || btf_type_is_enum(t))
> >  		return t->size;
> > +	if (btf_type_is_struct_arg(t))
> > +		return t->size;
> >  	*bad_type = t;
> >  	return -EINVAL;
> >  }
> > -- 
> > 2.25.4
> > 
> 
> 

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Alexei Starovoitov]

On Thu, Jun 18, 2020 at 01:48:06PM +0200, Jiri Olsa wrote:
> On Wed, Jun 17, 2020 at 04:20:54PM -0700, John Fastabend wrote:
> > Jiri Olsa wrote:
> > > This way we can have trampoline on function
> > > that has arguments with types like:
> > > 
> > >   kuid_t uid
> > >   kgid_t gid
> > > 
> > > which unwind into small structs like:
> > > 
> > >   typedef struct {
> > >         uid_t val;
> > >   } kuid_t;
> > > 
> > >   typedef struct {
> > >         gid_t val;
> > >   } kgid_t;
> > > 
> > > And we can use them in bpftrace like:
> > > (assuming d_path changes are in)

the patch doesn't seem to be related to d_path. Unless I'm missing something.

Please add a selftest. bpftrace example is nice, but selftest is still mandatory.

> > > 
> > >   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
> > >   Attaching 1 probe...
> > >   uid 0, gid 0
> > >   uid 1000, gid 1000
> > >   ...
> > > 
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  kernel/bpf/btf.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 58c9af1d4808..f8fee5833684 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
> > >  	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> > >  }
> > >  
> > > +/* type is struct and its size is within 8 bytes
> > > + * and it can be value of function argument
> > > + */
> > > +static bool btf_type_is_struct_arg(const struct btf_type *t)
> > > +{
> > > +	return btf_type_is_struct(t) && (t->size <= sizeof(u64));

extra () are unnecessary.

the function needs different name. May btf_type_is_struct_by_value() ?

> > 
> > Can you comment on why sizeof(u64) here? The int types can be larger
> > than 64 for example and don't have a similar check, maybe the should
> > as well?
> > 
> > Here is an example from some made up program I ran through clang and
> > bpftool.
> > 
> > [2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED
> > 
> > We also have btf_type_int_is_regular to decide if the int is of some
> > "regular" size but I don't see it used in these paths.
> 
> so this small structs are passed as scalars via function arguments,
> so the size limit is to fit teir value into register size which holds
> the argument
> 
> I'm not sure how 128bit numbers are passed to function as argument,
> but I think we can treat them separately if there's a need
> 
> jirka
> 
> > 
> > > +}
> > > +
> > >  static bool __btf_type_is_struct(const struct btf_type *t)
> > >  {
> > >  	return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
> > > @@ -3768,7 +3776,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > >  	/* skip modifiers */
> > >  	while (btf_type_is_modifier(t))
> > >  		t = btf_type_by_id(btf, t->type);
> > > -	if (btf_type_is_int(t) || btf_type_is_enum(t))
> > > +	if (btf_type_is_int(t) || btf_type_is_enum(t) || btf_type_is_struct_arg(t))
> > >  		/* accessing a scalar */
> > >  		return true;

It probably needs to be x86 gated?
I don't think all archs do that for small structs.

What kind of code clang generates for bpf prog?
I don't remember what we told clang to do for struct by value.
That has to be carefully defined and tested.

Re: [PATCH] bpf: Allow small structs to be type of function argument

[John Fastabend]

Jiri Olsa wrote:
> On Wed, Jun 17, 2020 at 04:20:54PM -0700, John Fastabend wrote:
> > Jiri Olsa wrote:
> > > This way we can have trampoline on function
> > > that has arguments with types like:
> > > 
> > >   kuid_t uid
> > >   kgid_t gid
> > > 
> > > which unwind into small structs like:
> > > 
> > >   typedef struct {
> > >         uid_t val;
> > >   } kuid_t;
> > > 
> > >   typedef struct {
> > >         gid_t val;
> > >   } kgid_t;
> > > 
> > > And we can use them in bpftrace like:
> > > (assuming d_path changes are in)
> > > 
> > >   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
> > >   Attaching 1 probe...
> > >   uid 0, gid 0
> > >   uid 1000, gid 1000
> > >   ...
> > > 
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  kernel/bpf/btf.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 58c9af1d4808..f8fee5833684 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
> > >  	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> > >  }
> > >  
> > > +/* type is struct and its size is within 8 bytes
> > > + * and it can be value of function argument
> > > + */
> > > +static bool btf_type_is_struct_arg(const struct btf_type *t)
> > > +{
> > > +	return btf_type_is_struct(t) && (t->size <= sizeof(u64));
> > 
> > Can you comment on why sizeof(u64) here? The int types can be larger
> > than 64 for example and don't have a similar check, maybe the should
> > as well?
> > 
> > Here is an example from some made up program I ran through clang and
> > bpftool.
> > 
> > [2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED
> > 
> > We also have btf_type_int_is_regular to decide if the int is of some
> > "regular" size but I don't see it used in these paths.
> 
> so this small structs are passed as scalars via function arguments,
> so the size limit is to fit teir value into register size which holds
> the argument
> 
> I'm not sure how 128bit numbers are passed to function as argument,
> but I think we can treat them separately if there's a need
> 

Moving Andrii up to the TO field ;)

Andrii, do we also need a guard on the int type with sizeof(u64)?
Otherwise the arg calculation might be incorrect? wdyt did I follow
along correctly.

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Andrii Nakryiko]

On Thu, Jun 18, 2020 at 3:50 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Jiri Olsa wrote:
> > On Wed, Jun 17, 2020 at 04:20:54PM -0700, John Fastabend wrote:
> > > Jiri Olsa wrote:
> > > > This way we can have trampoline on function
> > > > that has arguments with types like:
> > > >
> > > >   kuid_t uid
> > > >   kgid_t gid
> > > >
> > > > which unwind into small structs like:
> > > >
> > > >   typedef struct {
> > > >         uid_t val;
> > > >   } kuid_t;
> > > >
> > > >   typedef struct {
> > > >         gid_t val;
> > > >   } kgid_t;
> > > >
> > > > And we can use them in bpftrace like:
> > > > (assuming d_path changes are in)
> > > >
> > > >   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
> > > >   Attaching 1 probe...
> > > >   uid 0, gid 0
> > > >   uid 1000, gid 1000
> > > >   ...
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  kernel/bpf/btf.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 58c9af1d4808..f8fee5833684 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
> > > >   return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> > > >  }
> > > >
> > > > +/* type is struct and its size is within 8 bytes
> > > > + * and it can be value of function argument
> > > > + */
> > > > +static bool btf_type_is_struct_arg(const struct btf_type *t)
> > > > +{
> > > > + return btf_type_is_struct(t) && (t->size <= sizeof(u64));
> > >
> > > Can you comment on why sizeof(u64) here? The int types can be larger
> > > than 64 for example and don't have a similar check, maybe the should
> > > as well?
> > >
> > > Here is an example from some made up program I ran through clang and
> > > bpftool.
> > >
> > > [2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED
> > >
> > > We also have btf_type_int_is_regular to decide if the int is of some
> > > "regular" size but I don't see it used in these paths.
> >
> > so this small structs are passed as scalars via function arguments,
> > so the size limit is to fit teir value into register size which holds
> > the argument
> >
> > I'm not sure how 128bit numbers are passed to function as argument,
> > but I think we can treat them separately if there's a need
> >
>
> Moving Andrii up to the TO field ;)

I've got an upgrade, thanks :)

>
> Andrii, do we also need a guard on the int type with sizeof(u64)?
> Otherwise the arg calculation might be incorrect? wdyt did I follow
> along correctly.

Yes, we probably do. I actually never used __int128 in practice, but
decided to look at what Clang does for a function accepting __int128.
Turns out it passed it in two consecutive registers. So:

__weak int bla(__int128 x) { return (int)(x + 1); }

The assembly is:

      38:       b7 01 00 00 fe ff ff ff r1 = -2
      39:       b7 02 00 00 ff ff ff ff r2 = -1
      40:       85 10 00 00 ff ff ff ff call -1
      41:       bc 01 00 00 00 00 00 00 w1 = w0

So low 64-bits go into r1, high 64-bits into r2.

Which means the 1:1 mapping between registers and input arguments
breaks with __int128, at least for target BPF. I'm too lazy to check
for x86-64, though.

Re: [PATCH] bpf: Allow small structs to be type of function argument

[John Fastabend]

Andrii Nakryiko wrote:
> On Thu, Jun 18, 2020 at 3:50 PM John Fastabend <john.fastabend@gmail.com> wrote:
> >
> > Jiri Olsa wrote:
> > > On Wed, Jun 17, 2020 at 04:20:54PM -0700, John Fastabend wrote:
> > > > Jiri Olsa wrote:
> > > > > This way we can have trampoline on function
> > > > > that has arguments with types like:
> > > > >
> > > > >   kuid_t uid
> > > > >   kgid_t gid
> > > > >
> > > > > which unwind into small structs like:
> > > > >
> > > > >   typedef struct {
> > > > >         uid_t val;
> > > > >   } kuid_t;
> > > > >
> > > > >   typedef struct {
> > > > >         gid_t val;
> > > > >   } kgid_t;
> > > > >
> > > > > And we can use them in bpftrace like:
> > > > > (assuming d_path changes are in)
> > > > >
> > > > >   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
> > > > >   Attaching 1 probe...
> > > > >   uid 0, gid 0
> > > > >   uid 1000, gid 1000
> > > > >   ...
> > > > >
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  kernel/bpf/btf.c | 12 +++++++++++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > > index 58c9af1d4808..f8fee5833684 100644
> > > > > --- a/kernel/bpf/btf.c
> > > > > +++ b/kernel/bpf/btf.c
> > > > > @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
> > > > >   return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> > > > >  }
> > > > >
> > > > > +/* type is struct and its size is within 8 bytes
> > > > > + * and it can be value of function argument
> > > > > + */
> > > > > +static bool btf_type_is_struct_arg(const struct btf_type *t)
> > > > > +{
> > > > > + return btf_type_is_struct(t) && (t->size <= sizeof(u64));
> > > >
> > > > Can you comment on why sizeof(u64) here? The int types can be larger
> > > > than 64 for example and don't have a similar check, maybe the should
> > > > as well?
> > > >
> > > > Here is an example from some made up program I ran through clang and
> > > > bpftool.
> > > >
> > > > [2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED
> > > >
> > > > We also have btf_type_int_is_regular to decide if the int is of some
> > > > "regular" size but I don't see it used in these paths.
> > >
> > > so this small structs are passed as scalars via function arguments,
> > > so the size limit is to fit teir value into register size which holds
> > > the argument
> > >
> > > I'm not sure how 128bit numbers are passed to function as argument,
> > > but I think we can treat them separately if there's a need
> > >
> >
> > Moving Andrii up to the TO field ;)
> 
> I've got an upgrade, thanks :)
> 
> >
> > Andrii, do we also need a guard on the int type with sizeof(u64)?
> > Otherwise the arg calculation might be incorrect? wdyt did I follow
> > along correctly.
> 
> Yes, we probably do. I actually never used __int128 in practice, but
> decided to look at what Clang does for a function accepting __int128.
> Turns out it passed it in two consecutive registers. So:
> 
> __weak int bla(__int128 x) { return (int)(x + 1); }
> 
> The assembly is:
> 
>       38:       b7 01 00 00 fe ff ff ff r1 = -2
>       39:       b7 02 00 00 ff ff ff ff r2 = -1
>       40:       85 10 00 00 ff ff ff ff call -1
>       41:       bc 01 00 00 00 00 00 00 w1 = w0
> 
> So low 64-bits go into r1, high 64-bits into r2.
> 
> Which means the 1:1 mapping between registers and input arguments
> breaks with __int128, at least for target BPF. I'm too lazy to check
> for x86-64, though.

OK confirms what I suspected. For a fix we should bound int types
here to pointer word size which I think should be safe most everywhere.
I can draft a patch if you haven't done one already. For what its worth
RISC-V had some convention where it would use the even registers for
things. So

 foo(int a, __int128 b)

would put a in r0 and b in r2 and r3 leaving a hole in r1. But that
was some old reference manual and  might no longer be the case
in reality. Perhaps just spreading hearsay, but the point is we
should say something about what the BPF backend convention
is and write it down. We've started to bump into these things
lately.

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Alexei Starovoitov]

On Thu, Jun 18, 2020 at 5:26 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
>  foo(int a, __int128 b)
>
> would put a in r0 and b in r2 and r3 leaving a hole in r1. But that
> was some old reference manual and  might no longer be the case
> in reality. Perhaps just spreading hearsay, but the point is we
> should say something about what the BPF backend convention
> is and write it down. We've started to bump into these things
> lately.

calling convention for int128 in bpf is _undefined_.
calling convention for struct by value in bpf is also _undefined_.

In many cases the compiler has to have the backend code
so other parts of the compiler can function.
I didn't bother explicitly disabling every undefined case.
Please don't read too much into llvm generated code.

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Yonghong Song]

On 6/18/20 7:04 PM, Alexei Starovoitov wrote:
> On Thu, Jun 18, 2020 at 5:26 PM John Fastabend <john.fastabend@gmail.com> wrote:
>>
>>   foo(int a, __int128 b)
>>
>> would put a in r0 and b in r2 and r3 leaving a hole in r1. But that
>> was some old reference manual and  might no longer be the case

This should not happen if clang compilation with -target bpf.
This MAY happen if they compile with 'clang -target riscv' as the IR
could change before coming to bpf backend.

>> in reality. Perhaps just spreading hearsay, but the point is we
>> should say something about what the BPF backend convention
>> is and write it down. We've started to bump into these things
>> lately.
> 
> calling convention for int128 in bpf is _undefined_.
> calling convention for struct by value in bpf is also _undefined_.

Just to clarify a little bit. bpf backend did not do anything
special about int128 and struct type. It is using llvm default.
That is, int128 using two argument registers and struct passed
by address. But I do see some other architectures having their
own ways to handle these parameters like X86, AARCH64, AMDGPU, MIPS.

int128 is not widely used. passing struct as the argument is not
a good practice. So Agree with Alexei is not really worthwhile to
handle them in the place of arguments.

> 
> In many cases the compiler has to have the backend code
> so other parts of the compiler can function.
> I didn't bother explicitly disabling every undefined case.
> Please don't read too much into llvm generated code.
> 

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Jiri Olsa]

On Thu, Jun 18, 2020 at 03:05:11PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 18, 2020 at 01:48:06PM +0200, Jiri Olsa wrote:
> > On Wed, Jun 17, 2020 at 04:20:54PM -0700, John Fastabend wrote:
> > > Jiri Olsa wrote:
> > > > This way we can have trampoline on function
> > > > that has arguments with types like:
> > > > 
> > > >   kuid_t uid
> > > >   kgid_t gid
> > > > 
> > > > which unwind into small structs like:
> > > > 
> > > >   typedef struct {
> > > >         uid_t val;
> > > >   } kuid_t;
> > > > 
> > > >   typedef struct {
> > > >         gid_t val;
> > > >   } kgid_t;
> > > > 
> > > > And we can use them in bpftrace like:
> > > > (assuming d_path changes are in)
> 
> the patch doesn't seem to be related to d_path. Unless I'm missing something.

ugh, sry.. I had bpftrace example with dpath call in it,
then I removed it, but did not remove the comment ;-)

> 
> Please add a selftest. bpftrace example is nice, but selftest is still mandatory.

ok

> 
> > > > 
> > > >   # bpftrace -e 'lsm:path_chown { printf("uid %d, gid %d\n", args->uid, args->gid) }'
> > > >   Attaching 1 probe...
> > > >   uid 0, gid 0
> > > >   uid 1000, gid 1000
> > > >   ...
> > > > 
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  kernel/bpf/btf.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 58c9af1d4808..f8fee5833684 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -362,6 +362,14 @@ static bool btf_type_is_struct(const struct btf_type *t)
> > > >  	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
> > > >  }
> > > >  
> > > > +/* type is struct and its size is within 8 bytes
> > > > + * and it can be value of function argument
> > > > + */
> > > > +static bool btf_type_is_struct_arg(const struct btf_type *t)
> > > > +{
> > > > +	return btf_type_is_struct(t) && (t->size <= sizeof(u64));
> 
> extra () are unnecessary.
> 
> the function needs different name. May btf_type_is_struct_by_value() ?

ok

> 
> > > 
> > > Can you comment on why sizeof(u64) here? The int types can be larger
> > > than 64 for example and don't have a similar check, maybe the should
> > > as well?
> > > 
> > > Here is an example from some made up program I ran through clang and
> > > bpftool.
> > > 
> > > [2] INT '__int128' size=16 bits_offset=0 nr_bits=128 encoding=SIGNED
> > > 
> > > We also have btf_type_int_is_regular to decide if the int is of some
> > > "regular" size but I don't see it used in these paths.
> > 
> > so this small structs are passed as scalars via function arguments,
> > so the size limit is to fit teir value into register size which holds
> > the argument
> > 
> > I'm not sure how 128bit numbers are passed to function as argument,
> > but I think we can treat them separately if there's a need
> > 
> > jirka
> > 
> > > 
> > > > +}
> > > > +
> > > >  static bool __btf_type_is_struct(const struct btf_type *t)
> > > >  {
> > > >  	return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
> > > > @@ -3768,7 +3776,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > >  	/* skip modifiers */
> > > >  	while (btf_type_is_modifier(t))
> > > >  		t = btf_type_by_id(btf, t->type);
> > > > -	if (btf_type_is_int(t) || btf_type_is_enum(t))
> > > > +	if (btf_type_is_int(t) || btf_type_is_enum(t) || btf_type_is_struct_arg(t))
> > > >  		/* accessing a scalar */
> > > >  		return true;
> 
> It probably needs to be x86 gated?
> I don't think all archs do that for small structs.

right, but if btf_type_is_struct_arg == true in here,
the struct is in the argument value

> 
> What kind of code clang generates for bpf prog?
> I don't remember what we told clang to do for struct by value.
> That has to be carefully defined and tested.

will check,

thanks
jirka

Re: [PATCH] bpf: Allow small structs to be type of function argument

[John Fastabend]

Yonghong Song wrote:
> 
> 
> On 6/18/20 7:04 PM, Alexei Starovoitov wrote:
> > On Thu, Jun 18, 2020 at 5:26 PM John Fastabend <john.fastabend@gmail.com> wrote:
> >>
> >>   foo(int a, __int128 b)
> >>
> >> would put a in r0 and b in r2 and r3 leaving a hole in r1. But that
> >> was some old reference manual and  might no longer be the case
> 
> This should not happen if clang compilation with -target bpf.
> This MAY happen if they compile with 'clang -target riscv' as the IR
> could change before coming to bpf backend.

I guess this means in order to handle __int128 and structs in
btf_ctx_access we would have to know this did not happen. Otherwise
the arg to type mappings are off because we simply do

 arg = off / 8

> 
> >> in reality. Perhaps just spreading hearsay, but the point is we
> >> should say something about what the BPF backend convention
> >> is and write it down. We've started to bump into these things
> >> lately.
> > 
> > calling convention for int128 in bpf is _undefined_.
> > calling convention for struct by value in bpf is also _undefined_.
> 
> Just to clarify a little bit. bpf backend did not do anything
> special about int128 and struct type. It is using llvm default.
> That is, int128 using two argument registers and struct passed
> by address. But I do see some other architectures having their
> own ways to handle these parameters like X86, AARCH64, AMDGPU, MIPS.
> 
> int128 is not widely used. passing struct as the argument is not
> a good practice. So Agree with Alexei is not really worthwhile to
> handle them in the place of arguments.

Agree as well I'll just add a small fix to check btf_type_is_int()
size is <= u64 and that should be sufficient to skip handling the
int128 case.

> 
> > 
> > In many cases the compiler has to have the backend code
> > so other parts of the compiler can function.
> > I didn't bother explicitly disabling every undefined case.
> > Please don't read too much into llvm generated code.
> > 

Re: [PATCH] bpf: Allow small structs to be type of function argument

[Yonghong Song]

On 6/19/20 10:44 AM, John Fastabend wrote:
> Yonghong Song wrote:
>>
>>
>> On 6/18/20 7:04 PM, Alexei Starovoitov wrote:
>>> On Thu, Jun 18, 2020 at 5:26 PM John Fastabend <john.fastabend@gmail.com> wrote:
>>>>
>>>>    foo(int a, __int128 b)
>>>>
>>>> would put a in r0 and b in r2 and r3 leaving a hole in r1. But that
>>>> was some old reference manual and  might no longer be the case
>>
>> This should not happen if clang compilation with -target bpf.
>> This MAY happen if they compile with 'clang -target riscv' as the IR
>> could change before coming to bpf backend.
> 
> I guess this means in order to handle __int128 and structs in
> btf_ctx_access we would have to know this did not happen. Otherwise
> the arg to type mappings are off because we simply do
> 
>   arg = off / 8

Yes, btf_ctx_access already disqualified struct type, so a refined
check to ensure int width <= 64 should be sufficient.

> 
>>
>>>> in reality. Perhaps just spreading hearsay, but the point is we
>>>> should say something about what the BPF backend convention
>>>> is and write it down. We've started to bump into these things
>>>> lately.
>>>
>>> calling convention for int128 in bpf is _undefined_.
>>> calling convention for struct by value in bpf is also _undefined_.
>>
>> Just to clarify a little bit. bpf backend did not do anything
>> special about int128 and struct type. It is using llvm default.
>> That is, int128 using two argument registers and struct passed
>> by address. But I do see some other architectures having their
>> own ways to handle these parameters like X86, AARCH64, AMDGPU, MIPS.
>>
>> int128 is not widely used. passing struct as the argument is not
>> a good practice. So Agree with Alexei is not really worthwhile to
>> handle them in the place of arguments.
> 
> Agree as well I'll just add a small fix to check btf_type_is_int()
> size is <= u64 and that should be sufficient to skip handling the
> int128 case.

Agree.

> 
>>
>>>
>>> In many cases the compiler has to have the backend code
>>> so other parts of the compiler can function.
>>> I didn't bother explicitly disabling every undefined case.
>>> Please don't read too much into llvm generated code.
>>>
> 
>