diff mbox series

syscalls/setsockopt05: associate receiver with destination address

Message ID c73f6b2e1232d6a892ecef76370ea2cf6c7dd044.1598270814.git.jstancek@redhat.com
State Superseded
Headers show
Series syscalls/setsockopt05: associate receiver with destination address | expand

Commit Message

Jan Stancek Aug. 24, 2020, 12:10 p.m. UTC 
to avoid sporadic ECONNREFUSED errors:
  safe_net.c:202: BROK: setsockopt05.c:70: send(6, 0x3ffcaf7d828, 4000, 32768) failed: ECONNREFUSED (111)

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/setsockopt/setsockopt05.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Martin Doucha Aug. 24, 2020, 2:42 p.m. UTC | #1 
Hi,

On 24. 08. 20 14:10, Jan Stancek wrote:
> to avoid sporadic ECONNREFUSED errors:
>   safe_net.c:202: BROK: setsockopt05.c:70: send(6, 0x3ffcaf7d828, 4000, 32768) failed: ECONNREFUSED (111)
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/syscalls/setsockopt/setsockopt05.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> index e78ef236e337..469e5a64bf71 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> @@ -37,6 +37,7 @@ static void setup(void)
>  	int real_uid = getuid();
>  	int real_gid = getgid();
>  	int sock;
> +	int port = TST_GET_UNUSED_PORT(AF_INET, SOCK_DGRAM);
>  	struct ifreq ifr;
>  
>  	SAFE_UNSHARE(CLONE_NEWUSER);
> @@ -45,14 +46,14 @@ static void setup(void)
>  	SAFE_FILE_PRINTF("/proc/self/uid_map", "0 %d 1", real_uid);
>  	SAFE_FILE_PRINTF("/proc/self/gid_map", "0 %d 1", real_gid);
>  
> -	tst_init_sockaddr_inet_bin(&addr, INADDR_LOOPBACK, 12345);
> +	tst_init_sockaddr_inet_bin(&addr, INADDR_LOOPBACK, port);

Please don't use TST_GET_UNUSED_PORT() this way. The correct way to do
this is to set port to 0 and then read the address back using
SAFE_GETSOCKNAME() after bind(). It's the same amount of code but
without any race conditions.

>  	sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
>  	strcpy(ifr.ifr_name, "lo");
>  	ifr.ifr_mtu = 1500;
>  	SAFE_IOCTL(sock, SIOCSIFMTU, &ifr);
>  	ifr.ifr_flags = IFF_UP;
>  	SAFE_IOCTL(sock, SIOCSIFFLAGS, &ifr);
> -	SAFE_CLOSE(sock);

Don't forget to close the socket in cleanup().

> +	SAFE_BIND(sock, (struct sockaddr *)&addr, sizeof(struct sockaddr));
>  }
>  
>  static void run(void)
> 

Though I wonder whether setsockopt(SO_NO_CHECK, 1) is really supposed to
flush the partial packet. Are you sure it's not a bug in the kernel?
Jan Stancek Aug. 24, 2020, 3:01 p.m. UTC | #2 
----- Original Message -----
> Hi,
> 
> On 24. 08. 20 14:10, Jan Stancek wrote:
> > to avoid sporadic ECONNREFUSED errors:
> >   safe_net.c:202: BROK: setsockopt05.c:70: send(6, 0x3ffcaf7d828, 4000,
> >   32768) failed: ECONNREFUSED (111)
> > 
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  testcases/kernel/syscalls/setsockopt/setsockopt05.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> > b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> > index e78ef236e337..469e5a64bf71 100644
> > --- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> > +++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> > @@ -37,6 +37,7 @@ static void setup(void)
> >  	int real_uid = getuid();
> >  	int real_gid = getgid();
> >  	int sock;
> > +	int port = TST_GET_UNUSED_PORT(AF_INET, SOCK_DGRAM);
> >  	struct ifreq ifr;
> >  
> >  	SAFE_UNSHARE(CLONE_NEWUSER);
> > @@ -45,14 +46,14 @@ static void setup(void)
> >  	SAFE_FILE_PRINTF("/proc/self/uid_map", "0 %d 1", real_uid);
> >  	SAFE_FILE_PRINTF("/proc/self/gid_map", "0 %d 1", real_gid);
> >  
> > -	tst_init_sockaddr_inet_bin(&addr, INADDR_LOOPBACK, 12345);
> > +	tst_init_sockaddr_inet_bin(&addr, INADDR_LOOPBACK, port);
> 
> Please don't use TST_GET_UNUSED_PORT() this way. The correct way to do
> this is to set port to 0 and then read the address back using
> SAFE_GETSOCKNAME() after bind(). It's the same amount of code but
> without any race conditions.

