Badness at net/ipv4/inet_connection_sock.c:293

Badness at net/ipv4/inet_connection_sock.c:293

[Christian Kujau]

Hi there,

today I upgraded from 2.6.32-rc7 to the latest -git (mainline) and after a 
few hours the messages below appeared in the log. Apparently "privoxy" was 
very busy handling connections, but the messages never appeared with 
earlier kernels. 

Full dmesg & .config is on:
http://nerdbynature.de/bits/2.6.33-git/inet_accept/

Thanks,
Christian.

 ------------[ cut here ]------------
 Badness at net/ipv4/inet_connection_sock.c:293
 NIP: c02a0914 LR: c02a0904 CTR: 00000000
 REGS: eeecbd30 TRAP: 0700   Not tainted  (2.6.32)
 MSR: 00029032 <EE,ME,CE,IR,DR>  CR: 24002442  XER: 00000000
 TASK = eee9a820[1756] 'privoxy' THREAD: eeeca000
 GPR00: 00000001 eeecbde0 eee9a820 c03f7094 d4e86ba0 00000000 00001d40 00000000 
 GPR08: eeecbdf4 d4e86b40 00000000 c0d42000 00000000 10054804 bfedd9fc 10038614 
 GPR16: 100385f8 100385c8 00000000 1004c824 100385dc 1004c80c 1004c9d0 100385a4 
 GPR24: 1004c824 7fffffff fffffe04 eeecbe38 eeecbde8 7fffffff cb3b4840 eecc6520 
 NIP [c02a0914] inet_csk_accept+0x94/0x1f0
 LR [c02a0904] inet_csk_accept+0x84/0x1f0
 Call Trace:
 [eeecbde0] [c02a0904] inet_csk_accept+0x84/0x1f0 (unreliable)
 [eeecbe30] [c02c3794] inet_accept+0x40/0xb8
 [eeecbe50] [c0268448] sys_accept4+0xfc/0x1fc
 [eeecbf00] [c0268bd4] sys_socketcall+0x1d4/0x204
 [eeecbf40] [c0012dcc] ret_from_syscall+0x0/0x38
 --- Exception: c01 at 0xff393d4
     LR = 0xff393c0
 Instruction dump:
 0f000000 a13f00b4 7d645b78 3929ffff b13f00b4 816b0018 806b0008 4bdf0e41 
 881e0016 68000003 7c000034 5400d97e <0f000000> 7fe3fb78 4bfc948d 80010054 


 ------------[ cut here ]------------
 Badness at net/ipv4/af_inet.c:668
 NIP: c02c37b8 LR: c02c37a4 CTR: 00000000
 REGS: eeecbd80 TRAP: 0700   Tainted: G        W   (2.6.32)
 MSR: 00029032 <EE,ME,CE,IR,DR>  CR: 24002442  XER: 00000000
 TASK = eee9a820[1756] 'privoxy' THREAD: eeeca000
 GPR00: 00000001 eeecbe30 eee9a820 cb3b4840 00000000 00000000 00001d40 00000000 
 GPR08: eeecbdf4 00000003 00000000 eeeca000 00000000 10054804 bfedd9fc 10038614 
 GPR16: 100385f8 100385c8 00000000 1004c824 100385dc 1004c80c 1004c9d0 100385a4 
 GPR24: 1004c824 ffffffff 00000010 bfedd9a8 bfedd9ac ef645580 e5a5dc60 cb3b4840 
 NIP [c02c37b8] inet_accept+0x64/0xb8
 LR [c02c37a4] inet_accept+0x50/0xb8
 Call Trace:
 [eeecbe30] [c02c37a4] inet_accept+0x50/0xb8 (unreliable)
 [eeecbe50] [c0268448] sys_accept4+0xfc/0x1fc
 [eeecbf00] [c0268bd4] sys_socketcall+0x1d4/0x204
 [eeecbf40] [c0012dcc] ret_from_syscall+0x0/0x38
 --- Exception: c01 at 0xff393d4
     LR = 0xff393c0
 Instruction dump:
 816b000c 7d6903a6 4e800421 7c7f1b79 4182005c 38800000 4bfa66b1 893f0016 
 38000182 7c004e30 68000001 540007fe <0f000000> 4bd722c9 381e0010 901f0044 



-- 
BOFH excuse #137:

User was distributing pornography on server; system seized by FBI.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: Christian Kujau <lists@nerdbynature.de>
Date: Fri, 11 Dec 2009 23:43:10 -0800 (PST)

> today I upgraded from 2.6.32-rc7 to the latest -git (mainline) and after a 
> few hours the messages below appeared in the log. Apparently "privoxy" was 
> very busy handling connections, but the messages never appeared with 
> earlier kernels. 
> 
> Full dmesg & .config is on:
> http://nerdbynature.de/bits/2.6.33-git/inet_accept/

Yeah I started seeing this on one of my machines too, the
assertion is:

	WARN_ON(newsk->sk_state == TCP_SYN_RECV);

