[PATCH net] sctp: Free connecting association if there is a pending signal

[PATCH net] sctp: Free connecting association if there is a pending signal

[Petr Malat]

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

Hi!
I believe signal handling in the connect path is not implemented properly
as connect() terminates with EINPROGRESS instead of being restarted after
the signal handler returns (see attached sctp_connect_intr_bug.c).
I went through the POSIX manual pages and it seems EINPROGRESS can be
returned only if the socket is nonblocking. Linux documentation also
mention connect() could fail with EINPROGRESS if SO_SNDTIMEO expires (this
option is not discussed by POSIX manual pages). This is also how TCP
behaves.
I was able to solve the issue by freeing the association in the signal
handling path, see the attached sctp_connect_intr.patch, which also contains
the rationale for the change.
Please, review the change as I based it on code comments which may be obsolete
and I do not have knowledge of SCTP stack internals.
BR,
  Petr

[-- Attachment #2: sctp_connect_intr_bug.c --]
[-- Type: text/x-csrc, Size: 1177 bytes --]

#include <sys/socket.h>
#include <sys/types.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>
#include <errno.h>
#include <stdio.h>

#define DEAD_IP "127.16.0.1"
#define fail_if(arg) if (0 != (arg)) { \
	fprintf(stderr, "Failed at %d: %s\n", __LINE__, #arg);\
	return 1; }

void handler(int sig)
{
	printf("Got signal %d\n", sig);
}

int main(int argc, char *argv[])
{
	struct sockaddr_in addr4 = { .sin_family = AF_INET, .sin_port = htons(5555) };
	struct sockaddr *deadaddr = (struct sockaddr *)&addr4;
	struct sigaction sa = { .sa_handler = handler, .sa_flags = SA_RESTART }; 
	int sk, rtn;

	fail_if(1 != inet_pton(AF_INET, DEAD_IP, &addr4.sin_addr));

	fail_if(system("iptables -I OUTPUT -d " DEAD_IP "/32 -j DROP"));

	fail_if(0 > (sk = socket(PF_INET, SOCK_SEQPACKET, IPPROTO_SCTP)));
	fail_if(sigaction(SIGALRM, &sa, NULL));
	fail_if(alarm(3) < 0);

	rtn = connect(sk, deadaddr, sizeof *deadaddr);

	// The call should either resume the operation of fail with EINTR
	printf("rtn: %d errno: %d (%s)\n", rtn, errno, strerror(errno));

	fail_if(system("iptables -D OUTPUT -d " DEAD_IP "/32 -j DROP"));

	return 0;
}

[-- Attachment #3: sctp_connect_intr.patch --]
[-- Type: text/x-diff, Size: 1239 bytes --]

sctp: Free connecting association if there is a pending signal

When a connection attempt is interrupted by a signal, there are two
possibilities how the process can continue:
 - There has been a timeout specified through SO_SNDTIMEO, in that case
   the call must terminate the association and return -EINTR, because
   it's not possible to restart the call safely as the timeout is
   not an argument of the connect or SCTP_SOCKOPT_CONNECTX call.
 - If the connect timeout has not been limited, it's possible to restart
   the call, however the association must be freed as well to make that
   possible. Theoretically, one could pick up the ongoing association in
   the restart path, but the problem is we can't esily detect if
   the syscall restarting wasn't disabled by the user.
As a result, it's sensible to free the association independently if the
connect attempt is being restarted or not.

Signed-off-by: Petr Malat <oss@malat.biz>
--- linux-4.18-rc3.orig/net/sctp/socket.c	2018-07-09 11:46:51.991717457 +0200
+++ linux-4.18-rc3/net/sctp/socket.c	2018-07-05 15:17:29.292657471 +0200
@@ -8456,6 +8456,7 @@ do_error:
 	goto out;
 
 do_interrupted:
+	sctp_association_free(asoc);
 	err = sock_intr_errno(*timeo_p);
 	goto out;
 

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Neil Horman]

On Mon, Jul 09, 2018 at 12:11:44PM +0200, Petr Malat wrote:
> Hi!
> I believe signal handling in the connect path is not implemented properly
> as connect() terminates with EINPROGRESS instead of being restarted after
> the signal handler returns (see attached sctp_connect_intr_bug.c).
> I went through the POSIX manual pages and it seems EINPROGRESS can be
> returned only if the socket is nonblocking. Linux documentation also
> mention connect() could fail with EINPROGRESS if SO_SNDTIMEO expires (this
> option is not discussed by POSIX manual pages). This is also how TCP
> behaves.
> I was able to solve the issue by freeing the association in the signal
> handling path, see the attached sctp_connect_intr.patch, which also contains
> the rationale for the change.
> Please, review the change as I based it on code comments which may be obsolete
> and I do not have knowledge of SCTP stack internals.
> BR,
>   Petr

I'm not sure what exactly you are trying to solve here.  You can't restart a
connect() system call because there may be packets on the wire that are
implementing the state transitions needed to establish the connection you
requested.  Theres no way for the local system to determine if that state
transition has completed properly yet or not.  Terminating the connection as you
are doing here leads to significant traffic overhead for what might otherwise be
in most cases a successfull connection.  Thats why Linux deviates from the
standard and return EINPROGRESS, and requires that you check the SO_ERROR value
to determine if the connection is successfull (SO_ERROR = 0), or if its still
pending (note in the connect man page that you can select/poll a in-progress
socket for writeability to determine completion of the connection operation).

Sorry, but this doesn't seem like its actually fixing anything. 

Also, in the future, if you want to submit a patch, you'll want to do so using
the patch posting guidelines in the documentation, or it won't get integrated.

Regards
Neil

> #include <sys/socket.h>
> #include <sys/types.h>
> #include <arpa/inet.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <signal.h>
> #include <string.h>
> #include <errno.h>
> #include <stdio.h>
> 
> #define DEAD_IP "127.16.0.1"
> #define fail_if(arg) if (0 != (arg)) { \
> 	fprintf(stderr, "Failed at %d: %s\n", __LINE__, #arg);\
> 	return 1; }
> 
> void handler(int sig)
> {
> 	printf("Got signal %d\n", sig);
> }
> 
> int main(int argc, char *argv[])
> {
> 	struct sockaddr_in addr4 = { .sin_family = AF_INET, .sin_port = htons(5555) };
> 	struct sockaddr *deadaddr = (struct sockaddr *)&addr4;
> 	struct sigaction sa = { .sa_handler = handler, .sa_flags = SA_RESTART }; 
> 	int sk, rtn;
> 
> 	fail_if(1 != inet_pton(AF_INET, DEAD_IP, &addr4.sin_addr));
> 
> 	fail_if(system("iptables -I OUTPUT -d " DEAD_IP "/32 -j DROP"));
> 
> 	fail_if(0 > (sk = socket(PF_INET, SOCK_SEQPACKET, IPPROTO_SCTP)));
> 	fail_if(sigaction(SIGALRM, &sa, NULL));
> 	fail_if(alarm(3) < 0);
> 
> 	rtn = connect(sk, deadaddr, sizeof *deadaddr);
> 
> 	// The call should either resume the operation of fail with EINTR
> 	printf("rtn: %d errno: %d (%s)\n", rtn, errno, strerror(errno));
> 
> 	fail_if(system("iptables -D OUTPUT -d " DEAD_IP "/32 -j DROP"));
> 
> 	return 0;
> }

> sctp: Free connecting association if there is a pending signal
> 
> When a connection attempt is interrupted by a signal, there are two
> possibilities how the process can continue:
>  - There has been a timeout specified through SO_SNDTIMEO, in that case
>    the call must terminate the association and return -EINTR, because
>    it's not possible to restart the call safely as the timeout is
>    not an argument of the connect or SCTP_SOCKOPT_CONNECTX call.
>  - If the connect timeout has not been limited, it's possible to restart
>    the call, however the association must be freed as well to make that
>    possible. Theoretically, one could pick up the ongoing association in
>    the restart path, but the problem is we can't esily detect if
>    the syscall restarting wasn't disabled by the user.
> As a result, it's sensible to free the association independently if the
> connect attempt is being restarted or not.
> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> --- linux-4.18-rc3.orig/net/sctp/socket.c	2018-07-09 11:46:51.991717457 +0200
> +++ linux-4.18-rc3/net/sctp/socket.c	2018-07-05 15:17:29.292657471 +0200
> @@ -8456,6 +8456,7 @@ do_error:
>  	goto out;
>  
>  do_interrupted:
> +	sctp_association_free(asoc);
>  	err = sock_intr_errno(*timeo_p);
>  	goto out;
>  

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Petr Malat]

