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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <650cb45ba621d_d8cff208a2@john.notmuch>
Date: Thu, 21 Sep 2023 14:23:39 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Simon Horman <horms@...nel.org>, 
 John Fastabend <john.fastabend@...il.com>
Cc: daniel@...earbox.net, 
 ast@...nel.org, 
 andrii@...nel.org, 
 jakub@...udflare.com, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf 1/3] bpf: tcp_read_skb needs to pop skb regardless of
 seq

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@...il.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
> > 
> >