I wonder if some of the refactorings we did to allow per-route
SACK/DSACK/etc. controls messed some sequence of state changes
on TCP sockets.  But I can't find anything obvious in those
commits.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[Eric Dumazet]

Le 12/12/2009 10:03, David Miller a écrit :
> From: Christian Kujau <lists@nerdbynature.de>
> Date: Fri, 11 Dec 2009 23:43:10 -0800 (PST)
> 
>> today I upgraded from 2.6.32-rc7 to the latest -git (mainline) and after a 
>> few hours the messages below appeared in the log. Apparently "privoxy" was 
>> very busy handling connections, but the messages never appeared with 
>> earlier kernels. 
>>
>> Full dmesg & .config is on:
>> http://nerdbynature.de/bits/2.6.33-git/inet_accept/
> 
> Yeah I started seeing this on one of my machines too, the
> assertion is:
> 
> 	WARN_ON(newsk->sk_state == TCP_SYN_RECV);
> 
> I wonder if some of the refactorings we did to allow per-route
> SACK/DSACK/etc. controls messed some sequence of state changes
> on TCP sockets.  But I can't find anything obvious in those
> commits.

Could it be about syncookies patches ?

tcp_create_openreq_child() changes ?

Re: Badness at net/ipv4/inet_connection_sock.c:293

[lists]

On Sat, 12 Dec 2009 at 10:22, Eric Dumazet wrote:
> > Yeah I started seeing this on one of my machines too, the
> > assertion is:
> > 
> > 	WARN_ON(newsk->sk_state == TCP_SYN_RECV);
> > 
> > I wonder if some of the refactorings we did to allow per-route
> > SACK/DSACK/etc. controls messed some sequence of state changes
> > on TCP sockets.  But I can't find anything obvious in those
> > commits.
> 
> Could it be about syncookies patches ?
> tcp_create_openreq_child() changes ?

Hm, I'd like to reproduce this thing and just ran "siege" against the 
proxy, spawning > 10k connections (simliar to when the warnings appeared) 
but I can't trigger the message any more. I'm tempted to bisect, but this 
is no fun when I cannot reproduce the messages :-\

Christian.
-- 
BOFH excuse #140:

LBNC (luser brain not connected)

Re: Badness at net/ipv4/inet_connection_sock.c:293

[Eric Dumazet]

Le 12/12/2009 10:22, Eric Dumazet a écrit :
> Le 12/12/2009 10:03, David Miller a écrit :
>> From: Christian Kujau <lists@nerdbynature.de>
>> Date: Fri, 11 Dec 2009 23:43:10 -0800 (PST)
>>
>>> today I upgraded from 2.6.32-rc7 to the latest -git (mainline) and after a 
>>> few hours the messages below appeared in the log. Apparently "privoxy" was 
>>> very busy handling connections, but the messages never appeared with 
>>> earlier kernels. 
>>>
>>> Full dmesg & .config is on:
>>> http://nerdbynature.de/bits/2.6.33-git/inet_accept/
>>
>> Yeah I started seeing this on one of my machines too, the
>> assertion is:
>>
>> 	WARN_ON(newsk->sk_state == TCP_SYN_RECV);
>>
>> I wonder if some of the refactorings we did to allow per-route
>> SACK/DSACK/etc. controls messed some sequence of state changes
>> on TCP sockets.  But I can't find anything obvious in those
>> commits.
> 
> Could it be about syncookies patches ?
> 
> tcp_create_openreq_child() changes ?
> 

It seems to me tcp_create_openreq_child() doesnt properly initialize
newtp->cookie_values to NULL, but this should not produce warnings like that ?

Sorry to not provides a proper patch now, I have to run...

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 14 Dec 2009 06:56:31 +0100

> It seems to me tcp_create_openreq_child() doesnt properly initialize
> newtp->cookie_values to NULL, but this should not produce warnings like that ?

If oldtp->cookie_values is NULL, the child's should be as well
because of sk_clone().

Re: Badness at net/ipv4/inet_connection_sock.c:293

[Eric Dumazet]

Le 14/12/2009 08:45, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 14 Dec 2009 06:56:31 +0100
> 
>> It seems to me tcp_create_openreq_child() doesnt properly initialize
>> newtp->cookie_values to NULL, but this should not produce warnings like that ?
> 
> If oldtp->cookie_values is NULL, the child's should be as well
> because of sk_clone().

Right, maybe then its a tcp_ack() or a syncookie validation change ?


