netdev - Re: [PATCH bpf-next v2 00/11] Extend SOCKMAP to store listening sockets

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]

Date:   Fri, 10 Jan 2020 16:18:11 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        kernel-team@...udflare.com, Eric Dumazet <edumazet@...gle.com>,
        John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v2 00/11] Extend SOCKMAP to store listening
 sockets

On Fri, Jan 10, 2020 at 11:50:16AM +0100, Jakub Sitnicki wrote:
> With the realization that properly cloning listening sockets that have
> psock state/callbacks is tricky, comes the second version of patches.
> 
> The spirit of the patch set stays the same - make SOCKMAP a generic
> collection for listening and established sockets. This would let us use the
> SOCKMAP with reuseport today, and in the future hopefully with BPF programs
> that run at socket lookup time [0]. For a bit more context, please see v1
> cover letter [1].
> 
> The biggest change that happened since v1 is how we deal with clearing
> psock state in a copy of parent socket when cloning it (patches 3 & 4).
> 
> As much as I did not want to touch icsk/tcp clone path, it seems
> unavoidable. The changes were kept down to a minimum, with attention to not
> break existing users. That said, a review from the TCP maintainer would be
> invaluable (patches 3 & 4).
> 
> Patches 1 & 2 will conflict with recently posted "Fixes for sockmap/tls
> from more complex BPF progs" series [0]. I'll adapt or split them out this
> series once sockmap/tls fixes from John land in bpf-next branch.
> 
> Some food for thought - is mixing listening and established sockets in the
> same BPF map a good idea? I don't know but I couldn't find a good reason to
> restrict the user.
> 
> Considering how much the code evolved, I didn't carry over Acks from v1.
> 
> Thanks,
> jkbs
> 
> [0] https://lore.kernel.org/bpf/157851776348.1732.12600714815781177085.stgit@ubuntu3-kvm2/T/#t
> [1] https://lore.kernel.org/bpf/20191123110751.6729-1-jakub@cloudflare.com/
> 
> v1 -> v2:
> 
> - af_ops->syn_recv_sock callback is no longer overridden and burdened with
>   restoring sk_prot and clearing sk_user_data in the child socket. As child
>   socket is already hashed when syn_recv_sock returns, it is too late to
>   put it in the right state. Instead patches 3 & 4 restore sk_prot and
>   clear sk_user_data before we hash the child socket. (Pointed out by
>   Martin Lau)
> 
> - Annotate shared access to sk->sk_prot with READ_ONCE/WRITE_ONCE macros as
>   we write to it from sk_msg while socket might be getting cloned on
>   another CPU. (Suggested by John Fastabend)
> 
> - Convert tests for SOCKMAP holding listening sockets to return-on-error
>   style, and hook them up to test_progs. Also use BPF skeleton for setup.
>   Add new tests to cover the race scenario discovered during v1 review.

lgtm
Martin, John, please review.