Re: [Bugme-new] [Bug 32832] New: shutdown(2) does not fully shut down socket any more

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Wed, 6 Apr 2011 22:42:38 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=32832

There is a tescase attached to this bugzilla report.

           Summary: shutdown(2) does not fully shut down socket any more
           Product: Networking
           Version: 2.5
    Kernel Version: 2.6.38
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: IPV4
        AssignedTo: shemminger@linux-foundation.org
        ReportedBy: kees@outflux.net
        Regression: Yes


In 2.6.35 and earlier, shutdown(2) will fully remove a socket. This does not
appear to be true any more and is causing software to misbehave.

2.6.35:
$ ./testcase
parent: 5957
before:
tcp        0      0 0.0.0.0:12345           0.0.0.0:*               LISTEN     
after:
child: 5961
$ ./testcase
parent: 6001
before:
tcp        0      0 0.0.0.0:12345           0.0.0.0:*               LISTEN     
after:
child: 6002

2.6.38:
$ ./testcase
parent: 1138
before:
tcp        0      0 0.0.0.0:12345           0.0.0.0:*               LISTEN     
after:
child: 1142
$ ./testcase
bind: Address already in use

The listener doesn't show up in netstat any more, but as long as the child
process is running, the socket is unavailable. It is as if the shutdown(2)
behavior has partially reverted to close(2) behavior (but in the case of using
close(2), the child's socket would remain visible in netstat).

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 12 Apr 2011 16:15:56 -0700

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

Stephen Hemminger forwarded this to the list last week, and Eric
Dumazet is actively working on a fix.

On Tue, 12 Apr 2011 16:17:44 -0700 (PDT)
David Miller [off-list ref] wrote:

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 12 Apr 2011 16:15:56 -0700

quoted

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

Stephen Hemminger forwarded this to the list last week, and Eric
Dumazet is actively working on a fix.

OK.

Please don't forward bugzilla reports!  Instead do a reply-to-all and
add the cc's.  That way, bugzilla will capture the email discussion and
you won't get akpmspammed.

Sigh, kernel bugzilla is a PITA.  Some people do seem to like and
expect it though.

Le mardi 12 avril 2011 à 16:17 -0700, David Miller a écrit :

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 12 Apr 2011 16:15:56 -0700

quoted

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

Stephen Hemminger forwarded this to the list last week, and Eric
Dumazet is actively working on a fix.

I worked on it this week end to discover FreeBSD 8.1 would not allow
several CLOSE sockets bound to same port even with REUSEADDR.

So haproxy claim is a bit wrong (its trick doesnt work on FreeBSD), and
used an undocumented linux feature.

I feel this case is a call for SO_REUSEPORT, eventually.

http://www.unixguide.net/network/socketfaq/4.11.shtml

  SO_REUSEADDR allows your server to bind to an address which is in a
  TIME_WAIT state.  It does not allow more than one server to bind to
  the same address.  It was mentioned that use of this flag can create a
  security risk because another server can bind to a the same port, by
  binding to a specific address as opposed to INADDR_ANY.  The
  SO_REUSEPORT flag allows multiple processes to bind to the same
  address provided all of them use the SO_REUSEPORT option.


Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Le mercredi 13 avril 2011 à 04:55 +0200, Eric Dumazet a écrit :

Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Sorry, I should have mentioned commit id : c191a836a908d1dd6
(tcp: disallow bind() to reuse addr/port)

Le mercredi 13 avril 2011 04:55:27, Eric Dumazet a écrit :

I worked on it this week end to discover FreeBSD 8.1 would not allow
several CLOSE sockets bound to same port even with REUSEADDR.

Just to complete the information, yes it does, but only after a shutdown() 
call. And this is the use case of haproxy, amavisd (quoted in the bugzilla bug 
report), and others.

So haproxy claim is a bit wrong (its trick doesnt work on FreeBSD), and
used an undocumented linux feature.

Both test cases (the one I provided to explain the haproxy issue and the one 
provided by Kees) are not about binding 2 sockets at the same time but binding 
a new socket after the first one has been shutdown.
Sadly this also looks undocumented on FreeBSD (only saw a reference on it in a 
code comment).

Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Agree.

Many thanks for the time you already spent on that.

Le mercredi 13 avril 2011 à 09:06 +0200, Cyril Bonté a écrit :

Le mercredi 13 avril 2011 04:55:27, Eric Dumazet a écrit :

quoted

I worked on it this week end to discover FreeBSD 8.1 would not allow
several CLOSE sockets bound to same port even with REUSEADDR.

Just to complete the information, yes it does, but only after a shutdown() 
call. And this is the use case of haproxy, amavisd (quoted in the bugzilla bug 
report), and others.

Yes, but after a shutdown(), FreeBSD doesnt allow a reuse of the socket.
listen() is not available anymore. Its a bit like an unbind, or a full
close().

Hi Eric, Cyril,

quoted

Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Sorry, I should have mentioned commit id : c191a836a908d1dd6
(tcp: disallow bind() to reuse addr/port)

Cyril's use case looks suspect. I don't think that this is a good
reason for reverting this commit.

I would love to help on adding SO_REUSEPORT option in kernel.

thanks,
Daniel.

From: Daniel Baluta <redacted>
Date: Wed, 13 Apr 2011 14:57:18 +0300

Cyril's use case looks suspect. I don't think that this is a good
reason for reverting this commit.

I complete disagree.

Something that worked perfectly fine, probably for years, we broke.

We simply cannot do that, especially since we do not have a reasonable
alternative at this time.

Adding SO_REUSEPORT is a long range option, and not something that
will provide a fix for users right now.

So please don't even pretend to suggest that we shouldn't fix this
with a revert unless a simple, obvious, kernel fix presents itself.

On Wed, 13 Apr 2011 10:43:17 -0700 (PDT)
David Miller [off-list ref] wrote:

From: Daniel Baluta <redacted>
Date: Wed, 13 Apr 2011 14:57:18 +0300

quoted

Cyril's use case looks suspect. I don't think that this is a good
reason for reverting this commit.

I complete disagree.

Something that worked perfectly fine, probably for years, we broke.

We simply cannot do that, especially since we do not have a reasonable
alternative at this time.

Adding SO_REUSEPORT is a long range option, and not something that
will provide a fix for users right now.

So please don't even pretend to suggest that we shouldn't fix this
with a revert unless a simple, obvious, kernel fix presents itself.

Just to echo what Dave said.
Even though the semantics of this is not documented in some standard,
applications have been built on Linux expecting a certain behavior.
If you want to change what happens in this case, you have to have a
really good reason (like crash, security hole, or standards violation).

From: Eric Dumazet <redacted>
Date: Wed, 13 Apr 2011 05:00:08 +0200

Le mercredi 13 avril 2011 à 04:55 +0200, Eric Dumazet a écrit :

quoted

Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Sorry, I should have mentioned commit id : c191a836a908d1dd6
(tcp: disallow bind() to reuse addr/port)

I'm commiting the revert as follows to net-2.6, and will queue
it up for -stable as well:

--------------------
Revert "tcp: disallow bind() to reuse addr/port"

This reverts commit c191a836a908d1dd6b40c503741f91b914de3348.

It causes known regressions for programs that expect to be able to use
SO_REUSEADDR to shutdown a socket, then successfully rebind another
socket to the same ID.

Programs such as haproxy and amavisd expect this to work.

This should fix kernel bugzilla 32832.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/inet_connection_sock.c  |    5 ++---
 net/ipv6/inet6_connection_sock.c |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 6c0b7f4..38f23e7 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c

@@ -73,7 +73,7 @@ int inet_csk_bind_conflict(const struct sock *sk,
 		     !sk2->sk_bound_dev_if ||
 		     sk->sk_bound_dev_if == sk2->sk_bound_dev_if)) {
 			if (!reuse || !sk2->sk_reuse ||
-			    ((1 << sk2->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) {
+			    sk2->sk_state == TCP_LISTEN) {
 				const __be32 sk2_rcv_saddr = sk_rcv_saddr(sk2);
 				if (!sk2_rcv_saddr || !sk_rcv_saddr(sk) ||
 				    sk2_rcv_saddr == sk_rcv_saddr(sk))

@@ -122,8 +122,7 @@ again:
 					    (tb->num_owners < smallest_size || smallest_size == -1)) {
 						smallest_size = tb->num_owners;
 						smallest_rover = rover;
-						if (atomic_read(&hashinfo->bsockets) > (high - low) + 1 &&
-						    !inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+						if (atomic_read(&hashinfo->bsockets) > (high - low) + 1) {
 							spin_unlock(&head->lock);
 							snum = smallest_rover;
 							goto have_snum;

diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 1660546..f2c5b0f 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c

@@ -44,7 +44,7 @@ int inet6_csk_bind_conflict(const struct sock *sk,
 		     !sk2->sk_bound_dev_if ||
 		     sk->sk_bound_dev_if == sk2->sk_bound_dev_if) &&
 		    (!sk->sk_reuse || !sk2->sk_reuse ||
-		     ((1 << sk2->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) &&
+		     sk2->sk_state == TCP_LISTEN) &&
 		     ipv6_rcv_saddr_equal(sk, sk2))
 			break;
 	}

-- 
1.7.4.3

Le mercredi 13 avril 2011 à 12:09 -0700, David Miller a écrit :

From: Eric Dumazet <redacted>
Date: Wed, 13 Apr 2011 05:00:08 +0200

quoted

Le mercredi 13 avril 2011 à 04:55 +0200, Eric Dumazet a écrit :

quoted

Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Sorry, I should have mentioned commit id : c191a836a908d1dd6
(tcp: disallow bind() to reuse addr/port)

I'm commiting the revert as follows to net-2.6, and will queue
it up for -stable as well:

--------------------
Revert "tcp: disallow bind() to reuse addr/port"

This reverts commit c191a836a908d1dd6b40c503741f91b914de3348.

It causes known regressions for programs that expect to be able to use
SO_REUSEADDR to shutdown a socket, then successfully rebind another
socket to the same ID.

Programs such as haproxy and amavisd expect this to work.

This should fix kernel bugzilla 32832.

Signed-off-by: David S. Miller <davem@davemloft.net>
---

Thanks David, I was planning to submit this revert this morning, you
beat me ;)

On Wed, Apr 13, 2011 at 04:55:27AM +0200, Eric Dumazet wrote:

Le mardi 12 avril 2011 à 16:17 -0700, David Miller a écrit :

quoted

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 12 Apr 2011 16:15:56 -0700

quoted

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

Stephen Hemminger forwarded this to the list last week, and Eric
Dumazet is actively working on a fix.

I worked on it this week end to discover FreeBSD 8.1 would not allow
several CLOSE sockets bound to same port even with REUSEADDR.

So haproxy claim is a bit wrong (its trick doesnt work on FreeBSD), and
used an undocumented linux feature.

I feel this case is a call for SO_REUSEPORT, eventually.

http://www.unixguide.net/network/socketfaq/4.11.shtml

  SO_REUSEADDR allows your server to bind to an address which is in a
  TIME_WAIT state.  It does not allow more than one server to bind to
  the same address.  It was mentioned that use of this flag can create a
  security risk because another server can bind to a the same port, by
  binding to a specific address as opposed to INADDR_ANY.  The
  SO_REUSEPORT flag allows multiple processes to bind to the same
  address provided all of them use the SO_REUSEPORT option.


Since SO_REUSEPORT is not a 'stable fix', I suggest we revert the patch,
and eventually work on SO_REUSEPORT on net-next-2.6

What do you think ?

Not entirely related, but FWIW I think that SO_REUSEPORT would
be rather useful for haproxy.

I've been working on allowing haproxy to be reconfigured without dropping
or reusing connections. I have done this by re-using open sockets. But it
would have been rather a lot easier to achieve using SO_REUSEPORT -
assuming that I understand SO_REUSEPORT correctly.