[BUG] TIPC handling of -ERESTARTSYS in connect()

[BUG] TIPC handling of -ERESTARTSYS in connect()

[Chris Friesen]

Hi,

I'm seeing some behaviour that looks unintentional in the TIPC connect() call.
I'm running TIPC 1.7.7.  My userspace code (stripped of error handling) looks
like this:

	int sd = socket (AF_TIPC, SOCK_SEQPACKET,0);
	connect(sd,(struct sockaddr*)&topsrv,sizeof(topsrv));

where topsrv is the address of the TIPC topology server.  The thing that's weird
is that intermittently we get a EISCONN error on the connect call.

Looking at the TIPC connect() code, I think what is happening is that
wait_event_interruptible_timeout() is being interrupted by a signal and returns
-ERESTARTSYS.  This sets sock->state to SS_DISCONNECTING and exits.  Userspace
sees the ERESTARTSYS and retries the syscall, then we hit the
"sock->state != SS_UNCONNECTED" check and exit with -EISCONN.

I think current mainline is susceptible to this as well.

I'm not sure what the proper fix would be--can we detect coming back in that we were
waiting for a message and just skip down to the wait_event_interruptible_timeout()
call?

Chris


-- 

Chris Friesen
Software Designer

3500 Carling Avenue
Ottawa, Ontario K2H 8E9
www.genband.com

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Ying Xue]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

Hi Chris,

Although this is a known issue, still thanks for your report.

Regardless of 1.7.7 or mainline, the issue really exists.
Can you please check and verify the attached patch?

PS: the patch is based on 1.7.7 rather than mainline.

Thanks,
Ying

Chris Friesen wrote:
> Hi,
>
> I'm seeing some behaviour that looks unintentional in the TIPC connect() call.
> I'm running TIPC 1.7.7.  My userspace code (stripped of error handling) looks
> like this:
>
> 	int sd = socket (AF_TIPC, SOCK_SEQPACKET,0);
> 	connect(sd,(struct sockaddr*)&topsrv,sizeof(topsrv));
>
> where topsrv is the address of the TIPC topology server.  The thing that's weird
> is that intermittently we get a EISCONN error on the connect call.
>
> Looking at the TIPC connect() code, I think what is happening is that
> wait_event_interruptible_timeout() is being interrupted by a signal and returns
> -ERESTARTSYS.  This sets sock->state to SS_DISCONNECTING and exits.  Userspace
> sees the ERESTARTSYS and retries the syscall, then we hit the
> "sock->state != SS_UNCONNECTED" check and exit with -EISCONN.
>
> I think current mainline is susceptible to this as well.
>
> I'm not sure what the proper fix would be--can we detect coming back in that we were
> waiting for a message and just skip down to the wait_event_interruptible_timeout()
> call?
>
> Chris
>
>
>   


