netdev - Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run

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: <2b0b828a-ce02-b4dc-7f95-d7d1238e40bf@iogearbox.net>
Date:   Mon, 26 Nov 2018 14:39:15 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Lorenz Bauer <lmb@...udflare.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        linux-api@...r.kernel.org, Y Song <ys114321@...il.com>
Subject: Re: [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr

On 11/26/2018 01:45 PM, Lorenz Bauer wrote:
> On Sat, 24 Nov 2018 at 22:20, Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
>> On Fri, Nov 23, 2018 at 11:25:11PM +0100, Daniel Borkmann wrote:
>>> On 11/22/2018 03:09 PM, Lorenz Bauer wrote:
[...]
>>>>  LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
>>>>                              __u32 size, void *data_out, __u32 *size_out,
>>>>                              __u32 *retval, __u32 *duration);
>>>
>>> Could we add a comment into the header here stating that we discourage
>>> bpf_prog_test_run()'s use?
>>>
>>> It would probably also make sense since we go that route that we would
>>> convert the 10 bpf_prog_test_run() instances under test_progs.c at the
>>> same time so that people extending or looking at BPF kselftests don't
>>> copy discouraged bpf_prog_test_run() api as examples from this point
>>> onwards anymore.
>>
>> I would keep bpf_prog_test_run() and test_progs.c as-is.
>> I think the prog_test_run in the current form is actually fine.
>> I don't find it 'unsafe'.
>> When it's called on a given bpf prog the user knows what prog
>> suppose to do. If prog is increasing the packet size the test writer
>> knows that and should size the output buffer accordingly.
> 
> I guess this is a matter of perspective. I prefer an interface that
> gives me back
> an error message, rather than corrupt my stack / heap, when the assumptions
> change. It's also nicer to use from "managed" languages like Go where users
> aren't as accustomed to issues like these.
> 
> Do you object to me adding the disclaimer to the header as Daniel suggested?

Agree that prog_test_run() in the current form is actually fine, I was mostly
thinking that it may be non-obvious to users extending the tests or writing
their own test framework and checking how BPF kselftests does it (since it's
kind of a prime example), so they might end up using the wrong API and run
into mentioned issues w/o realizing. At min some comment with more context
would be needed.

>> Also there are plenty of tests where progs don't change the packet size
>> and any test with 'repeat > 1' should have the packet size
>> return to initial value. Like if the test is doing ipip encap
>> at the end of the run the bpf prog should remove that encap.
>> Otherwise 'repeat > 1' will produce wrong results.
> 
> Yup. Another thorny part of the test interface, which I'd like to improve! :)
> Don't know how to do it yet though.

Yep, somehow here we would need to restore original input packet layout/data
so that the test program always runs on the user defined one.