netdev - Re: XDP bpf_tail_call_redirect(): yea or nay?

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: <871rnvkdhw.fsf@toke.dk>
Date:   Thu, 07 May 2020 16:48:11 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Björn Töpel <bjorn.topel@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Netdev <netdev@...r.kernel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: XDP bpf_tail_call_redirect(): yea or nay?

Björn Töpel <bjorn.topel@...il.com> writes:

> On Thu, 7 May 2020 at 15:44, Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> Björn Töpel <bjorn.topel@...il.com> writes:
>>
>> > Before I start hacking on this, I might as well check with the XDP
>> > folks if this considered a crappy idea or not. :-)
>> >
>> > The XDP redirect flow for a packet is typical a dance of
>> > bpf_redirect_map() that updates the bpf_redirect_info structure with
>> > maps type/items, which is then followed by an xdp_do_redirect(). That
>> > function takes an action based on the bpf_redirect_info content.
>> >
>> > I'd like to get rid of the xdp_do_redirect() call, and the
>> > bpf_redirect_info (per-cpu) lookup. The idea is to introduce a new
>> > (oh-no!) XDP action, say, XDP_CONSUMED and a built-in helper with
>> > tail-call semantics.
>> >
>> > Something across the lines of:
>> >
>> > --8<--
>> >
>> > struct {
>> >         __uint(type, BPF_MAP_TYPE_XSKMAP);
>> >         __uint(max_entries, MAX_SOCKS);
>> >         __uint(key_size, sizeof(int));
>> >         __uint(value_size, sizeof(int));
>> > } xsks_map SEC(".maps");
>> >
>> > SEC("xdp1")
>> > int xdp_prog1(struct xdp_md *ctx)
>> > {
>> >         bpf_tail_call_redirect(ctx, &xsks_map, 0);
>> >         // Redirect the packet to an AF_XDP socket at entry 0 of the
>> >         // map.
>> >         //
>> >         // After a successful call, ctx is said to be
>> >         // consumed. XDP_CONSUMED will be returned by the program.
>> >         // Note that if the call is not successful, the buffer is
>> >         // still valid.
>> >         //
>> >         // XDP_CONSUMED in the driver means that the driver should not
>> >         // issue an xdp_do_direct() call, but only xdp_flush().
>> >         //
>> >         // The verifier need to be taught that XDP_CONSUMED can only
>> >         // be returned "indirectly", meaning a bpf_tail_call_XXX()
>> >         // call. An explicit "return XDP_CONSUMED" should be
>> >         // rejected. Can that be implemented?
>> >         return XDP_PASS; // or any other valid action.
>> > }
>> >
>> > -->8--
>> >
>> > The bpf_tail_call_redirect() would work with all redirectable maps.
>> >
>> > Thoughts? Tomatoes? Pitchforks?
>>
>> The above answers the 'what'. Might be easier to evaluate if you also
>> included the 'why'? :)
>>
>
> Ah! Sorry! Performance, performance, performance. Getting rid of a
> bunch of calls/instructions per packet, which helps my (AF_XDP) case.
> This would be faster than the regular REDIRECT path. Today, in
> bpf_redirect_map(), instead of actually performing the action, we
> populate the bpf_redirect_info structure, just to look up the action
> again in xdp_do_redirect().
>
> I'm pretty certain this would be a gain for AF_XDP (quite easy to do a
> quick hack, and measure). It would also shave off the same amount of
> instructions for "vanilla" XDP_REDIRECT cases. The bigger issue; Is
> this new semantic something people would be comfortable being added to
> XDP.

Well, my immediate thought would be that the added complexity would not
be worth it, because:

- A new action would mean either you'd need to patch all drivers or
  (more likely) we'd end up with yet another difference between drivers'
  XDP support.

- BPF developers would suddenly have to choose - do this new faster
  thing, or be compatible? And manage the choice based on drivers they
  expect to run on, etc. This was already confusing with
  bpf_redirect()/bpf_redirect_map(), and this would introduce a third
  option!

So in light of this, I'd say the performance benefit would have to be
quite substantial for this to be worth it. Which we won't know until you
try it, I guess :)

Thinking of alternatives - couldn't you shoe-horn this into the existing
helper and return code? Say, introduce an IMMEDIATE_RETURN flag to the
existing helpers, which would change the behaviour to the tail call
semantics. When used, xdp_do_redirect() would then return immediately
(or you could even turn xdp_do_redirect() into an inlined wrapper that
checks the flag before issuing a CALL to the existing function). Any
reason why that wouldn't work?

-Toke