* [PATCH] inet: race in wait for connect.
@ 2007-10-29 20:52 Stephen Hemminger
2007-10-29 21:29 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2007-10-29 20:52 UTC (permalink / raw)
To: David S. Miller; +Cc: Arnaldo Carvalho de Melo, netdev
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] inet: race in wait for connect.
2007-10-29 20:52 [PATCH] inet: race in wait for connect Stephen Hemminger
@ 2007-10-29 21:29 ` Arnaldo Carvalho de Melo
2007-10-29 21:37 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2007-10-29 21:29 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, netdev
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] inet: race in wait for connect.
2007-10-29 21:29 ` Arnaldo Carvalho de Melo
@ 2007-10-29 21:37 ` Stephen Hemminger
2007-10-29 22:38 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2007-10-29 21:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: David S. Miller, netdev
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] inet: race in wait for connect.
2007-10-29 21:37 ` Stephen Hemminger
@ 2007-10-29 22:38 ` David Miller
2007-10-29 23:16 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2007-10-29 22:38 UTC (permalink / raw)
To: shemminger; +Cc: acme, netdev
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] inet: race in wait for connect.
2007-10-29 22:38 ` David Miller
@ 2007-10-29 23:16 ` Stephen Hemminger
0 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2007-10-29 23:16 UTC (permalink / raw)
To: David Miller; +Cc: acme, netdev
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-10-29 23:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-29 20:52 [PATCH] inet: race in wait for connect Stephen Hemminger
2007-10-29 21:29 ` Arnaldo Carvalho de Melo
2007-10-29 21:37 ` Stephen Hemminger
2007-10-29 22:38 ` David Miller
2007-10-29 23:16 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).