Skip to contentSkip to conversation

TIPC handling of -ERESTARTSYS in connect()

BUG Chris Friesen 3 participants 8 messages Aug 30, 2012 – Nov 1, 2012

Subsystems (3)
Networking Network drivers TIPC network layer
lore
Share & tools
lore

Chris Friesen

author
#0 lore raw

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

Signature · 6 lines
Chris Friesen
Software Designer

3500 Carling Avenue
Ottawa, Ontario K2H 8E9
www.genband.com
#1 lore raw

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

Quoted 24 lines · 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?

Quoted 6 lines

Chris

From d1f38c0016bf9e1c24aa9c41efca857b6a302b4e Mon Sep 17 00:00:00 2001

From: Ying Xue <[email protected]>
Date: Fri, 31 Aug 2012 17:26:13 +0800
Subject: [PATCH] tipc: correctly handle -ERESTARTSYS return value of connect()

Signed-off-by Ying Xue <[email protected]>
---
 net/tipc/tipc_socket.c |  109 +++++++++++++++++++++++++-----------------------
 1 files changed, 57 insertions(+), 52 deletions(-)
net/tipc/tipc_socket.c +57 -52
index c135edf..aa5d57f 100644
@@ -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;
Signature · 1 line
1.7.1
author
#2 lore raw
Quoted 8 lines · Ying Xue

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

author
#3 lore raw
Quoted 11 lines · Chris Friesen
Quoted 8 lines · Ying Xue

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.

Quoted 6 lines

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

#4 lore raw
Quoted 8 lines
Quoted 5 lines

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

Quoted 4 lines

Chris

From 8c4606c4dbb1cf47f357cc3620ba5b70b433b670 Mon Sep 17 00:00:00 2001

From: Ying Xue <[email protected]>
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 <[email protected]>
---
 net/tipc/tipc_socket.c |  109 +++++++++++++++++++++++++-----------------------
 1 files changed, 57 insertions(+), 52 deletions(-)
net/tipc/tipc_socket.c +57 -52
index c135edf..d800087 100644
@@ -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;
Signature · 1 line
1.7.1
author
#5 lore raw
Quoted 9 lines · Ying Xue
Quoted 8 lines
Quoted 5 lines

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

author Tested-by
#6 lore raw
Quoted 7 lines · Chris Friesen
Quoted 6 lines · Ying Xue
Quoted 6 lines
Quoted 5 lines

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?

Quoted 8 lines
Quoted 5 lines
Quoted 4 lines

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 <[email protected]>

Chris

#7 lore raw
Quoted 6 lines

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

Tested-by Chris Friesen <[email protected]>

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

Regards,
Ying

Quoted 6 lines

Chris

Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html