On Mon, Jul 09, 2018 at 08:15:12AM -0400, Neil Horman wrote:
> You can't restart a connect() system call because there may be packets on the
> wire that are implementing the state transitions needed to establish the 
> connection you requested.  Theres no way for the local system to determine if
> that state transition has completed properly yet or not.  Terminating the 
> connection as you are doing here leads to significant traffic overhead for what
> might otherwise be in most cases a successfull connection.
This is questionable as the reason for sending the signal can be a request to
terminate the connection attempt.

> Thats why Linux deviates from the
> standard and return EINPROGRESS, and requires that you check the SO_ERROR value
> to determine if the connection is successfull (SO_ERROR = 0), or if its still
> pending (note in the connect man page that you can select/poll a in-progress
> socket for writeability to determine completion of the connection operation).
It's not a generic Linux problem as TCP doesn't suffer from this discrepancy, it
exists only in linux-sctp. Also, this behavior is not documented anywhere.

Current manual page in lksctp git explicitly says EINPROGRESS and EALREADY can
be returned on non-blocking sockets only:

EINPROGRESS
The socket is non-blocking and the connection cannot be completed immediately.

If you think this behavior is desired, then the manual page should be updated
and say that EINPROGRESS and EALREADY can be also returned on a blocking socket
if a signal was handled in the mean time.

