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: <5eecf98ba643c_137b2ad09f64a5c458@john-XPS-13-9370.notmuch>
Date:   Fri, 19 Jun 2020 10:44:43 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Yonghong Song <yhs@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Jiri Olsa <jolsa@...hat.com>, Andrii Nakryiko <andriin@...com>,
        Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...hat.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        KP Singh <kpsingh@...omium.org>,
        Masanori Misono <m.misono760@...il.com>
Subject: Re: [PATCH] bpf: Allow small structs to be type of function argument

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