tcp_v4_rcv()
        bh_lock_sock_nested(sk);
        if (!sock_owned_by_user(sk)) {

        -> tcp_v4_do_rcv()
                -> tcp_v4_hnd_req()
                        -> cookie_v4_check()
                                -> get_cookie_sock()
                                        -> child = syn_recv_sock()
                                        -> inet_csk_reqsk_queue_add(child) (TCP_SYN_RECV socket queued into parent)
                -> tcp_child_process() (backlog... not)
                        -> tcp_rcv_state_process()
                                -> acceptable = tcp_ack() > 0;
                                -> if (acceptable) -> sk_state = TCP_ESTABLISHED
                                        (but if tcp_ack() returned <= 0, state unchanged : TCP_SYN_RECV)


And commit 96e0bf4b5193d0d97d139f99e2dd128763d55521
(tcp: Discard segments that ack data not yet sent)

Did change this area a bit :

@@ -5632,7 +5639,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 
        /* step 5: check the ACK field */
        if (th->ack) {
-               int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH);
+               int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH) > 0;
 
                switch (sk->sk_state) {
                case TCP_SYN_RECV:

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 14 Dec 2009 18:34:00 +0100

> And commit 96e0bf4b5193d0d97d139f99e2dd128763d55521
> (tcp: Discard segments that ack data not yet sent)
> 
> Did change this area a bit :

Good catch.

I can reproduce this quite readily so I'll try reverting that
commit.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[John Dykstra]

On Mon, 2009-12-14 at 18:34 +0100, Eric Dumazet wrote:
> Le 14/12/2009 08:45, David Miller a écrit :
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 14 Dec 2009 06:56:31 +0100
> > 
> >> It seems to me tcp_create_openreq_child() doesnt properly initialize
> >> newtp->cookie_values to NULL, but this should not produce warnings like that ?
> > 
> > If oldtp->cookie_values is NULL, the child's should be as well
> > because of sk_clone().
> 
> Right, maybe then its a tcp_ack() or a syncookie validation change ?
> 
> 
> tcp_v4_rcv()
>         bh_lock_sock_nested(sk);
>         if (!sock_owned_by_user(sk)) {
> 
>         -> tcp_v4_do_rcv()
>                 -> tcp_v4_hnd_req()
>                         -> cookie_v4_check()
>                                 -> get_cookie_sock()
>                                         -> child = syn_recv_sock()
>                                         -> inet_csk_reqsk_queue_add(child) (TCP_SYN_RECV socket queued into parent)
>                 -> tcp_child_process() (backlog... not)
>                         -> tcp_rcv_state_process()
>                                 -> acceptable = tcp_ack() > 0;
>                                 -> if (acceptable) -> sk_state = TCP_ESTABLISHED
>                                         (but if tcp_ack() returned <= 0, state unchanged : TCP_SYN_RECV)
> 
> 
> And commit 96e0bf4b5193d0d97d139f99e2dd128763d55521
> (tcp: Discard segments that ack data not yet sent)
> 
> Did change this area a bit :
> 
> @@ -5632,7 +5639,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>  
>         /* step 5: check the ACK field */
>         if (th->ack) {
> -               int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH);
> +               int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH) > 0;
>  
>                 switch (sk->sk_state) {
>                 case TCP_SYN_RECV:

That test was changed to match a change in the return values of
tcp_ack().  No logic change was intended.

I won't be able to catch up on this thread and take a look at the code
until this evening, CST.

  --  John

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: John Dykstra <john.dykstra1@gmail.com>
Date: Mon, 14 Dec 2009 18:57:13 +0000

> That test was changed to match a change in the return values of
> tcp_ack().  No logic change was intended.

As Eric pointed out, changing tcp_ack()'s behavior effects state
transitions out of SYN_ACK, so this commit is very likely the culprit.

I usually see 2 or 3 three of these bug triggers in the first ten
minutes after bootup and I haven't seen one yet in 20 minutes
with the commit reverted.

This change is definitely wrong.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 14 Dec 2009 11:11:05 -0800 (PST)

> I usually see 2 or 3 three of these bug triggers in the first ten
> minutes after bootup and I haven't seen one yet in 20 minutes
> with the commit reverted.

Nevermind, I take that back, I just got two triggers :-/

Re: Badness at net/ipv4/inet_connection_sock.c:293

[Eric Dumazet]

Le 14/12/2009 20:16, David Miller a écrit :
> From: David Miller <davem@davemloft.net>
> Date: Mon, 14 Dec 2009 11:11:05 -0800 (PST)
> 
>> I usually see 2 or 3 three of these bug triggers in the first ten
>> minutes after bootup and I haven't seen one yet in 20 minutes
>> with the commit reverted.
> 
> Nevermind, I take that back, I just got two triggers :-/

ok :)

More investigation needed :)

I wonder which workload triggers this...

If you disable syncookies, I presume you dont have warnings ?

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 14 Dec 2009 20:23:32 +0100

> I wonder which workload triggers this...
> 
> If you disable syncookies, I presume you dont have warnings ?

Haven't tried, will do that.

All I do is receive email on this thing, but as you can
guess I'm a popular spam target, so lots of random sites.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 14 Dec 2009 20:23:32 +0100

> If you disable syncookies, I presume you dont have warnings ?

It happens even with syncookies disabled, hmmm...

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 14 Dec 2009 14:35:22 -0800 (PST)

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 14 Dec 2009 20:23:32 +0100
> 
>> If you disable syncookies, I presume you dont have warnings ?
> 
> It happens even with syncookies disabled, hmmm...

FWIW, I'm starting to bisect this.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[Eric Dumazet]