> Sorry, but this doesn't seem like its actually fixing anything.
It's actually fixing several usecases:
 1) SOCK_STREAM application uses alarm() to set a connection timeout (use of 
    SO_SNDTIMEO is not portable) and expects the connection attempt to be 
    terminated after the alarm is delivered. This applies mainly on ported
    TCP programs.
    These applications expect EINTR in a case of timeout, not EINPROGRESS.
 2) SOCK_SEQPACKET application uses signals to implement different connect
    timeouts for different peers as usage of SO_SNDTIMEO is not thread safe.
 3) Developer or administrator attaches gdb/strace to a process blocked in
    connect(), the application aborts as it doesn't expect to get EINPROGRESS on 
    a blocking socket.

To sum it up, the current documention and code are not aligned and one of them needs
to be fixed.
BR,
  Petr

@Neil: Sorry for the double post, I forgot to include CC addresses.

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Neil Horman]

On Mon, Jul 09, 2018 at 05:37:58PM +0200, Petr Malat wrote:
> On Mon, Jul 09, 2018 at 08:15:12AM -0400, Neil Horman wrote:
> > You can't restart a connect() system call because there may be packets on the
> > wire that are implementing the state transitions needed to establish the 
> > connection you requested.  Theres no way for the local system to determine if
> > that state transition has completed properly yet or not.  Terminating the 
> > connection as you are doing here leads to significant traffic overhead for what
> > might otherwise be in most cases a successfull connection.
> This is questionable as the reason for sending the signal can be a request to
> terminate the connection attempt.
> 
That may be, but the kernel has no notion of that.  If thats the intent, then
the correct approach is to catch the signal in question in the application and
close the socket.  That way you get the behvaior you are coding here, and the
application knows whats going on.

> > Thats why Linux deviates from the
> > standard and return EINPROGRESS, and requires that you check the SO_ERROR value
> > to determine if the connection is successfull (SO_ERROR = 0), or if its still
> > pending (note in the connect man page that you can select/poll a in-progress
> > socket for writeability to determine completion of the connection operation).
> It's not a generic Linux problem as TCP doesn't suffer from this discrepancy, it
> exists only in linux-sctp. Also, this behavior is not documented anywhere.
> 
That doesn't seem to be the case to me.  If you look at the tcp_v4_connect
path its pretty clear that a non-blocking socket will return EINPROGRESS if the
connect operation is interrupted.

> Current manual page in lksctp git explicitly says EINPROGRESS and EALREADY can
> be returned on non-blocking sockets only:
> 
> EINPROGRESS
> The socket is non-blocking and the connection cannot be completed immediately.
> 
> If you think this behavior is desired, then the manual page should be updated
> and say that EINPROGRESS and EALREADY can be also returned on a blocking socket
> if a signal was handled in the mean time.
> 
> > Sorry, but this doesn't seem like its actually fixing anything.
> It's actually fixing several usecases:
>  1) SOCK_STREAM application uses alarm() to set a connection timeout (use of 
>     SO_SNDTIMEO is not portable) and expects the connection attempt to be 
>     terminated after the alarm is delivered. This applies mainly on ported
>     TCP programs.
>     These applications expect EINTR in a case of timeout, not EINPROGRESS.
>  2) SOCK_SEQPACKET application uses signals to implement different connect
>     timeouts for different peers as usage of SO_SNDTIMEO is not thread safe.
>  3) Developer or administrator attaches gdb/strace to a process blocked in
>     connect(), the application aborts as it doesn't expect to get EINPROGRESS on 
>     a blocking socket.
> 
> To sum it up, the current documention and code are not aligned and one of them needs
> to be fixed.
Based on your description, it sounds like the behavior is actually correct for
non-blocking sockets.  The real problem is that a blocking socket is behaving
like a non-blocking socket, which does seem like a legitimate issue.
sctp_wait_for_connect will return EINPROGESS if the timeo pointer points to a
value that is zero.  I would think the best course of action is to figure out
why a socket might have that timeout set that way if its not non-blocking

