netdev - Re: [PATCH 06/14] bpf/tests: Add more BPF

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: <12ed8726-41c6-b173-b30a-1bd625a12718@fb.com>
Date:   Thu, 29 Jul 2021 08:39:25 -0700
From:   Yonghong Song <yhs@...com>
To:     Johan Almbladh <johan.almbladh@...finetworks.com>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Tony Ambardar <Tony.Ambardar@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 06/14] bpf/tests: Add more BPF_LSH/RSH/ARSH tests for
 ALU64



On 7/29/21 5:34 AM, Johan Almbladh wrote:
> On Thu, Jul 29, 2021 at 1:30 AM Yonghong Song <yhs@...com> wrote:
>>> @@ -4139,6 +4139,106 @@ static struct bpf_test tests[] = {
>>>                { },
>>>                { { 0, 0x80000000 } },
>>>        },
>>> +     {
>>> +             "ALU64_LSH_X: Shift < 32, low word",
>>> +             .u.insns_int = {
>>> +                     BPF_LD_IMM64(R0, 0x0123456789abcdefLL),
>>> +                     BPF_ALU32_IMM(BPF_MOV, R1, 12),
>>> +                     BPF_ALU64_REG(BPF_LSH, R0, R1),
>>> +                     BPF_EXIT_INSN(),
>>> +             },
>>> +             INTERNAL,
>>> +             { },
>>> +             { { 0, 0xbcdef000 } }
>>
>> In bpf_test struct, the result is defined as __u32
>>           struct {
>>                   int data_size;
>>                   __u32 result;
>>           } test[MAX_SUBTESTS];
>>
>> But the above result 0xbcdef000 does not really capture the bpf program
>> return value, which should be 0x3456789abcdef000.
>> Can we change "result" type to __u64 so the result truly captures the
>> program return value?
> 
> This was also my though at first, but I don't think that is possible.
> As I understand it, the eBPF functions have the prototype int
> func(struct *ctx). While the context pointer will have a different
> size on 32-bit and 64-bit architectures, the return value will always
> be 32 bits on most, or all, platforms.

Thanks for explanation. Yes, all BPF_PROG_RUN variables have bpf program
return type u32, so you are right, we cannot really check prog return
value against a 64bit R0.

> 
>> We have several other similar cases for the rest of this patch.
> 
> I have used two ways to check the full 64-bit result in such cases.
> 
> 1) Load the expected result as a 64-bit value in a register. Then jump
> conditionally if the result matches this value or not. The jump
> destinations each set a distinct value in R0, which is finally
> examined as the result.
> 
> 2) Run the test twice. The first one returns the low 32-bits of R0.
> The second adds a 32-bit right shift to return the high 32 bits.
> 
> When I first wrote the tests I tried to use as few complex
> instructions not under test as possible, in order to test each
> instruction in isolation. Since the 32-bit right shift is a much
> simpler operation than conditional jumps, at least in the 32-bit MIPS
> JIT, I chose method (2) for most of the tests. Existing tests seem to
> use method (1), so in some cases I used that instead when adding more
> tests of the same operation. The motivation for the simple one-by-one
> tests is mainly convenience and better diagnostics during JIT
> development. Both methods (1) and (2) are equally valid of course.

it is totally okay to use (2). Your tests are fine in that regard.

> 
> By the way, thanks a lot for the review, Yonghong!

You are welcome!

> 
> Johan
>