| Message ID | 1377266200-26691-1-git-send-email-erik.hugne@ericsson.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/23/2013 09:56 PM, erik.hugne@ericsson.com wrote: > From: Erik Hugne <erik.hugne@ericsson.com> > > This fixes a problem when connect() fails and returns the error > code as a positive value, whereas errno itself is never set. The > reason is that error codes set in sk_err should never be inverted. > > Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> Acked-by: Ying Xue <ying.xue@windriver.com> > --- > net/tipc/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c > index ce8249c..6cc7ddd 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -1257,7 +1257,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > /* Accept only ACK or NACK message */ > if (unlikely(msg_errcode(msg))) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = -ECONNREFUSED; > + sk->sk_err = ECONNREFUSED; > retval = TIPC_OK; > break; > } > @@ -1268,7 +1268,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > res = auto_connect(sock, msg); > if (res) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = res; > + sk->sk_err = -res; > retval = TIPC_OK; > break; > } > -- 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
On 13-08-23 09:56 AM, erik.hugne@ericsson.com wrote: > From: Erik Hugne <erik.hugne@ericsson.com> > > This fixes a problem when connect() fails and returns the error > code as a positive value, whereas errno itself is never set. The > reason is that error codes set in sk_err should never be inverted. That seems consistent with the other instances at least. ~/git/linux-head$ git grep ECONNREFUSED net|grep sk_err net/caif/caif_socket.c: cf_sk->sk.sk_err = ECONNREFUSED; net/decnet/dn_nsp_in.c: sk->sk_err = ECONNREFUSED; net/ipv4/tcp_input.c: sk->sk_err = ECONNREFUSED; net/tipc/socket.c: sk->sk_err = -ECONNREFUSED; ~/git/linux-head$ What was the high level user visible symptom in this case? Stalled connections or ... ? Thanks, Paul. -- > > Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> > --- > net/tipc/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c > index ce8249c..6cc7ddd 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -1257,7 +1257,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > /* Accept only ACK or NACK message */ > if (unlikely(msg_errcode(msg))) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = -ECONNREFUSED; > + sk->sk_err = ECONNREFUSED; > retval = TIPC_OK; > break; > } > @@ -1268,7 +1268,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) > res = auto_connect(sock, msg); > if (res) { > sock->state = SS_DISCONNECTING; > - sk->sk_err = res; > + sk->sk_err = -res; > retval = TIPC_OK; > break; > } > -- 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
On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote: > What was the high level user visible symptom in this case? > Stalled connections or ... ? Should the connect fail, if the publication/server is unavailable or some other error. The connect() call returns the error code directly (as a positive value). [...] socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3 setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0 setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0 connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111 sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe) In the strace above, error checking was done as: if (connect(fd,.....) < 0) perror("connect"); //E -- 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
On 13-08-27 11:18 AM, Erik Hugne wrote: > On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote: >> What was the high level user visible symptom in this case? >> Stalled connections or ... ? > > Should the connect fail, if the publication/server is unavailable or some other > error. The connect() call returns the error code directly (as a positive value). Please send a v2 with the end-user visible symptom clearly described; as this information is what people use in order to triage whether commits belong in stable, or net vs. net-next etc. For example: Should the connect fail, say if the publication/server is unavailable or some other error, then the code returns a positive return value. Since most code only checks for a negative return on connect(), it tries to continue, but will ultimately fail on the 1st sendto() as the strace snippet below shows. I've said "most code" since I simply don't know what it was that you were tracing below. It would help if we knew if this part of a common application or similar. Thanks, Paul. -- > > [...] > socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3 > setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0 > setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0 > connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111 > sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe) > > In the strace above, error checking was done as: > if (connect(fd,.....) < 0) > perror("connect"); > > //E > -- 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
On 08/28/2013 02:12 AM, Paul Gortmaker wrote: > On 13-08-27 11:18 AM, Erik Hugne wrote: >> On Tue, Aug 27, 2013 at 09:20:23AM -0400, Paul Gortmaker wrote: >>> What was the high level user visible symptom in this case? >>> Stalled connections or ... ? >> >> Should the connect fail, if the publication/server is unavailable or some other >> error. The connect() call returns the error code directly (as a positive value). > > Please send a v2 with the end-user visible symptom clearly described; > as this information is what people use in order to triage whether > commits belong in stable, or net vs. net-next etc. For example: > > Should the connect fail, say if the publication/server is unavailable or > some other error, then the code returns a positive return value. Since > most code only checks for a negative return on connect(), it tries to > continue, but will ultimately fail on the 1st sendto() as the strace > snippet below shows. > > I've said "most code" since I simply don't know what it was that you were > tracing below. It would help if we knew if this part of a common application > or similar. > Firstly it's absolutely wrong that connect() returns a positive error code - 111(i.e, ECONNREFUSED) if connect() is failed, that is, it violates POSIX standard. As the patch's changelog describes, sk_err is incorrectly set to a negative value instead of positive value in filter_connect(). In connect(), it will set its return value with sock_error(sk) if it fails. As the return value of sock_error(sk) almost can be deemed as -sk_err, this is why we see a positive error code of connect() in below strace log. But, the patch is absolutely able to fix the issue. Regards, Ying > Thanks, > Paul. > -- > >> >> [...] >> socket(0x1e /* PF_??? */, SOCK_SEQPACKET, 0) = 3 >> setsockopt(3, 0x10f /* SOL_?? */, 129, [0], 4) = 0 >> setsockopt(3, 0x10f /* SOL_?? */, 127, [0], 4) = 0 >> connect(3, {sa_family=0x1e /* AF_??? */, sa_data="\2\1\322\4\0\0\322\4\0\0\0\0\0\0"}, 16) = 111 >> sendto(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 66000, 0, NULL, 0) = -1 EPIPE (Broken pipe) >> >> In the strace above, error checking was done as: >> if (connect(fd,.....) < 0) >> perror("connect"); >> >> //E >> > > -- 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
diff --git a/net/tipc/socket.c b/net/tipc/socket.c index ce8249c..6cc7ddd 100644 --- a/net/tipc/socket.c +++ b/net/tipc/socket.c @@ -1257,7 +1257,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) /* Accept only ACK or NACK message */ if (unlikely(msg_errcode(msg))) { sock->state = SS_DISCONNECTING; - sk->sk_err = -ECONNREFUSED; + sk->sk_err = ECONNREFUSED; retval = TIPC_OK; break; } @@ -1268,7 +1268,7 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf) res = auto_connect(sock, msg); if (res) { sock->state = SS_DISCONNECTING; - sk->sk_err = res; + sk->sk_err = -res; retval = TIPC_OK; break; }