Neil

> BR,
>   Petr
> 
> @Neil: Sorry for the double post, I forgot to include CC addresses.
No worries, I think I responded to your other note, so I'm retyping here

> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Petr Malat]

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

Hi!
On Mon, Jul 09, 2018 at 03:08:37PM -0400, Neil Horman wrote:
> > This is questionable as the reason for sending the signal can be a request to
> > terminate the connection attempt.
> > 
> That may be, but the kernel has no notion of that.  If thats the intent, then
> the correct approach is to catch the signal in question in the application and
> close the socket.  That way you get the behvaior you are coding here, and the
> application knows whats going on.
Yes, that's another possible way how it could be implemented, however it
it's not possible to close only one association as the association number
is not known to the user at that time (which is not a problem for former
TCP apps).

> > > Thats why Linux deviates from the
> > > standard and return EINPROGRESS, and requires that you check the SO_ERROR value
> > > to determine if the connection is successfull (SO_ERROR == 0), or if its still
> > > pending (note in the connect man page that you can select/poll a in-progress
> > > socket for writeability to determine completion of the connection operation).
> > It's not a generic Linux problem as TCP doesn't suffer from this discrepancy, it
> > exists only in linux-sctp. Also, this behavior is not documented anywhere.
> > 
> That doesn't seem to be the case to me.  If you look at the tcp_v4_connect
> path its pretty clear that a non-blocking socket will return EINPROGRESS if the
> connect operation is interrupted.
I have extended the test to make it clear what I complain about - it's
the first case, but not the only case where the behavior differs from
TCP:

$ ./a.out | ts "%H:%M:%S" # Empty lines added for readability
10:45:46 Testing IPPROTO_SCTP on a blocking socket
10:45:49   1st connect - rtn: -1 errno: 114 (Operation already in progress)
10:45:49   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
10:45:49 Testing IPPROTO_TCP on a blocking socket
10:46:52   1st connect - rtn: -1 errno: 110 (Connection timed out)
10:47:55   2nd connect - rtn: -1 errno: 110 (Connection timed out)

10:47:55 Testing IPPROTO_SCTP on a timeo blocking socket
10:47:58   1st connect - rtn: -1 errno: 4 (Interrupted system call)
10:47:58   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
10:47:58 Testing IPPROTO_TCP on a timeo blocking socket
10:48:01   1st connect - rtn: -1 errno: 4 (Interrupted system call)
10:49:01   2nd connect - rtn: -1 errno: 110 (Connection timed out)

10:49:01 Testing IPPROTO_SCTP on a non-blocking socket
10:49:01   1st connect - rtn: -1 errno: 115 (Operation now in progress)
10:49:01   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
10:49:01 Testing IPPROTO_TCP on a non-blocking socket
10:49:01   1st connect - rtn: -1 errno: 115 (Operation now in progress)
10:49:01   2nd connect - rtn: -1 errno: 114 (Operation already in progress)

10:49:01 Testing IPPROTO_SCTP on a timeo blocking socket
10:49:04   1st connect - rtn: -1 errno: 4 (Interrupted system call)
10:49:04   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
10:49:04 Testing IPPROTO_TCP on a timeo blocking socket
10:49:07   1st connect - rtn: -1 errno: 4 (Interrupted system call)
10:50:07   2nd connect - rtn: -1 errno: 110 (Connection timed out)

All these differences except the first one are not violating what one
can read in the documentation as nothing explicitly says if a signal 
delivery on a non-blocking or sndtimeo socket should terminate the ongoing
connection attempt, but the first case is clearly not aligned with the
documentation, which says EINPROGRESS arrives only on non-blocking and
non-timeo sockets.
 
