netdev - Re: [PATCH bpf-next v3 00/15] Introducing 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 May 2018 13:22:17 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Björn Töpel <bjorn.topel@...il.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Network Development <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        michael.lundkvist@...csson.com,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Singhai, Anjali" <anjali.singhai@...el.com>,
        "Zhang, Qi Z" <qi.z.zhang@...el.com>
Subject: Re: [PATCH bpf-next v3 00/15] Introducing AF_XDP support

On Fri, May 4, 2018 at 1:38 AM, Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
> On Fri, May 04, 2018 at 12:49:09AM +0200, Daniel Borkmann wrote:
>> On 05/02/2018 01:01 PM, Björn Töpel wrote:
>> > From: Björn Töpel <bjorn.topel@...el.com>
>> >
>> > This patch set introduces a new address family called AF_XDP that is
>> > optimized for high performance packet processing and, in upcoming
>> > patch sets, zero-copy semantics. In this patch set, we have removed
>> > all zero-copy related code in order to make it smaller, simpler and
>> > hopefully more review friendly. This patch set only supports copy-mode
>> > for the generic XDP path (XDP_SKB) for both RX and TX and copy-mode
>> > for RX using the XDP_DRV path. Zero-copy support requires XDP and
>> > driver changes that Jesper Dangaard Brouer is working on. Some of his
>> > work has already been accepted. We will publish our zero-copy support
>> > for RX and TX on top of his patch sets at a later point in time.
>>
>> +1, would be great to see it land this cycle. Saw few minor nits here
>> and there but nothing to hold it up, for the series:
>>
>> Acked-by: Daniel Borkmann <daniel@...earbox.net>
>>
>> Thanks everyone!
>
> Great stuff!
>
> Applied to bpf-next, with one condition.
> Upcoming zero-copy patches for both RX and TX need to be posted
> and reviewed within this release window.
> If netdev community as a whole won't be able to agree on the zero-copy
> bits we'd need to revert this feature before the next merge window.

Thanks everyone for reviewing this. Highly appreciated.

Just so we understand the purpose correctly:

1: Do you want to see the ZC patches in order to verify that the user
space API holds? If so, we can produce an additional RFC  patch set
using a big chunk of code that we had in RFC V1. We are not proud of
this code since it is clunky, but it hopefully proves the point with
the uapi being the same.

2: And/Or are you worried about us all (the netdev community) not
agreeing on a way to implement ZC internally in the drivers and the
XDP infrastructure? This is not going to be possible to finish during
this cycle since we do not like the implementation we had in RFC V1.
Too intrusive and now we also have nicer abstractions from Jesper that
we can use and extend to provide a (hopefully) much cleaner and less
intrusive solution.

Just so that we focus on the right proof points.

> Few other minor nits:
> patch 3:
> +struct xdp_ring {
> +       __u32 producer __attribute__((aligned(64)));
> +       __u32 consumer __attribute__((aligned(64)));
> +};
> It kinda begs for ____cacheline_aligned_in_smp to be introduced for uapi headers.

Agreed.

> patch 5:
> +struct sockaddr_xdp {
> +       __u16 sxdp_family;
> +       __u32 sxdp_ifindex;
> Not great to have a hole in uapi struct. Please fix it in the follow up.

You are correct. Will fix.

> patch 7:
> Has a lot of synchronize_net(). I think udpate/delete side
> can be improved to avoid them. Otherwise users may unknowingly DoS.

OK. Could you please elaborate on what kind of DoS attacks can be
performed with this, so we can come up with the right solution here?

Thanks: Magnus

> As the next steps I suggest to prioritize the highest to ship
> zero-copy rx/tx patches and to add selftests.
>
> Thanks!
>