Fair point.

> 
> >  	sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
> >  	strcpy(ifr.ifr_name, "lo");
> >  	ifr.ifr_mtu = 1500;
> >  	SAFE_IOCTL(sock, SIOCSIFMTU, &ifr);
> >  	ifr.ifr_flags = IFF_UP;
> >  	SAFE_IOCTL(sock, SIOCSIFFLAGS, &ifr);
> > -	SAFE_CLOSE(sock);
> 
> Don't forget to close the socket in cleanup().
> 
> > +	SAFE_BIND(sock, (struct sockaddr *)&addr, sizeof(struct sockaddr));
> >  }
> >  
> >  static void run(void)
> > 
> 
> Though I wonder whether setsockopt(SO_NO_CHECK, 1) is really supposed to
> flush the partial packet. Are you sure it's not a bug in the kernel?

I assumed it's from previous one, not partial one. From man(7) udp:

 ECONNREFUSED
              No receiver was associated with the destination address.
              This might be caused by a previous packet sent over the socket.
Martin Doucha Aug. 24, 2020, 3:21 p.m. UTC | #3 
On 24. 08. 20 17:01, Jan Stancek wrote:
> I assumed it's from previous one, not partial one. From man(7) udp:
> 
>  ECONNREFUSED
>               No receiver was associated with the destination address.
>               This might be caused by a previous packet sent over the socket.
> 

Wait, looking closer at the error message, it looks like send(MSG_MORE)
is broken on your test kernel. Let me quote the man page here:

>        MSG_MORE (since Linux 2.4.4)
>               The  caller  has  more data to send.  This flag is used with TCP
>               sockets to obtain the same effect as the TCP_CORK socket  option
>               (see tcp(7)), with the difference that this flag can be set on a
>               per-call basis.
> 
>               Since Linux 2.6, this flag is also supported  for  UDP  sockets,
>               and  informs the kernel to package all of the data sent in calls
>               with this flag set into a single datagram which  is  transmitted
>               only  when  a call is performed that does not specify this flag.
>               (See also the UDP_CORK socket option described in udp(7).)

The data from the SAFE_SEND() call are supposed to stay in kernel buffer
until the send() call where we (intentionally) ignore the return value.
Jan Stancek Aug. 24, 2020, 4:11 p.m. UTC | #4 
----- Original Message -----
> On 24. 08. 20 17:01, Jan Stancek wrote:
> > I assumed it's from previous one, not partial one. From man(7) udp:
> > 
> >  ECONNREFUSED
> >               No receiver was associated with the destination address.
> >               This might be caused by a previous packet sent over the
> >               socket.
> > 
> 
> Wait, looking closer at the error message, it looks like send(MSG_MORE)
> is broken on your test kernel. Let me quote the man page here:

Aren't we getting propagated ICMP error to send() from previous iteration
of the loop?

Per https://tools.ietf.org/html/rfc1122#page-78
  The application is also responsible to avoid confusion from a delayed ICMP
  error message resulting from an earlier use of the same port(s).
Martin Doucha Aug. 25, 2020, 9:43 a.m. UTC | #5 
On 24. 08. 20 18:11, Jan Stancek wrote:
> Aren't we getting propagated ICMP error to send() from previous iteration
> of the loop?
> 
> Per https://tools.ietf.org/html/rfc1122#page-78
>   The application is also responsible to avoid confusion from a delayed ICMP
>   error message resulting from an earlier use of the same port(s).

The test is using the loopback address. There should be no delay. We're
running this test daily on 7 different kernel versions and 4 different
archs and I've never seen it fail with ECONNREFUSED, ever.

But you can prove me wrong by adding a few debug prints around both
send() calls.
Jan Stancek Aug. 25, 2020, 1:38 p.m. UTC | #6 
----- Original Message -----
> On 24. 08. 20 18:11, Jan Stancek wrote:
> > Aren't we getting propagated ICMP error to send() from previous iteration
> > of the loop?
> > 
> > Per https://tools.ietf.org/html/rfc1122#page-78
> >   The application is also responsible to avoid confusion from a delayed
> >   ICMP
> >   error message resulting from an earlier use of the same port(s).
> 
> The test is using the loopback address. There should be no delay. We're
> running this test daily on 7 different kernel versions and 4 different
> archs and I've never seen it fail with ECONNREFUSED, ever.


Similar here, it runs daily dozens of times, but appears to fail sporadically
only on s390x.

> 
> But you can prove me wrong by adding a few debug prints around both
> send() calls.

