Message ID | 20101110.124119.102563803.davem@davemloft.net |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
David Miller wrote, at 11/11/2010 04:41 AM: > From: Steve Chen <schen@mvista.com> > Date: Wed, 10 Nov 2010 07:24:51 -0600 > >> With commit f5fff5dc8a7a3f395b0525c02ba92c95d42b7390, a user program >> can pass in TCP_MAXSEG of 12 (or TCPOLEN_TSTAMP_ALIGNED), and cause >> kernel oops with division by 0 >> in tcp_select_initial_window. One way to prevent it is to change the >> minimum value for TCP_MAXSEG in do_tcp_setsockopt from 8 to some value >> over 12. Two questions. >> >> 1. Is this the right solution? >> 2. If it is, what is a good minimum value? > > Thanks Steve, I'll fix this like so: > > -------------------- > tcp: Increase TCP_MAXSEG socket option minimum. > > As noted by Steve Chen, since commit > f5fff5dc8a7a3f395b0525c02ba92c95d42b7390 ("tcp: advertise MSS > requested by user") we can end up with a situation where > tcp_select_initial_window() does a divide by a zero (or > even negative) mss value. > > The problem is that sometimes we subtract TCPOLEN_TSTAMP_ALIGNED > from the mss. > > Fix this by increasing the minimum from 8 to 8 plus the value > of TCPOLEN_TSTATMP_ALIGNED. In tcp_connect_init(), if tcp_header_len includes TCPOLEN_TSTAMP_ALIGNED(12 bytes) and TCPOLEN_MD5SIG_ALIGNED(20 bytes). This fix is still not perfect. The minimum value of TCP_MAXSEG is 20 tytes, tcp_select_initial_window() still be called with negative mss value.
From: Shan Wei <shanwei@cn.fujitsu.com> Date: Thu, 11 Nov 2010 13:15:01 +0800 > In tcp_connect_init(), if tcp_header_len includes TCPOLEN_TSTAMP_ALIGNED(12 bytes) > and TCPOLEN_MD5SIG_ALIGNED(20 bytes). If you knew this, why didn't you mention it in your initial report? :-/ I'll make the minimum 64 or something like that. -- 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
David Miller wrote, at 11/11/2010 01:33 PM: > From: Shan Wei <shanwei@cn.fujitsu.com> > Date: Thu, 11 Nov 2010 13:15:01 +0800 > >> In tcp_connect_init(), if tcp_header_len includes TCPOLEN_TSTAMP_ALIGNED(12 bytes) >> and TCPOLEN_MD5SIG_ALIGNED(20 bytes). > > If you knew this, why didn't you mention it in your initial report? :-/ Firstly reported by Steve Chen, not me. :-) I just review your patch. > I'll make the minimum 64 or something like that. Welcome.
Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp: Increase TCP_MAXSEG socket option minimum"). What is the reason TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8? Or is it better to use TCP_MIN_MSS from tcp.h: /* Minimal accepted MSS. It is (60+60+8) - (20+20). */ #define TCP_MIN_MSS 88U -- 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: Min Zhang <mzhang@mvista.com> Date: Fri, 12 Nov 2010 14:59:58 -0800 > Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp: > Increase TCP_MAXSEG socket option minimum"). What is the reason > TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is > TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8? > > Or is it better to use TCP_MIN_MSS from tcp.h: > > /* Minimal accepted MSS. It is (60+60+8) - (20+20). */ > #define TCP_MIN_MSS 88U I suppose TCP_MIN_MSS would be better to use, I'll make that change, thanks. -- 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
At 2010-11-13 7:26, David Miller wrote: > From: Min Zhang<mzhang@mvista.com> > Date: Fri, 12 Nov 2010 14:59:58 -0800 > >> Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp: >> Increase TCP_MAXSEG socket option minimum"). What is the reason >> TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is >> TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8? >> >> Or is it better to use TCP_MIN_MSS from tcp.h: >> >> /* Minimal accepted MSS. It is (60+60+8) - (20+20). */ >> #define TCP_MIN_MSS 88U > > I suppose TCP_MIN_MSS would be better to use, I'll make that > change, thanks. David, do you have plan to fix this bug using TCP_MIN_MSS? -- 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: Li Yewang <lyw@cn.fujitsu.com> Date: Tue, 23 Nov 2010 10:48:46 +0800 > > > At 2010-11-13 7:26, David Miller wrote: >> From: Min Zhang<mzhang@mvista.com> >> Date: Fri, 12 Nov 2010 14:59:58 -0800 >> >>> Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp: >>> Increase TCP_MAXSEG socket option minimum"). What is the reason >>> TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is >>> TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8? >>> >>> Or is it better to use TCP_MIN_MSS from tcp.h: >>> >>> /* Minimal accepted MSS. It is (60+60+8) - (20+20). */ >>> #define TCP_MIN_MSS 88U >> >> I suppose TCP_MIN_MSS would be better to use, I'll make that >> change, thanks. > > David, do you have plan to fix this bug using TCP_MIN_MSS? I will, it's deep in my backlog and pretty low priority right now. -- 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.c b/net/ipv4/tcp.c index 245603c..6b0eb4d 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2246,7 +2246,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level, /* Values greater than interface MTU won't take effect. However * at the point when this call is done we typically don't yet * know which interface is going to be used */ - if (val < 8 || val > MAX_TCP_WINDOW) { + if (val < TCPOLEN_TSTAMP_ALIGNED + 8 || val > MAX_TCP_WINDOW) { err = -EINVAL; break; }