[PATCH] inet: race in wait for connect.

[PATCH] inet: race in wait for connect.

[Stephen Hemminger]

Fix possible race while waiting for connections in accept. I don't
know of a test case that could reproduce this directly.

The state of the socket should be checked before checking the queue.
If the socket has left the TCP_LISTEN state, then the accept queue
is no longer valid.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/net/ipv4/inet_connection_sock.c	2007-10-26 11:54:41.000000000 -0700
+++ b/net/ipv4/inet_connection_sock.c	2007-10-29 08:34:03.000000000 -0700
@@ -203,12 +203,12 @@ static int inet_csk_wait_for_connect(str
 		if (reqsk_queue_empty(&icsk->icsk_accept_queue))
 			timeo = schedule_timeout(timeo);
 		lock_sock(sk);
-		err = 0;
-		if (!reqsk_queue_empty(&icsk->icsk_accept_queue))
-			break;
 		err = -EINVAL;
 		if (sk->sk_state != TCP_LISTEN)
 			break;
+		err = 0;
+		if (!reqsk_queue_empty(&icsk->icsk_accept_queue))
+			break;
 		err = sock_intr_errno(timeo);
 		if (signal_pending(current))
 			break;

Re: [PATCH] inet: race in wait for connect.

[Arnaldo Carvalho de Melo]

Em Mon, Oct 29, 2007 at 01:52:22PM -0700, Stephen Hemminger escreveu:
> Fix possible race while waiting for connections in accept. I don't
> know of a test case that could reproduce this directly.
> 
> The state of the socket should be checked before checking the queue.
> If the socket has left the TCP_LISTEN state, then the accept queue
> is no longer valid.

Well if it left from LISTEN to CLOSED inet_csk_listen_stop must have
been called, and that calls reqsk_queue_yank_acceptq, that sets it to
NULL, reqsk_queue_empty(&icsk->icsk_accept_queue) returns true and we
get to if (sk->sk_state != TCP_LISTEN), returning -EINVAL as with your
patch.  So I can't see a race, just one branch less in one case :-)

- Arnaldo
 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> --- a/net/ipv4/inet_connection_sock.c	2007-10-26 11:54:41.000000000 -0700
> +++ b/net/ipv4/inet_connection_sock.c	2007-10-29 08:34:03.000000000 -0700
> @@ -203,12 +203,12 @@ static int inet_csk_wait_for_connect(str
>  		if (reqsk_queue_empty(&icsk->icsk_accept_queue))
>  			timeo = schedule_timeout(timeo);
>  		lock_sock(sk);
> -		err = 0;
> -		if (!reqsk_queue_empty(&icsk->icsk_accept_queue))
> -			break;
>  		err = -EINVAL;
>  		if (sk->sk_state != TCP_LISTEN)
>  			break;
> +		err = 0;
> +		if (!reqsk_queue_empty(&icsk->icsk_accept_queue))
> +			break;
>  		err = sock_intr_errno(timeo);
>  		if (signal_pending(current))
>  			break;

Re: [PATCH] inet: race in wait for connect.

[Stephen Hemminger]

On Mon, 29 Oct 2007 19:29:06 -0200
"Arnaldo Carvalho de Melo" <acme@redhat.com> wrote:

> Em Mon, Oct 29, 2007 at 01:52:22PM -0700, Stephen Hemminger escreveu:
> > Fix possible race while waiting for connections in accept. I don't
> > know of a test case that could reproduce this directly.
> > 
> > The state of the socket should be checked before checking the queue.
> > If the socket has left the TCP_LISTEN state, then the accept queue
> > is no longer valid.
> 
> Well if it left from LISTEN to CLOSED inet_csk_listen_stop must have
> been called, and that calls reqsk_queue_yank_acceptq, that sets it to
> NULL, reqsk_queue_empty(&icsk->icsk_accept_queue) returns true and we
> get to if (sk->sk_state != TCP_LISTEN), returning -EINVAL as with your
> patch.  So I can't see a race, just one branch less in one case :-)
> 
> - Arnaldo

Yeah, your right. The listen queue is garbage at this point but the
accept queue is always empty.


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] inet: race in wait for connect.

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Mon, 29 Oct 2007 14:37:14 -0700

> On Mon, 29 Oct 2007 19:29:06 -0200
> "Arnaldo Carvalho de Melo" <acme@redhat.com> wrote:
> 
> > Em Mon, Oct 29, 2007 at 01:52:22PM -0700, Stephen Hemminger escreveu:
> > > Fix possible race while waiting for connections in accept. I don't
> > > know of a test case that could reproduce this directly.
> > > 
> > > The state of the socket should be checked before checking the queue.
> > > If the socket has left the TCP_LISTEN state, then the accept queue
> > > is no longer valid.
> > 
> > Well if it left from LISTEN to CLOSED inet_csk_listen_stop must have
> > been called, and that calls reqsk_queue_yank_acceptq, that sets it to
> > NULL, reqsk_queue_empty(&icsk->icsk_accept_queue) returns true and we
> > get to if (sk->sk_state != TCP_LISTEN), returning -EINVAL as with your
> > patch.  So I can't see a race, just one branch less in one case :-)
> > 
> > - Arnaldo
> 
> Yeah, your right. The listen queue is garbage at this point but the
> accept queue is always empty.

Is there some failing test case you are aware of which led
you to this piece of code or did you happen to notice it
while scanning around?

Re: [PATCH] inet: race in wait for connect.

[Stephen Hemminger]

On Mon, 29 Oct 2007 15:38:42 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Mon, 29 Oct 2007 14:37:14 -0700
> 
> > On Mon, 29 Oct 2007 19:29:06 -0200
> > "Arnaldo Carvalho de Melo" <acme@redhat.com> wrote:
> > 
> > > Em Mon, Oct 29, 2007 at 01:52:22PM -0700, Stephen Hemminger escreveu:
> > > > Fix possible race while waiting for connections in accept. I don't
> > > > know of a test case that could reproduce this directly.
> > > > 
> > > > The state of the socket should be checked before checking the queue.
> > > > If the socket has left the TCP_LISTEN state, then the accept queue
> > > > is no longer valid.
> > > 
> > > Well if it left from LISTEN to CLOSED inet_csk_listen_stop must have
> > > been called, and that calls reqsk_queue_yank_acceptq, that sets it to
> > > NULL, reqsk_queue_empty(&icsk->icsk_accept_queue) returns true and we
> > > get to if (sk->sk_state != TCP_LISTEN), returning -EINVAL as with your
> > > patch.  So I can't see a race, just one branch less in one case :-)
> > > 
> > > - Arnaldo
> > 
> > Yeah, your right. The listen queue is garbage at this point but the
> > accept queue is always empty.
> 
> Is there some failing test case you are aware of which led
> you to this piece of code or did you happen to notice it
> while scanning around?

While instrumenting the snot out of accept logic for looking at alternatives
to the MT-accept vs. close issue.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>