> > To sum it up, the current documention and code are not aligned and one of them needs
> > to be fixed.
> Based on your description, it sounds like the behavior is actually correct for
> non-blocking sockets.  The real problem is that a blocking socket is behaving
> like a non-blocking socket, which does seem like a legitimate issue.
> sctp_wait_for_connect will return EINPROGESS if the timeo pointer points to a
> value that is zero.  I would think the best course of action is to figure out
> why a socket might have that timeout set that way if its not non-blocking
I can look further and try to  change the patch to free the association
only for blocking sockets, but that leads me to a question what is
actually the intended behavior for these cases, should SCTP be as close
to TCP as possible (to make s/IPPROTO_TCP/IPPROTO_SCTP/ working for
most applications)?
BR,
  Petr

[-- Attachment #2: test_all_cases.c --]
[-- Type: text/x-csrc, Size: 1889 bytes --]

#include <sys/socket.h>
#include <sys/types.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>
#include <errno.h>
#include <stdio.h>

#define DEAD_IP "127.16.0.1"
#define fail_if(arg) if (0 != (arg)) { \
	fprintf(stderr, "Failed at %d: %s\n", __LINE__, #arg);\
	return 1; };

void handler(int sig)
{
	// printf("Got signal %d\n", sig);
}

int _test(int proto, int timeo, int blocking)
{
	struct sockaddr_in addr4 = { .sin_family = AF_INET, .sin_port = htons(5555) };
	struct sockaddr *deadaddr = (struct sockaddr *)&addr4;
	struct sigaction sa = { .sa_handler = handler, .sa_flags = SA_RESTART }; 
	struct timeval tv = { .tv_sec = 300 };
	int sk, rtn;

	fail_if(1 != inet_pton(AF_INET, DEAD_IP, &addr4.sin_addr));
	fail_if(0 > (sk = socket(PF_INET,
			SOCK_STREAM | (blocking ? 0 : SOCK_NONBLOCK), proto)));
	if (timeo) {
		fail_if(setsockopt(sk, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof tv));
	}
	fail_if(sigaction(SIGALRM, &sa, NULL));
	fail_if(alarm(3) < 0);

	rtn = connect(sk, deadaddr, sizeof *deadaddr);

	printf("  1st connect - rtn: %d errno: %d (%s)\n", rtn, errno, strerror(errno));
	rtn = connect(sk, deadaddr, sizeof *deadaddr);
	printf("  2nd connect - rtn: %d errno: %d (%s)\n", rtn, errno, strerror(errno));

	close(sk);
	return 0;
}

#define test(proto, timeo, blocking) \
	(printf("Testing " #proto " on a %s%sblocking socket\n", \
		timeo ? "timeo " : "", blocking ? "" : "non-"), \
		_test(proto, timeo, blocking))

int main(int argc, char *argv[])
{
	setlinebuf(stdout);
	fail_if(system("iptables -I OUTPUT -d " DEAD_IP "/32 -j DROP"));
	test(IPPROTO_SCTP, 0, 1);
	test(IPPROTO_TCP, 0, 1);
	test(IPPROTO_SCTP, 1, 1);
	test(IPPROTO_TCP, 1, 1);
	test(IPPROTO_SCTP, 0, 0);
	test(IPPROTO_TCP, 0, 0);
	test(IPPROTO_SCTP, 1, 1);
	test(IPPROTO_TCP, 1, 1);
	fail_if(system("iptables -D OUTPUT -d " DEAD_IP "/32 -j DROP"));
}

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Neil Horman]

On Wed, Jul 11, 2018 at 11:12:30AM +0200, Petr Malat wrote:
> Hi!
> On Mon, Jul 09, 2018 at 03:08:37PM -0400, Neil Horman wrote:
> > > This is questionable as the reason for sending the signal can be a request to
> > > terminate the connection attempt.
> > > 
> > That may be, but the kernel has no notion of that.  If thats the intent, then
> > the correct approach is to catch the signal in question in the application and
> > close the socket.  That way you get the behvaior you are coding here, and the
> > application knows whats going on.
> Yes, that's another possible way how it could be implemented, however it
> it's not possible to close only one association as the association number
> is not known to the user at that time (which is not a problem for former
> TCP apps).
> 
Its not possible to do that anyway with a 1 to many style socket.  If you want
to know how to do that you need to do one of the following:

