All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Sitnicki <jakub@cloudflare.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: daniel@iogearbox.net, bpf@vger.kernel.org,
	netdev@vger.kernel.org, edumazet@google.com, ast@kernel.org,
	andrii@kernel.org, will@isovalent.com
Subject: Re: [PATCH bpf v10 08/14] bpf: sockmap, incorrectly handling copied_seq
Date: Tue, 23 May 2023 11:09:45 +0200	[thread overview]
Message-ID: <87cz2r76rs.fsf@cloudflare.com> (raw)
In-Reply-To: <20230523025618.113937-9-john.fastabend@gmail.com>

On Mon, May 22, 2023 at 07:56 PM -07, John Fastabend wrote:
> The read_skb() logic is incrementing the tcp->copied_seq which is used for
> among other things calculating how many outstanding bytes can be read by
> the application. This results in application errors, if the application
> does an ioctl(FIONREAD) we return zero because this is calculated from
> the copied_seq value.
>
> To fix this we move tcp->copied_seq accounting into the recv handler so
> that we update these when the recvmsg() hook is called and data is in
> fact copied into user buffers. This gives an accurate FIONREAD value
> as expected and improves ACK handling. Before we were calling the
> tcp_rcv_space_adjust() which would update 'number of bytes copied to
> user in last RTT' which is wrong for programs returning SK_PASS. The
> bytes are only copied to the user when recvmsg is handled.
>
> Doing the fix for recvmsg is straightforward, but fixing redirect and
> SK_DROP pkts is a bit tricker. Build a tcp_psock_eat() helper and then
> call this from skmsg handlers. This fixes another issue where a broken
> socket with a BPF program doing a resubmit could hang the receiver. This
> happened because although read_skb() consumed the skb through sock_drop()
> it did not update the copied_seq. Now if a single reccv socket is
> redirecting to many sockets (for example for lb) the receiver sk will be
> hung even though we might expect it to continue. The hang comes from
> not updating the copied_seq numbers and memory pressure resulting from
> that.
>
> We have a slight layer problem of calling tcp_eat_skb even if its not
> a TCP socket. To fix we could refactor and create per type receiver
> handlers. I decided this is more work than we want in the fix and we
> already have some small tweaks depending on caller that use the
> helper skb_bpf_strparser(). So we extend that a bit and always set
> the strparser bit when it is in use and then we can gate the
> seq_copied updates on this.
>
> Fixes: 04919bed948dc ("tcp: Introduce tcp_read_skb()")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---

Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>

  reply	other threads:[~2023-05-23  9:10 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-23  2:56 [PATCH bpf v10 00/14] bpf sockmap fixes John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 01/14] bpf: sockmap, pass skb ownership through read_skb John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 02/14] bpf: sockmap, convert schedule_work into delayed_work John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 03/14] bpf: sockmap, reschedule is now done through backlog John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 04/14] bpf: sockmap, improved check for empty queue John Fastabend
2023-05-23  7:35   ` Jakub Sitnicki
2023-05-23  2:56 ` [PATCH bpf v10 05/14] bpf: sockmap, handle fin correctly John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 06/14] bpf: sockmap, TCP data stall on recv before accept John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 07/14] bpf: sockmap, wake up polling after data copy John Fastabend
2023-05-30  6:30   ` Eric Dumazet
2023-05-30 18:34     ` John Fastabend
2023-05-30 18:43       ` John Fastabend
2023-05-30 18:51         ` Eric Dumazet
2023-05-23  2:56 ` [PATCH bpf v10 08/14] bpf: sockmap, incorrectly handling copied_seq John Fastabend
2023-05-23  9:09   ` Jakub Sitnicki [this message]
2023-05-23  2:56 ` [PATCH bpf v10 09/14] bpf: sockmap, pull socket helpers out of listen test for general use John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 10/14] bpf: sockmap, build helper to create connected socket pair John Fastabend
2023-05-23  9:23   ` Jakub Sitnicki
2023-05-23  2:56 ` [PATCH bpf v10 11/14] bpf: sockmap, test shutdown() correctly exits epoll and recv()=0 John Fastabend
2023-05-23  9:41   ` Jakub Sitnicki
2023-05-23  2:56 ` [PATCH bpf v10 12/14] bpf: sockmap, test FIONREAD returns correct bytes in rx buffer John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 13/14] bpf: sockmap, test FIONREAD returns correct bytes in rx buffer with drops John Fastabend
2023-05-23  2:56 ` [PATCH bpf v10 14/14] bpf: sockmap, test progs verifier error with latest clang John Fastabend
2023-05-23 10:00   ` Jakub Sitnicki
2023-05-23 14:31 ` [PATCH bpf v10 00/14] bpf sockmap fixes Daniel Borkmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87cz2r76rs.fsf@cloudflare.com \
    --to=jakub@cloudflare.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=edumazet@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=will@isovalent.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.