lkml.org 
[lkml]   [2021]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.15 822/917] bpf, sockmap: Fix race in ingress receive verdict with redirect to self
    Date
    From: John Fastabend <john.fastabend@gmail.com>

    [ Upstream commit c5d2177a72a1659554922728fc407f59950aa929 ]

    A socket in a sockmap may have different combinations of programs attached
    depending on configuration. There can be no programs in which case the socket
    acts as a sink only. There can be a TX program in this case a BPF program is
    attached to sending side, but no RX program is attached. There can be an RX
    program only where sends have no BPF program attached, but receives are hooked
    with BPF. And finally, both TX and RX programs may be attached. Giving us the
    permutations:

    None, Tx, Rx, and TxRx

    To date most of our use cases have been TX case being used as a fast datapath
    to directly copy between local application and a userspace proxy. Or Rx cases
    and TxRX applications that are operating an in kernel based proxy. The traffic
    in the first case where we hook applications into a userspace application looks
    like this:

    AppA redirect AppB
    Tx <-----------> Rx
    | |
    + +
    TCP <--> lo <--> TCP

    In this case all traffic from AppA (after 3whs) is copied into the AppB
    ingress queue and no traffic is ever on the TCP recieive_queue.

    In the second case the application never receives, except in some rare error
    cases, traffic on the actual user space socket. Instead the send happens in
    the kernel.

    AppProxy socket pool
    sk0 ------------->{sk1,sk2, skn}
    ^ |
    | |
    | v
    ingress lb egress
    TCP TCP

    Here because traffic is never read off the socket with userspace recv() APIs
    there is only ever one reader on the sk receive_queue. Namely the BPF programs.

    However, we've started to introduce a third configuration where the BPF program
    on receive should process the data, but then the normal case is to push the
    data into the receive queue of AppB.

    AppB
    recv() (userspace)
    -----------------------
    tcp_bpf_recvmsg() (kernel)
    | |
    | |
    | |
    ingress_msgQ |
    | |
    RX_BPF |
    | |
    v v
    sk->receive_queue

    This is different from the App{A,B} redirect because traffic is first received
    on the sk->receive_queue.

    Now for the issue. The tcp_bpf_recvmsg() handler first checks the ingress_msg
    queue for any data handled by the BPF rx program and returned with PASS code
    so that it was enqueued on the ingress msg queue. Then if no data exists on
    that queue it checks the socket receive queue. Unfortunately, this is the same
    receive_queue the BPF program is reading data off of. So we get a race. Its
    possible for the recvmsg() hook to pull data off the receive_queue before the
    BPF hook has a chance to read it. It typically happens when an application is
    banging on recv() and getting EAGAINs. Until they manage to race with the RX
    BPF program.

    To fix this we note that before this patch at attach time when the socket is
    loaded into the map we check if it needs a TX program or just the base set of
    proto bpf hooks. Then it uses the above general RX hook regardless of if we
    have a BPF program attached at rx or not. This patch now extends this check to
    handle all cases enumerated above, TX, RX, TXRX, and none. And to fix above
    race when an RX program is attached we use a new hook that is nearly identical
    to the old one except now we do not let the recv() call skip the RX BPF program.
    Now only the BPF program pulls data from sk->receive_queue and recv() only
    pulls data from the ingress msgQ post BPF program handling.

    With this resolved our AppB from above has been up and running for many hours
    without detecting any errors. We do this by correlating counters in RX BPF
    events and the AppB to ensure data is never skipping the BPF program. Selftests,
    was not able to detect this because we only run them for a short period of time
    on well ordered send/recvs so we don't get any of the noise we see in real
    application environments.

    Fixes: 51199405f9672 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
    Signed-off-by: John Fastabend <john.fastabend@gmail.com>
    Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
    Tested-by: Jussi Maki <joamaki@gmail.com>
    Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
    Link: https://lore.kernel.org/bpf/20211103204736.248403-4-john.fastabend@gmail.com
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    net/ipv4/tcp_bpf.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 47 insertions(+)

    diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
    index 246f725b78c9b..f70aa0932bd6c 100644
    --- a/net/ipv4/tcp_bpf.c
    +++ b/net/ipv4/tcp_bpf.c
    @@ -172,6 +172,41 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
    return ret;
    }

    +static int tcp_bpf_recvmsg_parser(struct sock *sk,
    + struct msghdr *msg,
    + size_t len,
    + int nonblock,
    + int flags,
    + int *addr_len)
    +{
    + struct sk_psock *psock;
    + int copied;
    +
    + if (unlikely(flags & MSG_ERRQUEUE))
    + return inet_recv_error(sk, msg, len, addr_len);
    +
    + psock = sk_psock_get(sk);
    + if (unlikely(!psock))
    + return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
    +
    + lock_sock(sk);
    +msg_bytes_ready:
    + copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
    + if (!copied) {
    + long timeo;
    + int data;
    +
    + timeo = sock_rcvtimeo(sk, nonblock);
    + data = tcp_msg_wait_data(sk, psock, timeo);
    + if (data && !sk_psock_queue_empty(psock))
    + goto msg_bytes_ready;
    + copied = -EAGAIN;
    + }
    + release_sock(sk);
    + sk_psock_put(sk, psock);
    + return copied;
    +}
    +
    static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
    int nonblock, int flags, int *addr_len)
    {
    @@ -464,6 +499,8 @@ enum {
    enum {
    TCP_BPF_BASE,
    TCP_BPF_TX,
    + TCP_BPF_RX,
    + TCP_BPF_TXRX,
    TCP_BPF_NUM_CFGS,
    };

    @@ -482,6 +519,12 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
    prot[TCP_BPF_TX] = prot[TCP_BPF_BASE];
    prot[TCP_BPF_TX].sendmsg = tcp_bpf_sendmsg;
    prot[TCP_BPF_TX].sendpage = tcp_bpf_sendpage;
    +
    + prot[TCP_BPF_RX] = prot[TCP_BPF_BASE];
    + prot[TCP_BPF_RX].recvmsg = tcp_bpf_recvmsg_parser;
    +
    + prot[TCP_BPF_TXRX] = prot[TCP_BPF_TX];
    + prot[TCP_BPF_TXRX].recvmsg = tcp_bpf_recvmsg_parser;
    }

    static void tcp_bpf_check_v6_needs_rebuild(struct proto *ops)
    @@ -519,6 +562,10 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
    int family = sk->sk_family == AF_INET6 ? TCP_BPF_IPV6 : TCP_BPF_IPV4;
    int config = psock->progs.msg_parser ? TCP_BPF_TX : TCP_BPF_BASE;

    + if (psock->progs.stream_verdict || psock->progs.skb_verdict) {
    + config = (config == TCP_BPF_TX) ? TCP_BPF_TXRX : TCP_BPF_RX;
    + }
    +
    if (restore) {
    if (inet_csk_has_ulp(sk)) {
    /* TLS does not have an unhash proto in SW cases,
    --
    2.33.0


    \
     
     \ /
      Last update: 2021-11-16 01:40    [W:4.032 / U:1.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site