Simon Horman wrote:
> 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.
Yeah I'll send a v2. Thanks.
>
> > 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
> >
> >
