On 11/26/2018 01:45 PM, Lorenz Bauer wrote: > On Sat, 24 Nov 2018 at 22:20, Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> 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.
