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: <CAM1=_QQJ+uYXuU_nOVb3djW-G8wJs4Azz36pXk8mO3vQBuVouQ@mail.gmail.com>
Date:   Thu, 29 Jul 2021 14:34:02 +0200
From:   Johan Almbladh <johan.almbladh@...finetworks.com>
To:     Yonghong Song <yhs@...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 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.

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

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

Johan