1. extra prints
 65         for (i = 0; i < 1000; i++) {
 66                 sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
 67                 SAFE_CONNECT(sock, (struct sockaddr *)&addr, sizeof(addr));
 68                 printf("1 %d\n", i);
 69                 SAFE_SEND(1, sock, buf, BUFSIZE, MSG_MORE);
 70                 printf("2 %d\n", i);
 71                 SAFE_SETSOCKOPT_INT(sock, SOL_SOCKET, SO_NO_CHECK, 1);
 72                 printf("3 %d\n", i);
 73                 send(sock, buf, 1, 0);
 74                 printf("4 %d\n", i);
 75                 SAFE_CLOSE(sock);

1 246
2 246
3 246
4 246
1 247
2 247
3 247
4 247
1 248

safe_net.c:202: BROK: setsockopt05.c:69: send(3, 0x3ffc397db88, 4000, 32768) failed: ECONNREFUSED (111)

2. SO_NO_CHECK doesn't matter, following fails:
                sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
                SAFE_CONNECT(sock, (struct sockaddr *)&addr, sizeof(addr));
                SAFE_SEND(1, sock, buf, BUFSIZE, MSG_MORE);
                send(sock, buf, 1, 0);
                SAFE_CLOSE(sock);

3. MSG_MORE doesn't matter, following fails:
                sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
                SAFE_CONNECT(sock, (struct sockaddr *)&addr, sizeof(addr));
                SAFE_SEND(1, sock, buf, BUFSIZE, 0);
                SAFE_CLOSE(sock);

Regards,
Jan
Martin Doucha Aug. 26, 2020, 10:08 a.m. UTC | #7 
Let's see if it's really a delayed ICMP message. Can you reproduce the error
after applying this patch?

In the meantime, I'll write a test for send(MSG_MORE).

---
 testcases/kernel/syscalls/setsockopt/setsockopt05.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
index e78ef236e..594178744 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
@@ -63,6 +63,7 @@ static void run(void)
 	memset(buf, 0x42, BUFSIZE);
 
 	for (i = 0; i < 1000; i++) {
+		addr.sin_port = 12345 + i;
 		sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
 		SAFE_CONNECT(sock, (struct sockaddr *)&addr, sizeof(addr));
 		SAFE_SEND(1, sock, buf, BUFSIZE, MSG_MORE);
Martin Doucha Aug. 26, 2020, 10:20 a.m. UTC | #8 
Sorry, ignore this patch, I meant to add a bind() there to change the
source port...

On 26. 08. 20 12:08, Martin Doucha wrote:
> Let's see if it's really a delayed ICMP message. Can you reproduce the error
> after applying this patch?
> 
> In the meantime, I'll write a test for send(MSG_MORE).
> 
> ---
>  testcases/kernel/syscalls/setsockopt/setsockopt05.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> index e78ef236e..594178744 100644
> --- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> +++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
> @@ -63,6 +63,7 @@ static void run(void)
>  	memset(buf, 0x42, BUFSIZE);
>  
>  	for (i = 0; i < 1000; i++) {
> +		addr.sin_port = 12345 + i;
>  		sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
>  		SAFE_CONNECT(sock, (struct sockaddr *)&addr, sizeof(addr));
>  		SAFE_SEND(1, sock, buf, BUFSIZE, MSG_MORE);
>
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
index e78ef236e337..469e5a64bf71 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
@@ -37,6 +37,7 @@  static void setup(void)
 	int real_uid = getuid();
 	int real_gid = getgid();
 	int sock;
+	int port = TST_GET_UNUSED_PORT(AF_INET, SOCK_DGRAM);
 	struct ifreq ifr;
 
 	SAFE_UNSHARE(CLONE_NEWUSER);
@@ -45,14 +46,14 @@  static void setup(void)
 	SAFE_FILE_PRINTF("/proc/self/uid_map", "0 %d 1", real_uid);
 	SAFE_FILE_PRINTF("/proc/self/gid_map", "0 %d 1", real_gid);
 
-	tst_init_sockaddr_inet_bin(&addr, INADDR_LOOPBACK, 12345);
+	tst_init_sockaddr_inet_bin(&addr, INADDR_LOOPBACK, port);
 	sock = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
 	strcpy(ifr.ifr_name, "lo");
 	ifr.ifr_mtu = 1500;
 	SAFE_IOCTL(sock, SIOCSIFMTU, &ifr);
 	ifr.ifr_flags = IFF_UP;
 	SAFE_IOCTL(sock, SIOCSIFFLAGS, &ifr);
-	SAFE_CLOSE(sock);
+	SAFE_BIND(sock, (struct sockaddr *)&addr, sizeof(struct sockaddr));
 }
 
 static void run(void)