netdev - Re: [PATCH bpf-next v2 0/9] bpf: sockmap, sockhash: support storing UDP 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 for Android: free password hash cracker in your pocket

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

Message-ID: <875zfndawf.fsf@cloudflare.com>
Date:   Sun, 01 Mar 2020 22:26:40 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Lorenz Bauer <lmb@...udflare.com>
Cc:     john.fastabend@...il.com, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        kernel-team@...udflare.com, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 0/9] bpf: sockmap, sockhash: support storing UDP sockets

On Fri, Feb 28, 2020 at 12:53 PM CET, Lorenz Bauer wrote:
> Thanks for all the reviews so far! I've fixed the identified bug and addressed
> feedback as much as possible.
>
> I've not taken up Jakub's suggestion to get rid of sk_psock_hooks_init. My
> intention is to encapsulate initializing both v4 and v6 protos. Really, it's
> down to personal preference, and I'm happy to remove it if others prefer
> Jakub's approach. I'm also still eager for a solution that requires less
> machinery.

I was going to do expenses but this seemed more fun. Challenge accepted.

I think we can massage tcp_bpf to extract sk_prot initialization bits
out of it into skmsg, and have skmsg call back into tcp/udp_bpf.

3 callbacks from skmsg back to tcp/udp_bpf would be needed to get there:

 - assert_proto_ops
 - check_v6_needs_rebuild
 - get_proto (akin to choose_proto in this series)

With that in place we could go for a direct dispatch based on sock type:

#define sk_psock_assert_proto_ops(sk, ops)		\
	(sk->sk_type == SOCK_STREAM			\
	 ? tcp_bpf_assert_proto_ops(ops)		\
	 : udp_bpf_assert_proto_ops(ops))

The steps to get there would be:

 1. extract tcp_bpf_get_proto
 2. fold tcp_bpf_update_sk_prot
 3. move tcp_bpf_init -> sk_psock_init_proto
 4. fold tcp_bpf_reinit_sk_prot
 5. move tcp_bpf_reinit -> sk_psock_reinit_proto
 6. add macros for callbacks into tcp_bpf
 7. add udp_bpf

... which I've given a shot at, mixing it into your patches:

  https://github.com/jsitnicki/linux/commits/extract-sk-psock-proto

Note, I didn't make an effort to share the code for rebuilding v6 proto
between tcp_bpf and udp_bpf. This construct seems hard to read already
as is without making it generic.

Final thought, I would probably place the bits common between tcp_bpf
and udp_bpf in skmsg under sk_psock_* namespace, instead of sockmap.

It is just my interpretation, but I think that was the idea outlined in
commit 604326b41a6f ("bpf, sockmap: convert to generic sk_msg
interface"):

    The code itself has been split and refactored into three bigger
    pieces: i) the generic sk_msg API which deals with managing the
    scatter gather ring, providing helpers for walking and mangling,
    transferring application data from user space into it, and preparing
    it for BPF pre/post-processing, ii) the plain sock map itself
    where sockets can be attached to or detached from; these bits
    are independent of i) which can now be used also without sock
    map, and iii) the integration with plain TCP as one protocol
    to be used for processing L7 application data (later this could
    e.g. also be extended to other protocols like UDP).

As skmsg already hosts some sk_psock_* functions used by tcp_bpf, and
they share the same build toggle NET_SK_MSG, that seems natural.

>
> Changes since v1:
> - Check newsk->sk_prot in tcp_bpf_clone
> - Fix compilation with BPF_STREAM_PARSER disabled
> - Use spin_lock_init instead of static initializer
> - Elaborate on TCPF_SYN_RECV
> - Cosmetic changes to TEST macros, and more tests
> - Add Jakub and me as maintainers
>
> Lorenz Bauer (9):
>   bpf: sockmap: only check ULP for TCP sockets
>   bpf: tcp: guard declarations with CONFIG_NET_SOCK_MSG
>   bpf: sockmap: move generic sockmap hooks from BPF TCP
>   skmsg: introduce sk_psock_hooks
>   bpf: sockmap: allow UDP sockets
>   selftests: bpf: don't listen() on UDP sockets
>   selftests: bpf: add tests for UDP sockets in sockmap
>   selftests: bpf: enable UDP sockmap reuseport tests
>   bpf, doc: update maintainers for L7 BPF
>
>  MAINTAINERS                                   |   3 +
>  include/linux/bpf.h                           |   4 +-
>  include/linux/skmsg.h                         |  72 +++----
>  include/linux/udp.h                           |   4 +
>  include/net/tcp.h                             |  18 +-
>  net/core/skmsg.c                              |  55 +++++
>  net/core/sock_map.c                           | 160 ++++++++++----
>  net/ipv4/Makefile                             |   1 +
>  net/ipv4/tcp_bpf.c                            | 169 +++------------
>  net/ipv4/udp_bpf.c                            |  53 +++++
>  .../bpf/prog_tests/select_reuseport.c         |   6 -
>  .../selftests/bpf/prog_tests/sockmap_listen.c | 204 +++++++++++++-----
>  12 files changed, 465 insertions(+), 284 deletions(-)
>  create mode 100644 net/ipv4/udp_bpf.c