netdev - Re: [PATCH bpf-next v2 4/8] xsk: register XDP sockets at bind(), and add new AF

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: <36d92c18-e0a0-a4dc-3797-539f8fe6333c@intel.com>
Date:   Thu, 21 Jan 2021 09:18:48 +0100
From:   Björn Töpel <bjorn.topel@...el.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Björn Töpel <bjorn.topel@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>, maximmi@...dia.com,
        "David S. Miller" <davem@...emloft.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Ciara Loftus <ciara.loftus@...el.com>,
        weqaar.a.janjua@...el.com
Subject: Re: [PATCH bpf-next v2 4/8] xsk: register XDP sockets at bind(), and
 add new AF_XDP BPF helper

On 2021-01-20 22:15, Alexei Starovoitov wrote:
> On Wed, Jan 20, 2021 at 12:26 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> This argument, however, I buy: bpf_redirect() is the single-purpose
>> helper for redirecting to an ifindex, bpf_redirect_xsk() is the
>> single-purpose helper for redirecting to an XSK, and bpf_redirect_map()
>> is the generic one that does both of those and more. Fair enough,
>> consider me convinced :)
>>
>>> A lot of back-and-forth for *one* if-statement, but it's kind of a
>>> design thing for me. ;-)
>>
>> Surely you don't mean to imply that you have *better* things to do with
>> your time than have a 10-emails-long argument over a single if
>> statement? ;)
> 
> After reading this thread I think I have to pour cold water on the design.
> 
> The performance blip comes from hard coded assumptions:
> + queue_id = xdp->rxq->queue_index;
> + xs = READ_ONCE(dev->_rx[queue_id].xsk);
>

Yes, one can see this as a constrained map:

* The map belongs to a certain netdev.
* Each entry corresponds to a certain queue id.

I.e if we do a successful (non-NULL) lookup, we *know* that all sockets
in that map belong to the netdev, and has the correct queue id.

By doing that we can get rid of two run-time checks: "Is the socket
bound to this netdev?" and "Is this the correct queue id?".

> bpf can have specialized helpers, but imo this is beyond what's reasonable.
> Please move such things into the program and try to make
> bpf_redirect_map faster.
>

I obviously prefer this path, and ideally combined with a way to even
more specialize xdp_do_redirect(). Then again, you are the maintainer! :-)

Maybe an alternative be adding a new type of XSKMAP constrained in the
similar way as above, and continue with bpf_redirect_map(), but with
this new map the index argument would be ignored. Unfortunately the BPF
context (xdp_buff in this case) is not passed to bpf_redirect_map(), so
getting the actual queue_id in the helper is hard. Adding the context as
an additional argument would be a new helper...

I'll need to think a bit more about it. Input/ideas are welcome!

> Making af_xdp non-root is orthogonal. If there is actual need for that
> it has to be designed thoroughly and not presented as "this helper may
> help to do that".
> I don't think "may" will materialize unless people actually work
> toward the goal of non-root.
> 

Fair enough! Same goal could be reached using the existing map approach.

Björn