Re: [PATCH bpf-next v3 6/8] bpf: Add XDP_REDIRECT support to XDP for bpf_prog_run()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Sat, Dec 11, 2021 at 10:43 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> +
>> +static void bpf_test_run_xdp_teardown(struct bpf_test_timer *t)
>> +{
>> +       struct xdp_mem_info mem = {
>> +               .id = t->xdp.pp->xdp_mem_id,
>> +               .type = MEM_TYPE_PAGE_POOL,
>> +       };
>
> pls add a new line.
>
>> +       xdp_unreg_mem_model(&mem);
>> +}
>> +
>> +static bool ctx_was_changed(struct xdp_page_head *head)
>> +{
>> +       return (head->orig_ctx.data != head->ctx.data ||
>> +               head->orig_ctx.data_meta != head->ctx.data_meta ||
>> +               head->orig_ctx.data_end != head->ctx.data_end);
>
> redundant ()
>
>>         bpf_test_timer_enter(&t);
>>         old_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>>         do {
>>                 run_ctx.prog_item = &item;
>> -               if (xdp)
>> +               if (xdp && xdp_redirect) {
>> +                       ret = bpf_test_run_xdp_redirect(&t, prog, ctx);
>> +                       if (unlikely(ret < 0))
>> +                               break;
>> +                       *retval = ret;
>> +               } else if (xdp) {
>>                         *retval = bpf_prog_run_xdp(prog, ctx);
>
> Can we do this unconditionally without introducing a new uapi flag?
> I mean "return bpf_redirect()" was a nop under test_run.
> What kind of tests might break if it stops being a nop?

Well, I view the existing mode of bpf_prog_test_run() with XDP as a way
to write XDP unit tests: it allows you to submit a packet, run your XDP
program on it, and check that it returned the right value and did the
right modifications. This means if you XDP program does 'return
bpf_redirect()', userspace will still get the XDP_REDIRECT value and so
it can check correctness of your XDP program.

With this flag the behaviour changes quite drastically, in that it will
actually put packets on the wire instead of getting back the program
return. So I think it makes more sense to make it a separate opt-in
mode; the old behaviour can still be useful for checking XDP program
behaviour.

-Toke