[-- Attachment #2: 0001-tipc-correctly-handle-ERESTARTSYS-return-value-of-co.patch --]
[-- Type: text/x-diff, Size: 3842 bytes --]

>From d1f38c0016bf9e1c24aa9c41efca857b6a302b4e Mon Sep 17 00:00:00 2001
From: Ying Xue <ying.xue@windriver.com>
Date: Fri, 31 Aug 2012 17:26:13 +0800
Subject: [PATCH] tipc: correctly handle -ERESTARTSYS return value of connect()

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/tipc_socket.c |  109 +++++++++++++++++++++++++-----------------------
 1 files changed, 57 insertions(+), 52 deletions(-)

diff --git a/net/tipc/tipc_socket.c b/net/tipc/tipc_socket.c
index c135edf..aa5d57f 100644
--- a/net/tipc/tipc_socket.c
+++ b/net/tipc/tipc_socket.c
@@ -1456,21 +1456,6 @@ static int connect(struct socket *sock, struct sockaddr *dest, int destlen,
 		goto exit;
 	}
 
-	/* Issue Posix-compliant error code if socket is in the wrong state */
-
-	if (sock->state == SS_LISTENING) {
-		res = -EOPNOTSUPP;
-		goto exit;
-	}
-	if (sock->state == SS_CONNECTING) {
-		res = -EALREADY;
-		goto exit;
-	}
-	if (sock->state != SS_UNCONNECTED) {
-		res = -EISCONN;
-		goto exit;
-	}
-
 	/*
 	 * Reject connection attempt using multicast address
 	 *
@@ -1483,54 +1468,74 @@ static int connect(struct socket *sock, struct sockaddr *dest, int destlen,
 		goto exit;
 	}
 
-	/* Reject any messages already in receive queue (very unlikely) */
-
-	reject_rx_queue(sk);
+	switch (sock->state) {
+	case SS_UNCONNECTED:
+		/* Reject any messages already in receive queue
+		 * (very unlikely) */
+		reject_rx_queue(sk);
 
-	/* Send a 'SYN-' to destination */
+		/* Send a 'SYN-' to destination */
+		m.msg_name = dest;
+		m.msg_namelen = destlen;
+		res = send_msg(NULL, sock, &m, 0);
+		if (res < 0) {
+			goto exit;
+		}
 
-	m.msg_name = dest;
-	m.msg_namelen = destlen;
-	res = send_msg(NULL, sock, &m, 0);
-	if (res < 0) {
-		goto exit;
+		/*
+		 * Just entered SS_CONNECTING state; the only
+		 * difference is that return value in non-blocking
+		 * case is EINPROGRESS, rather than EALREADY.
+		 */
+		res = -EINPROGRESS;
+		break;
+	case SS_CONNECTING:
+		res = -EALREADY;
+		break;
+	case SS_CONNECTED:
+		res = -EISCONN;
+		break;
+	default:
+		res = -EINVAL;
+		break;
 	}
 
-	/* Wait until an 'ACK' or 'RST' arrives, or a timeout occurs */
-
-	timeout = tipc_sk(sk)->conn_timeout;
-	release_sock(sk);
-	res = wait_event_interruptible_timeout(*sk->sk_sleep,
-			(!skb_queue_empty(&sk->sk_receive_queue) ||
-			(sock->state != SS_CONNECTING)),
-			timeout ? (long)msecs_to_jiffies(timeout)
-				: MAX_SCHEDULE_TIMEOUT);
-	lock_sock(sk);
+	if (sock->state == SS_CONNECTING) {
+		/* Wait until an 'ACK' or 'RST' arrives, or a timeout occurs */
+		timeout = tipc_sk(sk)->conn_timeout;
+		release_sock(sk);
+		res = wait_event_interruptible_timeout(*sk->sk_sleep,
+				(!skb_queue_empty(&sk->sk_receive_queue) ||
+				(sock->state != SS_CONNECTING)),
+				timeout ? (long)msecs_to_jiffies(timeout)
+					: MAX_SCHEDULE_TIMEOUT);
+		lock_sock(sk);
 
-	if (res > 0) {
-		buf = skb_peek(&sk->sk_receive_queue);
-		if (buf != NULL) {
-			msg = buf_msg(buf);
-			res = auto_connect(sock, msg);
-			if (!res) {
-				if (!msg_data_sz(msg))
-					advance_rx_queue(sk);
+		if (res > 0) {
+			buf = skb_peek(&sk->sk_receive_queue);
+			if (buf != NULL) {
+				msg = buf_msg(buf);
+				res = auto_connect(sock, msg);
+				if (!res) {
+					if (!msg_data_sz(msg))
+						advance_rx_queue(sk);
+				}
+			} else {
+				if (sock->state == SS_CONNECTED) {
+					res = -EISCONN;
+				} else {
+					res = -ECONNREFUSED;
+				}
 			}
 		} else {
-			if (sock->state == SS_CONNECTED) {
-				res = -EISCONN;
+			if (res == 0) {
+				sock->state = SS_DISCONNECTING;
+				res = -ETIMEDOUT;
 			} else {
-				res = -ECONNREFUSED;
+				; /* leave "res" unchanged */
 			}
 		}
-	} else {
-		if (res == 0)
-			res = -ETIMEDOUT;
-		else
-			; /* leave "res" unchanged */
-		sock->state = SS_DISCONNECTING;
 	}
-
 exit:
 	release_sock(sk);
 	return res;
-- 
1.7.1

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Chris Friesen]

On 08/31/2012 03:37 AM, Ying Xue wrote:
> Hi Chris,
>
> Although this is a known issue, still thanks for your report.
>
> Regardless of 1.7.7 or mainline, the issue really exists.
> Can you please check and verify the attached patch?
>
> PS: the patch is based on 1.7.7 rather than mainline.


I haven't had a chance to test it yet but from visual inspection the 
patch looks pretty good.

It looks like the case where we come in with "sock->state == 
SS_LISTENING" has changed.  Previously we would return -EOPNOTSUPP but 
now it'll be -EINVAL.  Is that intentional?

Chris

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Chris Friesen]

On 08/31/2012 09:09 AM, Chris Friesen wrote:
> On 08/31/2012 03:37 AM, Ying Xue wrote:
>> Hi Chris,
>>
>> Although this is a known issue, still thanks for your report.
>>
>> Regardless of 1.7.7 or mainline, the issue really exists.
>> Can you please check and verify the attached patch?
>>
>> PS: the patch is based on 1.7.7 rather than mainline.
>
>
> I haven't had a chance to test it yet but from visual inspection the
> patch looks pretty good.
>
> It looks like the case where we come in with "sock->state ==
> SS_LISTENING" has changed. Previously we would return -EOPNOTSUPP but
> now it'll be -EINVAL. Is that intentional?

Just noticed something else.  In the SS_CONNECTING case I don't think 
there's any point in setting "res = -EALREADY" since a bit further down 
it gets set unconditionally anyway.

Chris

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Ying Xue]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]


>>
>> It looks like the case where we come in with "sock->state ==
>> SS_LISTENING" has changed. Previously we would return -EOPNOTSUPP but
>> now it'll be -EINVAL. Is that intentional?
>
> Just noticed something else.  In the SS_CONNECTING case I don't think 
> there's any point in setting "res = -EALREADY" since a bit further 
> down it gets set unconditionally anyway.
>
Yes, you are right. Please check v2 version.

Thanks,
Ying

> Chris
>


[-- Attachment #2: 0001-tipc-correct-return-value-of-connect-when-it-is-inte.patch --]
[-- Type: text/x-diff, Size: 4437 bytes --]

>From 8c4606c4dbb1cf47f357cc3620ba5b70b433b670 Mon Sep 17 00:00:00 2001
From: Ying Xue <ying.xue@windriver.com>
Date: Mon, 3 Sep 2012 10:29:28 +0800
Subject: [PATCH v2] tipc: correct return value of connect() when it is interrupted by a signal

When connect() is interrupted by a signal, it returns -ERESTARTSYS
error code, meanwhile socket state is also changed to SS_DISCONNECTING
state. If connect() syscall exits from kernel space to userspace,
kernel will deal with the pending signal. Once seeing the -ERESTARTSYS,
it will retry this syscall again. As a result, finally userspace gets
-EISCONN error code from connect() at the moment.

Correct behavior of connect() is that we should not set socket state
SS_DISCONNECTING when signal happens, but it may return a successful
code to userspace after signal is handled.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/tipc_socket.c |  109 +++++++++++++++++++++++++-----------------------
 1 files changed, 57 insertions(+), 52 deletions(-)

diff --git a/net/tipc/tipc_socket.c b/net/tipc/tipc_socket.c
index c135edf..d800087 100644
--- a/net/tipc/tipc_socket.c
+++ b/net/tipc/tipc_socket.c
@@ -1456,21 +1456,6 @@ static int connect(struct socket *sock, struct sockaddr *dest, int destlen,
 		goto exit;
 	}
 
-	/* Issue Posix-compliant error code if socket is in the wrong state */
-
-	if (sock->state == SS_LISTENING) {
-		res = -EOPNOTSUPP;
-		goto exit;
-	}
-	if (sock->state == SS_CONNECTING) {
-		res = -EALREADY;
-		goto exit;
-	}
-	if (sock->state != SS_UNCONNECTED) {
-		res = -EISCONN;
-		goto exit;
-	}
-
 	/*
 	 * Reject connection attempt using multicast address
 	 *
@@ -1483,54 +1468,74 @@ static int connect(struct socket *sock, struct sockaddr *dest, int destlen,
 		goto exit;
 	}
 
-	/* Reject any messages already in receive queue (very unlikely) */
-
-	reject_rx_queue(sk);
+	switch (sock->state) {
+	case SS_UNCONNECTED:
+		/* Reject any messages already in receive queue
+		 * (very unlikely) */
+		reject_rx_queue(sk);
 
-	/* Send a 'SYN-' to destination */
+		/* Send a 'SYN-' to destination */
+		m.msg_name = dest;
+		m.msg_namelen = destlen;
+		res = send_msg(NULL, sock, &m, 0);
+		if (res < 0) {
+			goto exit;
+		}
 
-	m.msg_name = dest;
-	m.msg_namelen = destlen;
-	res = send_msg(NULL, sock, &m, 0);
-	if (res < 0) {
-		goto exit;
+		/*
+		 * Just entered SS_CONNECTING state; the only
+		 * difference is that return value in non-blocking
+		 * case is EINPROGRESS, rather than EALREADY.
+		 */
+		res = -EINPROGRESS;
+		break;
+	case SS_LISTENING:
+		res = -EOPNOTSUPP;
+		break;
+	case SS_CONNECTED:
+		res = -EISCONN;
+		break;
+	default:
+		res = -EINVAL;
+		break;
 	}
 
-	/* Wait until an 'ACK' or 'RST' arrives, or a timeout occurs */
-
-	timeout = tipc_sk(sk)->conn_timeout;
-	release_sock(sk);
-	res = wait_event_interruptible_timeout(*sk->sk_sleep,
-			(!skb_queue_empty(&sk->sk_receive_queue) ||
-			(sock->state != SS_CONNECTING)),
-			timeout ? (long)msecs_to_jiffies(timeout)
-				: MAX_SCHEDULE_TIMEOUT);
-	lock_sock(sk);
+	if (sock->state == SS_CONNECTING) {
+		/* Wait until an 'ACK' or 'RST' arrives, or a timeout occurs */
+		timeout = tipc_sk(sk)->conn_timeout;
+		release_sock(sk);
+		res = wait_event_interruptible_timeout(*sk->sk_sleep,
+				(!skb_queue_empty(&sk->sk_receive_queue) ||
+				(sock->state != SS_CONNECTING)),
+				timeout ? (long)msecs_to_jiffies(timeout)
+					: MAX_SCHEDULE_TIMEOUT);
+		lock_sock(sk);
 
-	if (res > 0) {
-		buf = skb_peek(&sk->sk_receive_queue);
-		if (buf != NULL) {
-			msg = buf_msg(buf);
-			res = auto_connect(sock, msg);
-			if (!res) {
-				if (!msg_data_sz(msg))
-					advance_rx_queue(sk);
+		if (res > 0) {
+			buf = skb_peek(&sk->sk_receive_queue);
+			if (buf != NULL) {
+				msg = buf_msg(buf);
+				res = auto_connect(sock, msg);
+				if (!res) {
+					if (!msg_data_sz(msg))
+						advance_rx_queue(sk);
+				}
+			} else {
+				if (sock->state == SS_CONNECTED) {
+					res = -EISCONN;
+				} else {
+					res = -ECONNREFUSED;
+				}
 			}
 		} else {
-			if (sock->state == SS_CONNECTED) {
-				res = -EISCONN;
+			if (res == 0) {
+				sock->state = SS_DISCONNECTING;
+				res = -ETIMEDOUT;
 			} else {
-				res = -ECONNREFUSED;
+				; /* leave "res" unchanged */
 			}
 		}
-	} else {
-		if (res == 0)
-			res = -ETIMEDOUT;
-		else
-			; /* leave "res" unchanged */
-		sock->state = SS_DISCONNECTING;
 	}
-
 exit:
 	release_sock(sk);
 	return res;
-- 
1.7.1

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Chris Friesen]

On 09/02/2012 09:16 PM, Ying Xue wrote:
>
>>>
>>> It looks like the case where we come in with "sock->state ==
>>> SS_LISTENING" has changed. Previously we would return -EOPNOTSUPP but
>>> now it'll be -EINVAL. Is that intentional?
>>
>> Just noticed something else.  In the SS_CONNECTING case I don't think 
>> there's any point in setting "res = -EALREADY" since a bit further 
>> down it gets set unconditionally anyway.
>>
> Yes, you are right. Please check v2 version.

Looks good to me.  Still haven't had a chance to actually test it 
though, we're pushing out a release and time is scarce.

Chris

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Chris Friesen]

On 09/04/2012 08:54 AM, Chris Friesen wrote:
> On 09/02/2012 09:16 PM, Ying Xue wrote:
>>
>>>>
>>>> It looks like the case where we come in with "sock->state ==
>>>> SS_LISTENING" has changed. Previously we would return -EOPNOTSUPP but
>>>> now it'll be -EINVAL. Is that intentional?
>>>
>>> Just noticed something else. In the SS_CONNECTING case I don't think
>>> there's any point in setting "res = -EALREADY" since a bit further
>>> down it gets set unconditionally anyway.
>>>
>> Yes, you are right. Please check v2 version.
>
> Looks good to me. Still haven't had a chance to actually test it though,
> we're pushing out a release and time is scarce.

I finally got a chance to test this out, and it does indeed fix the 
problem.  Please push it upstream.

Tested-by: Chris Friesen <chris.friesen@genband.com>

Chris

Re: [BUG] TIPC handling of -ERESTARTSYS in connect()

[Xue Ying]

>
> I finally got a chance to test this out, and it does indeed fix the 
> problem.  Please push it upstream.
>
> Tested-by: Chris Friesen <chris.friesen@genband.com>
>

Thanks for your hard working, I will submit it again.

Regards,
Ying

> Chris
> -- 
> 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
>