Prev: possible bug in checkpatch.pl with __deprecated?
Next: TTY patches for 2.6.33-git
From: David Miller on 14 Dec 2009 02:50
From: Eric Dumazet <eric.dumazet(a)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().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eric Dumazet on 14 Dec 2009 12:40
Le 14/12/2009 08:45, David Miller a �crit :
> From: Eric Dumazet <eric.dumazet(a)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:



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 14 Dec 2009 13:30
From: Eric Dumazet <eric.dumazet(a)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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: John Dykstra on 14 Dec 2009 14:00
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(a)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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 14 Dec 2009 14:20
From: John Dykstra <john.dykstra1(a)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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: possible bug in checkpatch.pl with __deprecated?
Next: TTY patches for 2.6.33-git