1) Use the peeloff api to map each association to a socket id, so you can just
close it when a connection error occurs.

2) Use getpaddrs to identify all your active connections so that you can
identify the new association id if one fails, peel it off and close it

> > > > Thats why Linux deviates from the
> > > > standard and return EINPROGRESS, and requires that you check the SO_ERROR value
> > > > to determine if the connection is successfull (SO_ERROR = 0), or if its still
> > > > pending (note in the connect man page that you can select/poll a in-progress
> > > > socket for writeability to determine completion of the connection operation).
> > > It's not a generic Linux problem as TCP doesn't suffer from this discrepancy, it
> > > exists only in linux-sctp. Also, this behavior is not documented anywhere.
> > > 
> > That doesn't seem to be the case to me.  If you look at the tcp_v4_connect
> > path its pretty clear that a non-blocking socket will return EINPROGRESS if the
> > connect operation is interrupted.
> I have extended the test to make it clear what I complain about - it's
> the first case, but not the only case where the behavior differs from
> TCP:
SCTP is not TCP, though I suppose thats not overly relevant here.

> 
> $ ./a.out | ts "%H:%M:%S" # Empty lines added for readability
> 10:45:46 Testing IPPROTO_SCTP on a blocking socket
> 10:45:49   1st connect - rtn: -1 errno: 114 (Operation already in progress)
> 10:45:49   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> 10:45:49 Testing IPPROTO_TCP on a blocking socket
> 10:46:52   1st connect - rtn: -1 errno: 110 (Connection timed out)
> 10:47:55   2nd connect - rtn: -1 errno: 110 (Connection timed out)
> 
This is the origional case, and I agree that it seems like SCTP is acting as a
non-blocking socket here (I presume it returns EINPROGRESS immediately, rather
than after some timeout value)?

> 10:47:55 Testing IPPROTO_SCTP on a timeo blocking socket
> 10:47:58   1st connect - rtn: -1 errno: 4 (Interrupted system call)
> 10:47:58   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> 10:47:58 Testing IPPROTO_TCP on a timeo blocking socket
> 10:48:01   1st connect - rtn: -1 errno: 4 (Interrupted system call)
> 10:49:01   2nd connect - rtn: -1 errno: 110 (Connection timed out)
> 
This seems legitimate to me, in that SCTP, on the second call indicated
EINPROGRESS, just like the sctp_connectx man page documents

> 10:49:01 Testing IPPROTO_SCTP on a non-blocking socket
> 10:49:01   1st connect - rtn: -1 errno: 115 (Operation now in progress)
> 10:49:01   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> 10:49:01 Testing IPPROTO_TCP on a non-blocking socket
> 10:49:01   1st connect - rtn: -1 errno: 115 (Operation now in progress)
> 10:49:01   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> 
This matches, and looks fine

> 10:49:01 Testing IPPROTO_SCTP on a timeo blocking socket
> 10:49:04   1st connect - rtn: -1 errno: 4 (Interrupted system call)
> 10:49:04   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> 10:49:04 Testing IPPROTO_TCP on a timeo blocking socket
> 10:49:07   1st connect - rtn: -1 errno: 4 (Interrupted system call)
> 10:50:07   2nd connect - rtn: -1 errno: 110 (Connection timed out)
> 
This is a duplicate of case 2 above.

> All these differences except the first one are not violating what one
> can read in the documentation as nothing explicitly says if a signal 
> delivery on a non-blocking or sndtimeo socket should terminate the ongoing
> connection attempt, but the first case is clearly not aligned with the
> documentation, which says EINPROGRESS arrives only on non-blocking and
> non-timeo sockets.
>  
Right, as we discussed in previous notes, it appears that the sctp socket is
behaving like a non-blocking socket when it should block until the operation is
complete.

> > > To sum it up, the current documention and code are not aligned and one of them needs
> > > to be fixed.
> > Based on your description, it sounds like the behavior is actually correct for
> > non-blocking sockets.  The real problem is that a blocking socket is behaving
> > like a non-blocking socket, which does seem like a legitimate issue.
> > sctp_wait_for_connect will return EINPROGESS if the timeo pointer points to a
> > value that is zero.  I would think the best course of action is to figure out
> > why a socket might have that timeout set that way if its not non-blocking
> I can look further and try to  change the patch to free the association
> only for blocking sockets, but that leads me to a question what is
> actually the intended behavior for these cases, should SCTP be as close
> to TCP as possible (to make s/IPPROTO_TCP/IPPROTO_SCTP/ working for
> most applications)?
> BR,
>   Petr
Again, the only thing that I see as wrong here is that sctp is behaving like a
non blocking socket in a blocking case.  Thats what needs to be fixed.  If there
is an association leak because of that, that should be fixed as well, but I
don't see that there should be one.  The association holds/puts look balanced,
and the freeing should happen appropriately already, we just need to make the
call path block until the connection times out.

