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: <CAJ+HfNim_4pZz4SvV06R5pZ0AffT=v7G6AqRNm=mz+OBgOpm7A@mail.gmail.com>
Date:   Fri, 8 May 2020 11:08:40 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.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?

On Thu, 7 May 2020 at 16:48, Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
[]
> 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.
>

Right, but it would be trivial to add for drivers that already support
XDP_REDIRECT, so I'm not worried about that particular problem. That
aside, let's move on. I agree that adding action should be avoided!

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

True. For the sake of the argument; Adding flags vs adding a new
helper, i.e. bpf_redirect_map(flag_with_new_semantic) vs a new helper.
Today XDP developers that use bpf_redirect_map() need to consider
whether the kernel support the "pass action via flags" or not, so this
would be a *fourth* option. :-P

I'm with you here. The best option would be a transparent one.

> 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 :)
>

Hear, hear!

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

Sure, but this wouldn't remove the per-cpu/bpf_redirect_info lookup.
Then again, maybe it's better to start there. To clarify, just a flag
isn't sufficient. It would need to be a guarantee that the program
exists after the call, i.e. tail call support. From clang/BPF
instruction (Alexei's/John's replies), or something like
bpf_tail_call(). Unless I'm missing something... Or do you mean that
the flag IMMEDIATE_RETURN would perform the action in the helper? The
context would be stale after that call, and the verifier should reject
a program that pokes the context, but the flag is a runtime thing. It
sounds like a pretty complex verifier task to determine if
IMMEDIATE_RETURN is set, and then reject ctx accesses there.

Thanks for the input, and good ideas!
Björn

> -Toke
>