Re: [BUG] null pointer dereference in tcp_gso_segment()

[Willy Tarreau <w@1wt.eu>]

On Wed, Jan 22, 2014 at 02:18:45PM -0800, Eric Dumazet wrote:
> On Wed, 2014-01-22 at 23:02 +0100, Arnaud Ebalard wrote:
> > Hi Eric,
> > 
> > Eric Dumazet <eric.dumazet@gmail.com> writes:
> > 
> > >> Unless there is an assumption I missed somewhere in the function, the
> > >> problem may occur during the first round of the loop, because (unlike
> > >> the 'while' condition does at line 21) skb->next is not checked against
> > >> null at lines 17 above before it is passed to tcp_hdr() at line 18.
> > >> 
> > >> To be honest, I am asking because I am not familiar w/ the code and it
> > >> is somewhat old so I wonder why noone got hit before. AFAICT,
> > >> f4c50d990dcf ([NET]: Add software TSOv4) added TSOv4 support in 2006 via
> > >> introduction of tcp_tso_segmen() (with the same kind of deref but
> > >> possibly different assumptions) which was more recently modified via
> > >> 28850dc7c7 (net: tcp: move GRO/GSO functions to tcp_offload) to become
> > >> tcp_gso_segment().
> > >> 
> > >> David, can you confirm the analysis and possibly comment on the
> > >> conditions needed for the bug to manifest?
> > >
> > > A gso packet contains at least 2 segments.
> > 
> > By whom / where is it enforced?
> 
> For example, tcp_gso_segment() does the following check :
> 
> if (unlikely(skb->len <= mss))
> 	goto out;
> 
> If there was one segment, then skb->len should also be smaller than mss
> 
> Since TCP stack seemed to be the provider of the packet in your stack
> trace, check tcp_set_skb_tso_segs()

Thanks Eric for the explanation. From Arnaud's trace, I suspect that he's
received an ACK which has released some pending data, so it's very likely
indeed that at least two segments were released at once given that the
receiver is likely to ACK every two segments.

Also we can expect that the received ACK was copy-breaked. I don't know
if some sort of skb recycling may happen at this stage and reveal some
bad corner cases (eg: improperly initialized skb during the rx path that
causes everything to break when it's recycled for the tx path), but Arnaud
you can easily disable the rx_copybreak feature by setting the rx_copybreak
module argument to zero. You can change it at run time in /sys/module. At
least it will tell us if it could be related or not.

Regards,
Willy