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
| ||
|
Message-ID: <CANn89i+z8mOB+YAwvKN=0EwLN-gQDKit8WA9SbeCyxdACB_O_Q@mail.gmail.com> Date: Wed, 11 Dec 2019 11:39:23 -0800 From: Eric Dumazet <edumazet@...gle.com> To: Neal Cardwell <ncardwell@...gle.com> Cc: "David S . Miller" <davem@...emloft.net>, netdev <netdev@...r.kernel.org>, Eric Dumazet <eric.dumazet@...il.com>, Christoph Paasch <cpaasch@...le.com>, Jason Baron <jbaron@...mai.com>, Yuchung Cheng <ycheng@...gle.com>, Soheil Hassas Yeganeh <soheil@...gle.com> Subject: Re: [PATCH net] tcp: do not send empty skb from tcp_write_xmit() On Wed, Dec 11, 2019 at 11:17 AM Neal Cardwell <ncardwell@...gle.com> wrote: > > On Wed, Dec 11, 2019 at 2:34 AM Eric Dumazet <edumazet@...gle.com> wrote: > > > > Backport of commit fdfc5c8594c2 ("tcp: remove empty skb from > > write queue in error cases") in linux-4.14 stable triggered > > various bugs. One of them has been fixed in commit ba2ddb43f270 > > ("tcp: Don't dequeue SYN/FIN-segments from write-queue"), but > > we still have crashes in some occasions. > > > > Root-cause is that when tcp_sendmsg() has allocated a fresh > > skb and could not append a fragment before being blocked > > in sk_stream_wait_memory(), tcp_write_xmit() might be called > > and decide to send this fresh and empty skb. > > > > Sending an empty packet is not only silly, it might have caused > > many issues we had in the past with tp->packets_out being > > out of sync. > > > > Fixes: c65f7f00c587 ("[TCP]: Simplify SKB data portion allocation with NETIF_F_SG.") > > Signed-off-by: Eric Dumazet <edumazet@...gle.com> > > Cc: Christoph Paasch <cpaasch@...le.com> > > Cc: Neal Cardwell <ncardwell@...gle.com> > > Cc: Jason Baron <jbaron@...mai.com> > > --- > > net/ipv4/tcp_output.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > > index b184f03d743715ef4b2d166ceae651529be77953..57f434a8e41ffd6bc584cb4d9e87703491a378c1 100644 > > --- a/net/ipv4/tcp_output.c > > +++ b/net/ipv4/tcp_output.c > > @@ -2438,6 +2438,14 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle, > > if (tcp_small_queue_check(sk, skb, 0)) > > break; > > > > + /* Argh, we hit an empty skb(), presumably a thread > > + * is sleeping in sendmsg()/sk_stream_wait_memory(). > > + * We do not want to send a pure-ack packet and have > > + * a strange looking rtx queue with empty packet(s). > > + */ > > + if (TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq) > > + break; > > + > > if (unlikely(tcp_transmit_skb(sk, skb, 1, gfp))) > > break; > > > > -- > > Thanks for the fix, Eric! > > Is there any risk that any current or future bugs that create > persistently empty skbs could cause the connection to "freeze", unable > to reach the tcp_transmit_skb() call in tcp_write_xmit()? > > To avoid this risk, would it make sense to delete the empty skb and > continue the tcp_write_xmit() transmit loop, rather than breaking out > of the loop? This 'empty' skb must be the last in the queue. Removing it from the queue would not prevent tcp_write_xmit() from breaking the loop. If we remove it, then we force sendmsg() to re-allocate a fresh skb, which seems more work, and would paper around the bugs that would be un-noticed. Another question to ask is if we need to reconsider how we use tcp_write_queue_empty() as an indicator for 'I have something in the write queue' This is obviously wrong if the write queue has a single empty skb. Maybe we should instead use tp->write_seq != tp->snd_nxt
Powered by blists - more mailing lists