Date:   Sat, 11 Jan 2020 14:47:12 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>, bpf@...r.kernel.org
Cc:     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

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.

Thanks I just posted a v2 of that series so once that lands we will need
to respin this series.

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

+1 in general I've been trying to avoid adding arbitrary restriction.
In this case I agree I can't think of a good reason to do it for my use
cases but lets not stop someone from doing it if their use case wants to
for some reason.

> 
> Considering how much the code evolved, I didn't carry over Acks from v1.

Sounds good thanks for keeping this series going.

> 
> 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.
> 
> RFC -> v1:
> 
> - Switch from overriding proto->accept to af_ops->syn_recv_sock, which
>   happens earlier. Clearing the psock state after accept() does not work
>   for child sockets that become orphaned (never got accepted). v4-mapped
>   sockets need special care.
> 
> - Return the socket cookie on SOCKMAP lookup from syscall to be on par with
>   REUSEPORT_SOCKARRAY. Requires SOCKMAP to take u64 on lookup/update from
>   syscall.
> 
> - Make bpf_sk_redirect_map (ingress) and bpf_msg_redirect_map (egress)
>   SOCKMAP helpers fail when target socket is a listening one.
> 
> - Make bpf_sk_select_reuseport helper fail when target is a TCP established
>   socket.
> 
> - Teach libbpf to recognize SK_REUSEPORT program type from section name.
> 
> - Add a dedicated set of tests for SOCKMAP holding listening sockets,
>   covering map operations, overridden socket callbacks, and BPF helpers.
> 
> 
> Jakub Sitnicki (11):
>   bpf, sk_msg: Don't reset saved sock proto on restore
>   net, sk_msg: Annotate lockless access to sk_prot on clone
>   net, sk_msg: Clear sk_user_data pointer on clone if tagged
>   tcp_bpf: Don't let child socket inherit parent protocol ops on copy
>   bpf, sockmap: Allow inserting listening TCP sockets into sockmap
>   bpf, sockmap: Don't set up sockmap progs for listening sockets
>   bpf, sockmap: Return socket cookie on lookup from syscall
>   bpf, sockmap: Let all kernel-land lookup values in SOCKMAP
>   bpf: Allow selecting reuseport socket from a SOCKMAP
>   selftests/bpf: Extend SK_REUSEPORT tests to cover SOCKMAP
>   selftests/bpf: Tests for SOCKMAP holding listening sockets
> 
>  include/linux/skmsg.h                         |   14 +-
>  include/net/sock.h                            |   37 +-
>  include/net/tcp.h                             |    1 +
>  kernel/bpf/verifier.c                         |    6 +-
>  net/core/filter.c                             |   15 +-
>  net/core/skmsg.c                              |    2 +-
>  net/core/sock.c                               |   11 +-
>  net/core/sock_map.c                           |  120 +-
>  net/ipv4/tcp_bpf.c                            |   19 +-
>  net/ipv4/tcp_minisocks.c                      |    2 +
>  net/ipv4/tcp_ulp.c                            |    2 +-
>  net/tls/tls_main.c                            |    2 +-
>  .../bpf/prog_tests/select_reuseport.c         |   60 +-
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 1378 +++++++++++++++++
>  .../selftests/bpf/progs/test_sockmap_listen.c |   76 +
>  tools/testing/selftests/bpf/test_maps.c       |    6 +-
>  16 files changed, 1696 insertions(+), 55 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> 
> -- 
> 2.24.1
> 

Powered by blists - more mailing lists