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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200618114806.GA2369163@krava>
Date:   Thu, 18 Jun 2020 13:48:06 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...hat.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Andrii Nakryiko <andriin@...com>,
        KP Singh <kpsingh@...omium.org>,
        Masanori Misono <m.misono760@...il.com>
Subject: Re: [PATCH] bpf: Allow small structs to be type of function argument

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@...nel.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
> > 
> 
>