netdev - Re: [PATCH RFC v3 0/8] xdp: Infrastructure to generalize XDP

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: <20170221150907.5ae68ffa@cakuba.netronome.com>
Date:   Tue, 21 Feb 2017 15:09:07 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     Tom Herbert <tom@...bertland.com>
Cc:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <kernel-team@...com>
Subject: Re: [PATCH RFC v3 0/8] xdp: Infrastructure to generalize XDP

Hi, 

I still have a few worries about this patch set...

On Tue, 21 Feb 2017 11:34:09 -0800, Tom Herbert wrote:
> This patch set generalizes XDP by making the hooks in drivers to be
> generic. This has a number of advantages:
> 
>   - Allows a means to pipeline XDP programs together

Does it add anything beyond what we already can achieve with tail
calls?  Is there expectations that users will install independent
black box/pre-compiled programs on the same interface?

>   - Reduces the amount of code and complexity needed in drivers to
>     manage XDP

Other than for Mellanox drivers the savings are in *teen lines of code?

>   - Provides a more structured environment that is extensible to new
>     features while being mostly transparent to the drivers

So far all features we added required explicit driver support.
Checksumming support as an example will require driver changes, too.
Generalized way to call programs is probably not going to buy us much?

>   - Allow XDP programs to be set per device or per queue

Some drivers already provide that even though we don't have a user API
to set it.

>   - Moves management of BPF programs out of driver into a common
>     infrastructure

I appreciate this one, but IMHO this generalized infrastructure is way
too complicated for the purpose.  Why not simply place an XDP program
pointer into netdev and napi and provide driver with a helper to install
the program there?  Or am I simply not understanding the benefits of
the generalized hooks?

I probably have a slightly unique perspective caring about offloads but
IMHO the cost of this patchset outweighs the benefits right now.  You
haven't actually implement the OFFLOAD command at all (grep for
XDP_OFFLOAD_BPF) but having to deal with the intermediary layer will
certainly complicate things here.  

There are also miscellaneous optimizations which are possible when
drivers can inspect the program, like only reserving headroom if
program may use header adjust...  it does seem to me that this set
introduces a lot of complexity and only superficial driver
simplifications.  I would personally rather wait with infrastructure
like this until John's TX-to-other netdev is well formulated.

Obviously this set will also be a major pain when backporting things,
too.