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] [day] [month] [year] [list]

Message-ID: <4a82d587-0ca0-d04d-76c8-e83e9e5076f8@fb.com>
Date:   Fri, 19 Jun 2020 11:56:07 -0700
From:   Yonghong Song <yhs@...com>
To:     John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...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



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

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