Re: [PATCH bpf 1/3] bpf: tcp_read_skb needs to pop skb regardless of seq

[John Fastabend <john.fastabend@gmail.com>]

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@gmail.com>
> > ---
> >  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
> > 
> >