On Wed, Sep 20, 2023 at 04:27:04PM -0700, John Fastabend wrote:
> Before fix e5c6de5fa0258 tcp_read_skb() would increment the tp->copied-seq
> value. This (as described in the commit) would cause an error for apps
> because once that is incremented the application might believe there is no
> data to be read. Then some apps would stall or abort believing no data is
> available.
>
> However, the fix is incomplete because it introduces another issue in
> the skb dequeue. The loop does tcp_recv_skb() in a while loop to consume
> as many skbs as possible. The problem is the call is,
>
> tcp_recv_skb(sk, seq, &offset)
>
> Where 'seq' is
>
> u32 seq = tp->copied_seq;
>
> Now we can hit a case where we've yet incremented copied_seq from BPF side,
> but then tcp_recv_skb() fails this test,
>
> if (offset < skb->len || (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN))
>
> so that instead of returning the skb we call tcp_eat_recv_skb() which frees
> the skb. This is because the routine believes the SKB has been collapsed
> per comment,
>
> /* This looks weird, but this can happen if TCP collapsing
> * splitted a fat GRO packet, while we released socket lock
> * in skb_splice_bits()
> */
>
> This can't happen here we've unlinked the full SKB and orphaned it. Anyways
> it would confuse any BPF programs if the data were suddenly moved underneath
> it.
>
> To fix this situation do simpler operation and just skb_peek() the data
> of the queue followed by the unlink. It shouldn't need to check this
> condition and tcp_read_skb() reads entire skbs so there is no need to
> handle the 'offset!=0' case as we would see in tcp_read_sock().
>
> Fixes: e5c6de5fa0258 ("bpf, sockmap: Incorrectly handling copied_seq")
> Fixes: 04919bed948dc ("tcp: Introduce tcp_read_skb()")
> Signed-off-by: John Fastabend <john.fastabend@xxxxxxxxx>
> ---
> net/ipv4/tcp.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 0c3040a63ebd..45e7f39e67bc 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1625,12 +1625,11 @@ int tcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
> u32 seq = tp->copied_seq;
Hi John,
according to clang-16, with this change seq is now set but unused.
I guess seq can simply be removed as part of this change.
> struct sk_buff *skb;
> int copied = 0;
> - u32 offset;
>
> if (sk->sk_state == TCP_LISTEN)
> return -ENOTCONN;
>
> - while ((skb = tcp_recv_skb(sk, seq, &offset)) != NULL) {
> + while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
> u8 tcp_flags;
> int used;
>
> --
> 2.33.0
>
>
