Message ID | 4AE92F4D.6070101@gmail.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 29 Oct 2009 06:59:41 +0100 > David, what do you think of following patch ? > > I wonder if we should reorganize code to add sanity checks in tcp_unlink_write_queue() > that the skb we delete from queue is not still referenced. > > [PATCH] tcp: clear retrans hints in tcp_send_synack() > > There is a small possibility the skb we unlink from write queue > is still referenced by retrans hints. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Yes, the first thing I thought of when I saw this crash was the hints. I'll think this over. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 29 Oct 2009 06:59:41 +0100 > [PATCH] tcp: clear retrans hints in tcp_send_synack() > > There is a small possibility the skb we unlink from write queue > is still referenced by retrans hints. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> So, this would only be true if we were dealing with a data packet here. We're not, this is a SYN+ACK which happens to be cloned in the write queue. The hint SKBs pointers can only point to real data packets. And we're only dealing with data packets once we enter established state, and when we enter established by definition we have unlinked and freed up any SYN and SYN+ACK SKBs in the write queue. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index fcd278a..b22a72d 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -2201,6 +2201,7 @@ int tcp_send_synack(struct sock *sk) struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC); if (nskb == NULL) return -ENOMEM; + tcp_clear_all_retrans_hints(tcp_sk(sk)); tcp_unlink_write_queue(skb, sk); skb_header_release(nskb); __tcp_add_write_queue_head(sk, nskb);