* [BUG] TIPC handling of -ERESTARTSYS in connect() @ 2012-08-30 15:20 Chris Friesen 2012-08-31 9:37 ` Ying Xue 0 siblings, 1 reply; 8+ messages in thread From: Chris Friesen @ 2012-08-30 15:20 UTC (permalink / raw) To: netdev, ying.xue, Allan Stephens, Jon Maloy 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-08-30 15:20 [BUG] TIPC handling of -ERESTARTSYS in connect() Chris Friesen @ 2012-08-31 9:37 ` Ying Xue 2012-08-31 15:09 ` Chris Friesen 0 siblings, 1 reply; 8+ messages in thread From: Ying Xue @ 2012-08-31 9:37 UTC (permalink / raw) To: Chris Friesen; +Cc: netdev, Allan Stephens, Jon Maloy [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-08-31 9:37 ` Ying Xue @ 2012-08-31 15:09 ` Chris Friesen 2012-08-31 15:18 ` Chris Friesen 0 siblings, 1 reply; 8+ messages in thread From: Chris Friesen @ 2012-08-31 15:09 UTC (permalink / raw) To: Ying Xue; +Cc: netdev, Allan Stephens, Jon Maloy 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-08-31 15:09 ` Chris Friesen @ 2012-08-31 15:18 ` Chris Friesen 2012-09-03 3:16 ` Ying Xue 0 siblings, 1 reply; 8+ messages in thread From: Chris Friesen @ 2012-08-31 15:18 UTC (permalink / raw) To: Ying Xue; +Cc: netdev, Allan Stephens, Jon Maloy 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-08-31 15:18 ` Chris Friesen @ 2012-09-03 3:16 ` Ying Xue 2012-09-04 14:54 ` Chris Friesen 0 siblings, 1 reply; 8+ messages in thread From: Ying Xue @ 2012-09-03 3:16 UTC (permalink / raw) To: Chris Friesen; +Cc: netdev, Allan Stephens, Jon Maloy [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-09-03 3:16 ` Ying Xue @ 2012-09-04 14:54 ` Chris Friesen 2012-10-31 23:01 ` Chris Friesen 0 siblings, 1 reply; 8+ messages in thread From: Chris Friesen @ 2012-09-04 14:54 UTC (permalink / raw) To: Ying Xue; +Cc: netdev, Allan Stephens, Jon Maloy 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-09-04 14:54 ` Chris Friesen @ 2012-10-31 23:01 ` Chris Friesen 2012-11-01 6:35 ` Xue Ying 0 siblings, 1 reply; 8+ messages in thread From: Chris Friesen @ 2012-10-31 23:01 UTC (permalink / raw) To: Ying Xue; +Cc: netdev, Allan Stephens, Jon Maloy 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [BUG] TIPC handling of -ERESTARTSYS in connect() 2012-10-31 23:01 ` Chris Friesen @ 2012-11-01 6:35 ` Xue Ying 0 siblings, 0 replies; 8+ messages in thread From: Xue Ying @ 2012-11-01 6:35 UTC (permalink / raw) To: Chris Friesen; +Cc: Ying Xue, netdev, Allan Stephens, Jon Maloy > > 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-11-01 6:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-30 15:20 [BUG] TIPC handling of -ERESTARTSYS in connect() Chris Friesen 2012-08-31 9:37 ` Ying Xue 2012-08-31 15:09 ` Chris Friesen 2012-08-31 15:18 ` Chris Friesen 2012-09-03 3:16 ` Ying Xue 2012-09-04 14:54 ` Chris Friesen 2012-10-31 23:01 ` Chris Friesen 2012-11-01 6:35 ` Xue Ying
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).