Le 15/12/2009 08:18, David Miller a écrit :
> From: David Miller <davem@davemloft.net>
> Date: Mon, 14 Dec 2009 14:35:22 -0800 (PST)
> 
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Mon, 14 Dec 2009 20:23:32 +0100
>>
>>> If you disable syncookies, I presume you dont have warnings ?
>>
>> It happens even with syncookies disabled, hmmm...
> 
> FWIW, I'm starting to bisect this.


OK, I took a look at commit 05eaade2782fb0c90d3034fd7a7d5a16266182bb,
but I have no more time today for kernel dev, sorry :(

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 14 Dec 2009 23:18:40 -0800 (PST)

> From: David Miller <davem@davemloft.net>
> Date: Mon, 14 Dec 2009 14:35:22 -0800 (PST)
> 
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Mon, 14 Dec 2009 20:23:32 +0100
>> 
>>> If you disable syncookies, I presume you dont have warnings ?
>> 
>> It happens even with syncookies disabled, hmmm...
> 
> FWIW, I'm starting to bisect this.

Ok, it's the patch series that adds all of the per-route
SACK/DSACK/TIMESTAMP controls.

I hand reverted the entire series and the badness triggers no longer
occur.  I couldn't bisect through the individual changes because of
all of the bug fixes that happened at the end of them.

I'm pretty sure the problem eminates from changing the 'estab'
parameter passed into tcp_parse_options() from
tcp_timewait_state_process() and tcp_check_req().

But even if we fix that, this patch series is very fundamentally
broken.  It takes the 'dst' for the listening socket when creating new
incoming connections in tcp_check_req() to probe the per-route settings.

Listening sockets don't have a route.  So this can't ever work
properly.

And getting the route for the child request we're making (the right
one) can't be done until we setup all of the state and call down into
the code which emits the SYN+ACK packet and look the route up by hand.

I'm simply going to revert this entire series.  Sorry this didn't work
out, but we're not adding this feautre this time around.  Maybe you
can write a working version for the next merge widnow.

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 15 Dec 2009 20:50:55 -0800 (PST)

> I'm simply going to revert this entire series.

tcp: Revert per-route SACK/DSACK/TIMESTAMP changes.

It creates a regression, triggering badness for SYN_RECV
sockets, for example:

[19148.022102] Badness at net/ipv4/inet_connection_sock.c:293
[19148.022570] NIP: c02a0914 LR: c02a0904 CTR: 00000000
[19148.023035] REGS: eeecbd30 TRAP: 0700   Not tainted  (2.6.32)
[19148.023496] MSR: 00029032 <EE,ME,CE,IR,DR>  CR: 24002442  XER: 00000000
[19148.024012] TASK = eee9a820[1756] 'privoxy' THREAD: eeeca000

This is likely caused by the change in the 'estab' parameter
passed to tcp_parse_options() when invoked by the functions
in net/ipv4/tcp_minisocks.c

But even if that is fixed, the ->conn_request() changes made in
this patch series is fundamentally wrong.  They try to use the
listening socket's 'dst' to probe the route settings.  The
listening socket doesn't even have a route, and you can't
get the right route (the child request one) until much later
after we setup all of the state, and it must be done by hand.

This stuff really isn't ready, so the best thing to do is a
full revert.  This reverts the following commits:

f55017a93f1a74d50244b1254b9a2bd7ac9bbf7d
022c3f7d82f0f1c68018696f2f027b87b9bb45c2
1aba721eba1d84a2defce45b950272cee1e6c72a
cda42ebd67ee5fdf09d7057b5a4584d36fe8a335
345cda2fd695534be5a4494f1b59da9daed33663
dc343475ed062e13fc260acccaab91d7d80fd5b2
05eaade2782fb0c90d3034fd7a7d5a16266182bb
6a2a2d6bf8581216e08be15fcb563cfd6c430e1e

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/linux/rtnetlink.h |    6 ++----
 include/net/dst.h         |    2 +-
 include/net/tcp.h         |    3 +--
 net/ipv4/syncookies.c     |   27 +++++++++++++--------------
 net/ipv4/tcp_input.c      |   24 ++++++++----------------
 net/ipv4/tcp_ipv4.c       |   21 +++++++++------------
 net/ipv4/tcp_minisocks.c  |   10 +++++-----
 net/ipv4/tcp_output.c     |   18 +++++-------------
 net/ipv6/syncookies.c     |   28 +++++++++++++---------------
 net/ipv6/tcp_ipv6.c       |    3 +--
 10 files changed, 58 insertions(+), 84 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 14fc906..05330fc 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -368,11 +368,9 @@ enum {
 #define RTAX_MAX (__RTAX_MAX - 1)
 
 #define RTAX_FEATURE_ECN	0x00000001
-#define RTAX_FEATURE_NO_SACK	0x00000002
-#define RTAX_FEATURE_NO_TSTAMP	0x00000004
+#define RTAX_FEATURE_SACK	0x00000002
+#define RTAX_FEATURE_TIMESTAMP	0x00000004
 #define RTAX_FEATURE_ALLFRAG	0x00000008
-#define RTAX_FEATURE_NO_WSCALE	0x00000010
-#define RTAX_FEATURE_NO_DSACK	0x00000020
 
 struct rta_session {
 	__u8	proto;
diff --git a/include/net/dst.h b/include/net/dst.h
index 387cb3c..39c4a59 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -113,7 +113,7 @@ dst_metric(const struct dst_entry *dst, int metric)
 static inline u32
 dst_feature(const struct dst_entry *dst, u32 feature)
 {
-	return (dst ? dst_metric(dst, RTAX_FEATURES) & feature : 0);
+	return dst_metric(dst, RTAX_FEATURES) & feature;
 }
 
 static inline u32 dst_mtu(const struct dst_entry *dst)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 1b6f7d3..34f5cc2 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -408,8 +408,7 @@ extern int			tcp_recvmsg(struct kiocb *iocb, struct sock *sk,
 extern void			tcp_parse_options(struct sk_buff *skb,
 						  struct tcp_options_received *opt_rx,
 						  u8 **hvpp,
-						  int estab,
-						  struct dst_entry *dst);
+						  int estab);
 
 extern u8			*tcp_parse_md5sig_option(struct tcphdr *th);
 
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 26399ad..66fd80e 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -277,6 +277,13 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 
 	NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_SYNCOOKIESRECV);
 
+	/* check for timestamp cookie support */
+	memset(&tcp_opt, 0, sizeof(tcp_opt));
+	tcp_parse_options(skb, &tcp_opt, &hash_location, 0);
+
+	if (tcp_opt.saw_tstamp)
+		cookie_check_timestamp(&tcp_opt);
+
 	ret = NULL;
 	req = inet_reqsk_alloc(&tcp_request_sock_ops); /* for safety */
 	if (!req)
@@ -292,6 +299,12 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	ireq->loc_addr		= ip_hdr(skb)->daddr;
 	ireq->rmt_addr		= ip_hdr(skb)->saddr;
 	ireq->ecn_ok		= 0;
+	ireq->snd_wscale	= tcp_opt.snd_wscale;
+	ireq->rcv_wscale	= tcp_opt.rcv_wscale;
+	ireq->sack_ok		= tcp_opt.sack_ok;
+	ireq->wscale_ok		= tcp_opt.wscale_ok;
+	ireq->tstamp_ok		= tcp_opt.saw_tstamp;
+	req->ts_recent		= tcp_opt.saw_tstamp ? tcp_opt.rcv_tsval : 0;
 
 	/* We throwed the options of the initial SYN away, so we hope
 	 * the ACK carries the same options again (see RFC1122 4.2.3.8)
@@ -340,20 +353,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 		}
 	}
 
-	/* check for timestamp cookie support */
-	memset(&tcp_opt, 0, sizeof(tcp_opt));
-	tcp_parse_options(skb, &tcp_opt, &hash_location, 0, &rt->u.dst);
-
-	if (tcp_opt.saw_tstamp)
-		cookie_check_timestamp(&tcp_opt);
-
-	ireq->snd_wscale        = tcp_opt.snd_wscale;
-	ireq->rcv_wscale        = tcp_opt.rcv_wscale;
-	ireq->sack_ok           = tcp_opt.sack_ok;
-	ireq->wscale_ok         = tcp_opt.wscale_ok;
-	ireq->tstamp_ok         = tcp_opt.saw_tstamp;
-	req->ts_recent          = tcp_opt.saw_tstamp ? tcp_opt.rcv_tsval : 0;
-
 	/* Try to redo what tcp_v4_send_synack did. */
 	req->window_clamp = tp->window_clamp ? :dst_metric(&rt->u.dst, RTAX_WINDOW);
 
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 12cab7d..28e0296 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3727,7 +3727,7 @@ old_ack:
  * the fast version below fails.
  */
 void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
-		       u8 **hvpp, int estab,  struct dst_entry *dst)
+		       u8 **hvpp, int estab)
 {
 	unsigned char *ptr;
 	struct tcphdr *th = tcp_hdr(skb);
@@ -3766,8 +3766,7 @@ void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
 				break;
 			case TCPOPT_WINDOW:
 				if (opsize == TCPOLEN_WINDOW && th->syn &&
-				    !estab && sysctl_tcp_window_scaling &&
-				    !dst_feature(dst, RTAX_FEATURE_NO_WSCALE)) {
+				    !estab && sysctl_tcp_window_scaling) {
 					__u8 snd_wscale = *(__u8 *)ptr;
 					opt_rx->wscale_ok = 1;
 					if (snd_wscale > 14) {
@@ -3783,8 +3782,7 @@ void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
 			case TCPOPT_TIMESTAMP:
 				if ((opsize == TCPOLEN_TIMESTAMP) &&
 				    ((estab && opt_rx->tstamp_ok) ||
-				     (!estab && sysctl_tcp_timestamps &&
-				      !dst_feature(dst, RTAX_FEATURE_NO_TSTAMP)))) {
+				     (!estab && sysctl_tcp_timestamps))) {
 					opt_rx->saw_tstamp = 1;
 					opt_rx->rcv_tsval = get_unaligned_be32(ptr);
 					opt_rx->rcv_tsecr = get_unaligned_be32(ptr + 4);
@@ -3792,8 +3790,7 @@ void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
 				break;
 			case TCPOPT_SACK_PERM:
 				if (opsize == TCPOLEN_SACK_PERM && th->syn &&
-				    !estab && sysctl_tcp_sack &&
-				    !dst_feature(dst, RTAX_FEATURE_NO_SACK)) {
+				    !estab && sysctl_tcp_sack) {
 					opt_rx->sack_ok = 1;
 					tcp_sack_reset(opt_rx);
 				}
@@ -3878,7 +3875,7 @@ static int tcp_fast_parse_options(struct sk_buff *skb, struct tcphdr *th,
 		if (tcp_parse_aligned_timestamp(tp, th))
 			return 1;
 	}
-	tcp_parse_options(skb, &tp->rx_opt, hvpp, 1, NULL);
+	tcp_parse_options(skb, &tp->rx_opt, hvpp, 1);
 	return 1;
 }
 
@@ -4133,10 +4130,8 @@ static inline int tcp_sack_extend(struct tcp_sack_block *sp, u32 seq,
 static void tcp_dsack_set(struct sock *sk, u32 seq, u32 end_seq)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct dst_entry *dst = __sk_dst_get(sk);
 
-	if (tcp_is_sack(tp) && sysctl_tcp_dsack &&
-	    !dst_feature(dst, RTAX_FEATURE_NO_DSACK)) {
+	if (tcp_is_sack(tp) && sysctl_tcp_dsack) {
 		int mib_idx;
 
 		if (before(seq, tp->rcv_nxt))
@@ -4165,15 +4160,13 @@ static void tcp_dsack_extend(struct sock *sk, u32 seq, u32 end_seq)
 static void tcp_send_dupack(struct sock *sk, struct sk_buff *skb)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct dst_entry *dst = __sk_dst_get(sk);
 
 	if (TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(skb)->seq &&
 	    before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
 		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOST);
 		tcp_enter_quickack_mode(sk);
 
-		if (tcp_is_sack(tp) && sysctl_tcp_dsack &&
-		    !dst_feature(dst, RTAX_FEATURE_NO_DSACK)) {
+		if (tcp_is_sack(tp) && sysctl_tcp_dsack) {
 			u32 end_seq = TCP_SKB_CB(skb)->end_seq;
 
 			if (after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt))
@@ -5428,11 +5421,10 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 	u8 *hash_location;
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct dst_entry *dst = __sk_dst_get(sk);
 	struct tcp_cookie_values *cvp = tp->cookie_values;
 	int saved_clamp = tp->rx_opt.mss_clamp;
 
-	tcp_parse_options(skb, &tp->rx_opt, &hash_location, 0, dst);
+	tcp_parse_options(skb, &tp->rx_opt, &hash_location, 0);
 
 	if (th->ack) {
 		/* rfc793:
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 15e9603..65b8ebf 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1262,20 +1262,10 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	tcp_rsk(req)->af_specific = &tcp_request_sock_ipv4_ops;
 #endif
 
-	ireq = inet_rsk(req);
-	ireq->loc_addr = daddr;
-	ireq->rmt_addr = saddr;
-	ireq->no_srccheck = inet_sk(sk)->transparent;
-	ireq->opt = tcp_v4_save_options(sk, skb);
-
-	dst = inet_csk_route_req(sk, req);
-	if(!dst)
-		goto drop_and_free;
-
 	tcp_clear_options(&tmp_opt);
 	tmp_opt.mss_clamp = TCP_MSS_DEFAULT;
 	tmp_opt.user_mss  = tp->rx_opt.user_mss;
-	tcp_parse_options(skb, &tmp_opt, &hash_location, 0, dst);
+	tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
 
 	if (tmp_opt.cookie_plus > 0 &&
 	    tmp_opt.saw_tstamp &&
@@ -1319,8 +1309,14 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	tmp_opt.tstamp_ok = tmp_opt.saw_tstamp;
 	tcp_openreq_init(req, &tmp_opt, skb);
 
+	ireq = inet_rsk(req);
+	ireq->loc_addr = daddr;
+	ireq->rmt_addr = saddr;
+	ireq->no_srccheck = inet_sk(sk)->transparent;
+	ireq->opt = tcp_v4_save_options(sk, skb);
+
 	if (security_inet_conn_request(sk, skb, req))
-		goto drop_and_release;
+		goto drop_and_free;
 
 	if (!want_cookie)
 		TCP_ECN_create_request(req, tcp_hdr(skb));
@@ -1345,6 +1341,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 		 */
 		if (tmp_opt.saw_tstamp &&
 		    tcp_death_row.sysctl_tw_recycle &&
+		    (dst = inet_csk_route_req(sk, req)) != NULL &&
 		    (peer = rt_get_peer((struct rtable *)dst)) != NULL &&
 		    peer->v4daddr == saddr) {
 			if ((u32)get_seconds() - peer->tcp_ts_stamp < TCP_PAWS_MSL &&
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 87accec..f206ee5 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -95,9 +95,9 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
 	struct tcp_timewait_sock *tcptw = tcp_twsk((struct sock *)tw);
 	int paws_reject = 0;
 
+	tmp_opt.saw_tstamp = 0;
 	if (th->doff > (sizeof(*th) >> 2) && tcptw->tw_ts_recent_stamp) {
-		tmp_opt.tstamp_ok = 1;
-		tcp_parse_options(skb, &tmp_opt, &hash_location, 1, NULL);
+		tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
 
 		if (tmp_opt.saw_tstamp) {
 			tmp_opt.ts_recent	= tcptw->tw_ts_recent;
@@ -526,9 +526,9 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 	__be32 flg = tcp_flag_word(th) & (TCP_FLAG_RST|TCP_FLAG_SYN|TCP_FLAG_ACK);
 	int paws_reject = 0;
 
-	if ((th->doff > (sizeof(*th) >> 2)) && (req->ts_recent)) {
-		tmp_opt.tstamp_ok = 1;
-		tcp_parse_options(skb, &tmp_opt, &hash_location, 1, NULL);
+	tmp_opt.saw_tstamp = 0;
+	if (th->doff > (sizeof(struct tcphdr)>>2)) {
+		tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
 
 		if (tmp_opt.saw_tstamp) {
 			tmp_opt.ts_recent = req->ts_recent;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 93316a9..383ce23 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -553,7 +553,6 @@ static unsigned tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 				struct tcp_md5sig_key **md5) {
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct tcp_cookie_values *cvp = tp->cookie_values;
-	struct dst_entry *dst = __sk_dst_get(sk);
 	unsigned remaining = MAX_TCP_OPTION_SPACE;
 	u8 cookie_size = (!tp->rx_opt.cookie_out_never && cvp != NULL) ?
 			 tcp_cookie_size_check(cvp->cookie_desired) :
@@ -581,22 +580,18 @@ static unsigned tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 	opts->mss = tcp_advertise_mss(sk);
 	remaining -= TCPOLEN_MSS_ALIGNED;
 
-	if (likely(sysctl_tcp_timestamps &&
-		   !dst_feature(dst, RTAX_FEATURE_NO_TSTAMP) &&
-		   *md5 == NULL)) {
+	if (likely(sysctl_tcp_timestamps && *md5 == NULL)) {
 		opts->options |= OPTION_TS;
 		opts->tsval = TCP_SKB_CB(skb)->when;
 		opts->tsecr = tp->rx_opt.ts_recent;
 		remaining -= TCPOLEN_TSTAMP_ALIGNED;
 	}
-	if (likely(sysctl_tcp_window_scaling &&
-		   !dst_feature(dst, RTAX_FEATURE_NO_WSCALE))) {
+	if (likely(sysctl_tcp_window_scaling)) {
 		opts->ws = tp->rx_opt.rcv_wscale;
 		opts->options |= OPTION_WSCALE;
 		remaining -= TCPOLEN_WSCALE_ALIGNED;
 	}
-	if (likely(sysctl_tcp_sack &&
-		   !dst_feature(dst, RTAX_FEATURE_NO_SACK))) {
+	if (likely(sysctl_tcp_sack)) {
 		opts->options |= OPTION_SACK_ADVERTISE;
 		if (unlikely(!(OPTION_TS & opts->options)))
 			remaining -= TCPOLEN_SACKPERM_ALIGNED;
@@ -2527,9 +2522,7 @@ static void tcp_connect_init(struct sock *sk)
 	 * See tcp_input.c:tcp_rcv_state_process case TCP_SYN_SENT.
 	 */
 	tp->tcp_header_len = sizeof(struct tcphdr) +
-		(sysctl_tcp_timestamps &&
-		(!dst_feature(dst, RTAX_FEATURE_NO_TSTAMP) ?
-		  TCPOLEN_TSTAMP_ALIGNED : 0));
+		(sysctl_tcp_timestamps ? TCPOLEN_TSTAMP_ALIGNED : 0);
 
 #ifdef CONFIG_TCP_MD5SIG
 	if (tp->af_specific->md5_lookup(sk, sk) != NULL)
@@ -2555,8 +2548,7 @@ static void tcp_connect_init(struct sock *sk)
 				  tp->advmss - (tp->rx_opt.ts_recent_stamp ? tp->tcp_header_len - sizeof(struct tcphdr) : 0),
 				  &tp->rcv_wnd,
 				  &tp->window_clamp,
-				  (sysctl_tcp_window_scaling &&
-				   !dst_feature(dst, RTAX_FEATURE_NO_WSCALE)),
+				  sysctl_tcp_window_scaling,
 				  &rcv_wscale);
 
 	tp->rx_opt.rcv_wscale = rcv_wscale;
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 5b9af50..7208a06 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -185,6 +185,13 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 
 	NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_SYNCOOKIESRECV);
 
+	/* check for timestamp cookie support */
+	memset(&tcp_opt, 0, sizeof(tcp_opt));
+	tcp_parse_options(skb, &tcp_opt, &hash_location, 0);
+
+	if (tcp_opt.saw_tstamp)
+		cookie_check_timestamp(&tcp_opt);
+
 	ret = NULL;
 	req = inet6_reqsk_alloc(&tcp6_request_sock_ops);
 	if (!req)
@@ -218,6 +225,12 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	req->expires = 0UL;
 	req->retrans = 0;
 	ireq->ecn_ok		= 0;
+	ireq->snd_wscale	= tcp_opt.snd_wscale;
+	ireq->rcv_wscale	= tcp_opt.rcv_wscale;
+	ireq->sack_ok		= tcp_opt.sack_ok;
+	ireq->wscale_ok		= tcp_opt.wscale_ok;
+	ireq->tstamp_ok		= tcp_opt.saw_tstamp;
+	req->ts_recent		= tcp_opt.saw_tstamp ? tcp_opt.rcv_tsval : 0;
 	treq->rcv_isn = ntohl(th->seq) - 1;
 	treq->snt_isn = cookie;
 
@@ -253,21 +266,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 			goto out_free;
 	}
 
-	/* check for timestamp cookie support */
-	memset(&tcp_opt, 0, sizeof(tcp_opt));
-	tcp_parse_options(skb, &tcp_opt, &hash_location, 0, dst);
-
-	if (tcp_opt.saw_tstamp)
-		cookie_check_timestamp(&tcp_opt);
-
-	req->ts_recent          = tcp_opt.saw_tstamp ? tcp_opt.rcv_tsval : 0;
-
-	ireq->snd_wscale        = tcp_opt.snd_wscale;
-	ireq->rcv_wscale        = tcp_opt.rcv_wscale;
-	ireq->sack_ok           = tcp_opt.sack_ok;
-	ireq->wscale_ok         = tcp_opt.wscale_ok;
-	ireq->tstamp_ok         = tcp_opt.saw_tstamp;
-
 	req->window_clamp = tp->window_clamp ? :dst_metric(dst, RTAX_WINDOW);
 	tcp_select_initial_window(tcp_full_space(sk), req->mss,
 				  &req->rcv_wnd, &req->window_clamp,
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index ee9cf62..febfd59 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1169,7 +1169,6 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	struct inet6_request_sock *treq;
 	struct ipv6_pinfo *np = inet6_sk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct dst_entry *dst = __sk_dst_get(sk);
 	__u32 isn = TCP_SKB_CB(skb)->when;
 #ifdef CONFIG_SYN_COOKIES
 	int want_cookie = 0;
@@ -1208,7 +1207,7 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	tcp_clear_options(&tmp_opt);
 	tmp_opt.mss_clamp = IPV6_MIN_MTU - sizeof(struct tcphdr) - sizeof(struct ipv6hdr);
 	tmp_opt.user_mss = tp->rx_opt.user_mss;
-	tcp_parse_options(skb, &tmp_opt, &hash_location, 0, dst);
+	tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
 
 	if (tmp_opt.cookie_plus > 0 &&
 	    tmp_opt.saw_tstamp &&
-- 
1.6.5

Re: Badness at net/ipv4/inet_connection_sock.c:293

[Eric Dumazet]

Le 14/12/2009 20:11, David Miller a écrit :
> From: John Dykstra <john.dykstra1@gmail.com>
> Date: Mon, 14 Dec 2009 18:57:13 +0000
> 
>> That test was changed to match a change in the return values of
>> tcp_ack().  No logic change was intended.
> 
> As Eric pointed out, changing tcp_ack()'s behavior effects state
> transitions out of SYN_ACK, so this commit is very likely the culprit.
> 
> I usually see 2 or 3 three of these bug triggers in the first ten
> minutes after bootup and I haven't seen one yet in 20 minutes
> with the commit reverted.
> 
> This change is definitely wrong.

Hmm, I didnt yet spotted the problem, just gave some hints ...

Re: Badness at net/ipv4/inet_connection_sock.c:293

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 14 Dec 2009 20:22:13 +0100

> Le 14/12/2009 20:11, David Miller a écrit :
>> From: John Dykstra <john.dykstra1@gmail.com>
>> Date: Mon, 14 Dec 2009 18:57:13 +0000
>> 
>>> That test was changed to match a change in the return values of
>>> tcp_ack().  No logic change was intended.
>> 
>> As Eric pointed out, changing tcp_ack()'s behavior effects state
>> transitions out of SYN_ACK, so this commit is very likely the culprit.
>> 
>> I usually see 2 or 3 three of these bug triggers in the first ten
>> minutes after bootup and I haven't seen one yet in 20 minutes
>> with the commit reverted.
>> 
>> This change is definitely wrong.
> 
> Hmm, I didnt yet spotted the problem, just gave some hints ...

You did point out that tcp_ack's behavior could influence
TCP_SYN_* state behavior, and that's all I claimed.

My "definitely wrong" conclusion was based upon not triggering
the warning for several minutes (which turned out to be a fluke),
not your analysis.