> #include <sys/socket.h>
> #include <sys/types.h>
> #include <arpa/inet.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <signal.h>
> #include <string.h>
> #include <errno.h>
> #include <stdio.h>
> 
> #define DEAD_IP "127.16.0.1"
> #define fail_if(arg) if (0 != (arg)) { \
> 	fprintf(stderr, "Failed at %d: %s\n", __LINE__, #arg);\
> 	return 1; };
> 
> void handler(int sig)
> {
> 	// printf("Got signal %d\n", sig);
> }
> 
> int _test(int proto, int timeo, int blocking)
> {
> 	struct sockaddr_in addr4 = { .sin_family = AF_INET, .sin_port = htons(5555) };
> 	struct sockaddr *deadaddr = (struct sockaddr *)&addr4;
> 	struct sigaction sa = { .sa_handler = handler, .sa_flags = SA_RESTART }; 
> 	struct timeval tv = { .tv_sec = 300 };
> 	int sk, rtn;
> 
> 	fail_if(1 != inet_pton(AF_INET, DEAD_IP, &addr4.sin_addr));
> 	fail_if(0 > (sk = socket(PF_INET,
> 			SOCK_STREAM | (blocking ? 0 : SOCK_NONBLOCK), proto)));
> 	if (timeo) {
> 		fail_if(setsockopt(sk, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof tv));
> 	}
> 	fail_if(sigaction(SIGALRM, &sa, NULL));
> 	fail_if(alarm(3) < 0);
> 
> 	rtn = connect(sk, deadaddr, sizeof *deadaddr);
> 
> 	printf("  1st connect - rtn: %d errno: %d (%s)\n", rtn, errno, strerror(errno));
> 	rtn = connect(sk, deadaddr, sizeof *deadaddr);
> 	printf("  2nd connect - rtn: %d errno: %d (%s)\n", rtn, errno, strerror(errno));
> 
> 	close(sk);
> 	return 0;
> }
> 
> #define test(proto, timeo, blocking) \
> 	(printf("Testing " #proto " on a %s%sblocking socket\n", \
> 		timeo ? "timeo " : "", blocking ? "" : "non-"), \
> 		_test(proto, timeo, blocking))
> 
> int main(int argc, char *argv[])
> {
> 	setlinebuf(stdout);
> 	fail_if(system("iptables -I OUTPUT -d " DEAD_IP "/32 -j DROP"));
> 	test(IPPROTO_SCTP, 0, 1);
> 	test(IPPROTO_TCP, 0, 1);
> 	test(IPPROTO_SCTP, 1, 1);
> 	test(IPPROTO_TCP, 1, 1);
> 	test(IPPROTO_SCTP, 0, 0);
> 	test(IPPROTO_TCP, 0, 0);
> 	test(IPPROTO_SCTP, 1, 1);
> 	test(IPPROTO_TCP, 1, 1);
> 	fail_if(system("iptables -D OUTPUT -d " DEAD_IP "/32 -j DROP"));
> }

RE: [PATCH net] sctp: Free connecting association if there is a pending signal

[David Laight]

From: Petr Malat
> Sent: 11 July 2018 10:13
...
> I can look further and try to  change the patch to free the association
> only for blocking sockets, but that leads me to a question what is
> actually the intended behavior for these cases, should SCTP be as close
> to TCP as possible (to make s/IPPROTO_TCP/IPPROTO_SCTP/ working for
> most applications)?

Most TCP applications will fail to work properly over SCTP for
other reasons.

At least one problem is that unless the application enables receipt
of 'sctp_association_event' it won't be told about connection
restarts and will get very confused by the missing data.

It is also worth noting that TCP is likely to be considerably
faster than SCTP.
Partially because the SCTP code is significantly more complicated,
and partially because it is difficult for an application to get
multiple data chunks into a single ethernet frame (unless the
traffic pattern allows Nagle be enabled).

Even some of the protocols which (I think) were co-developed with
SCTP (I'm thinking about SIGTRAN M3UA and M2PA) have mis-understandings
about the way SCTP behaves and probably work better over TCP!

	David

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Petr Malat]

Hi!
> > $ ./a.out | ts "%H:%M:%S" # Empty lines added for readability
> > 10:45:46 Testing IPPROTO_SCTP on a blocking socket
> > 10:45:49   1st connect - rtn: -1 errno: 114 (Operation already in progress)
> > 10:45:49   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> > 10:45:49 Testing IPPROTO_TCP on a blocking socket
> > 10:46:52   1st connect - rtn: -1 errno: 110 (Connection timed out)
> > 10:47:55   2nd connect - rtn: -1 errno: 110 (Connection timed out)
> > 
> This is the origional case, and I agree that it seems like SCTP is acting as a
> non-blocking socket here (I presume it returns EINPROGRESS immediately, rather
> than after some timeout value)?
It returns after a signal is handled - in my test the signal is
generated by alarm, but it doesn't matter what is the source of
the signal.
The blocking socket can be easilly identified by the return value
from sock_intr_errno, which is ERESTARTSYS for a blocking operation
and EINTR for a non-blocking one.
I can change my patch to free the association only if ERESTARTSYS
is returned. What do you think about it?
  Petr

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Neil Horman]

On Wed, Jul 11, 2018 at 07:20:02PM +0200, Petr Malat wrote:
> Hi!
> > > $ ./a.out | ts "%H:%M:%S" # Empty lines added for readability
> > > 10:45:46 Testing IPPROTO_SCTP on a blocking socket
> > > 10:45:49   1st connect - rtn: -1 errno: 114 (Operation already in progress)
> > > 10:45:49   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> > > 10:45:49 Testing IPPROTO_TCP on a blocking socket
> > > 10:46:52   1st connect - rtn: -1 errno: 110 (Connection timed out)
> > > 10:47:55   2nd connect - rtn: -1 errno: 110 (Connection timed out)
> > > 
> > This is the origional case, and I agree that it seems like SCTP is acting as a
> > non-blocking socket here (I presume it returns EINPROGRESS immediately, rather
> > than after some timeout value)?
> It returns after a signal is handled - in my test the signal is
> generated by alarm, but it doesn't matter what is the source of
> the signal.
> The blocking socket can be easilly identified by the return value
> from sock_intr_errno, which is ERESTARTSYS for a blocking operation
> and EINTR for a non-blocking one.
> I can change my patch to free the association only if ERESTARTSYS
> is returned. What do you think about it?
>   Petr
> 
No, I don't think we should be freeing the association without taking direction
from user space here, as I noted before.  
Neil

Re: [PATCH net] sctp: Free connecting association if there is a pending signal

[Petr Malat]

On Wed, Jul 11, 2018 at 07:43:39PM -0400, Neil Horman wrote:
> > > > $ ./a.out | ts "%H:%M:%S" # Empty lines added for readability
> > > > 10:45:46 Testing IPPROTO_SCTP on a blocking socket
> > > > 10:45:49   1st connect - rtn: -1 errno: 114 (Operation already in progress)
> > > > 10:45:49   2nd connect - rtn: -1 errno: 114 (Operation already in progress)
> > > > 10:45:49 Testing IPPROTO_TCP on a blocking socket
> > > > 10:46:52   1st connect - rtn: -1 errno: 110 (Connection timed out)
> > > > 10:47:55   2nd connect - rtn: -1 errno: 110 (Connection timed out)
> > > > 
> > > This is the origional case, and I agree that it seems like SCTP is acting as a
> > > non-blocking socket here (I presume it returns EINPROGRESS immediately, rather
> > > than after some timeout value)?
> > It returns after a signal is handled - in my test the signal is
> > generated by alarm, but it doesn't matter what is the source of
> > the signal.
> > The blocking socket can be easilly identified by the return value
> > from sock_intr_errno, which is ERESTARTSYS for a blocking operation
> > and EINTR for a non-blocking one.
> > I can change my patch to free the association only if ERESTARTSYS
> > is returned. What do you think about it?
> No, I don't think we should be freeing the association without taking direction
> from user space here, as I noted before.  
How else it could be fixed then? I think freeing the association is the
only possible option on blocking sockets, as applications using blocking
sockets do not expect the connection attempt to continue on the 
background.
Theoretically, it could be optimized to free the association only if
SA_RESTART is not enabled on that socket and try to pick it and restore
waiting in the other case.
  Petr