net: do not allow changing SO_REUSEADDR/SO_REUSEPORT on bound sockets

From: Maciej Żenczykowski <[email protected]>

It is not safe to do so because such sockets are already in the hash tables and changing these options can result in invalidating the tb->fastreuse(port) caching.

This can have later far reaching consequences wrt. bind conflict checks which rely on these caches (for optimization purposes).

Not to mention that you can currently end up with two identical non-reuseport listening sockets bound to the same local ip:port by clearing reuseport on them after they've already both been bound.

There is unfortunately no EISBOUND error or anything similar, and EISCONN seems to be misleading for a bound-but-not-connected socket, so use EUCLEAN 'Structure needs cleaning' which AFAICT is the closest you can get to meaning 'socket in bad state'. (although perhaps EINVAL wouldn't be a bad choice either?)

This does unfortunately run the risk of breaking buggy userspace programs...

Signed-off-by Maciej Żenczykowski <[email protected]>

Cc Eric Dumazet <[email protected]>

Change-Id: I77c2b3429b2fdf42671eee0fa7a8ba721c94963b

---
 net/core/sock.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

net/core/sock.c +14 -2

index 435a0ba85e52..feca4c98f8a0 100644
@@ -728,9 +728,22 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
                        sock_valbool_flag(sk, SOCK_DBG, valbool);
                break;
        case SO_REUSEADDR:
-               sk->sk_reuse = (valbool ? SK_CAN_REUSE : SK_NO_REUSE);
+               val = (valbool ? SK_CAN_REUSE : SK_NO_REUSE);
+               if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) &&
+                   inet_sk(sk)->inet_num &&
+                   (sk->sk_reuse != val)) {
+                       ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN;
+                       break;
+               }
+               sk->sk_reuse = val;
                break;
        case SO_REUSEPORT:
+               if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) &&
+                   inet_sk(sk)->inet_num &&
+                   (sk->sk_reuseport != valbool)) {
+                       ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN;
+                       break;
+               }
                sk->sk_reuseport = valbool;
                break;
        case SO_TYPE:
--

2.17.1.1185.g55be947832-goog

From: Maciej Żenczykowski <[email protected]>

It is not safe to do so because such sockets are already in the hash tables and changing these options can result in invalidating the tb->fastreuse(port) caching.

From: Maciej Żenczykowski <[email protected]>

It is not safe to do so because such sockets are already in the hash tables and changing these options can result in invalidating the tb->fastreuse(port) caching.

This can have later far reaching consequences wrt. bind conflict checks which rely on these caches (for optimization purposes).

Not to mention that you can currently end up with two identical non-reuseport listening sockets bound to the same local ip:port by clearing reuseport on them after they've already both been bound.

There is unfortunately no EISBOUND error or anything similar, and EISCONN seems to be misleading for a bound-but-not-connected socket, so use EUCLEAN 'Structure needs cleaning' which AFAICT is the closest you can get to meaning 'socket in bad state'. (although perhaps EINVAL wouldn't be a bad choice either?)

This does unfortunately run the risk of breaking buggy userspace programs...

Signed-off-by Maciej Żenczykowski <[email protected]>

Change-Id I77c2b3429b2fdf42671eee0fa7a8ba721c94963b

From: Maciej Żenczykowski <[email protected]>

It is not safe to do so because such sockets are already in the hash tables and changing these options can result in invalidating the tb->fastreuse(port) caching.

This can have later far reaching consequences wrt. bind conflict checks which rely on these caches (for optimization purposes).

Not to mention that you can currently end up with two identical non-reuseport listening sockets bound to the same local ip:port by clearing reuseport on them after they've already both been bound.

There is unfortunately no EISBOUND error or anything similar, and EISCONN seems to be misleading for a bound-but-not-connected socket, so use EUCLEAN 'Structure needs cleaning' which AFAICT is the closest you can get to meaning 'socket in bad state'. (although perhaps EINVAL wouldn't be a bad choice either?)

This does unfortunately run the risk of breaking buggy userspace programs...

Signed-off-by Maciej Żenczykowski <[email protected]>

Cc Eric Dumazet <[email protected]>

Change-Id I77c2b3429b2fdf42671eee0fa7a8ba721c94963b

Reviewed-by Eric Dumazet <[email protected]>

---
 net/core/sock.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

net/core/sock.c +14 -1

index 435a0ba85e52..feca4c98f8a0 100644
@@ -728,9 +728,22 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 			sock_valbool_flag(sk, SOCK_DBG, valbool);
 		break;
 	case SO_REUSEADDR:
-		sk->sk_reuse = (valbool ? SK_CAN_REUSE : SK_NO_REUSE);
+		val = (valbool ? SK_CAN_REUSE : SK_NO_REUSE);
+		if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) &&
+		    inet_sk(sk)->inet_num &&
+		    (sk->sk_reuse != val)) {
+			ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN;
+			break;
+		}
+		sk->sk_reuse = val;
 		break;
 	case SO_REUSEPORT:
+		if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) &&
+		    inet_sk(sk)->inet_num &&
+		    (sk->sk_reuseport != valbool)) {
+			ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN;
+			break;
+		}
 		sk->sk_reuseport = valbool;
 		break;
 	case SO_TYPE:

Yes, it does, we found this internally last night and been debating what to do about it.

Fundamentally what it points out is that prior to this patch CRIU could get the host into an inconsistent state.

It inserts all the sockets into the hashtables with SO_REUSEADDR set, and then (potentially) clears it on some of them...
But the tb cache still thinks it's set on all of them.
So later attempts to bind() a socket with SO_REUSEADDR set can then succeed even though they should fail (or something like that).

I wonder if we instead need a socket option to basically say 'ignore all conflicts' that CRIU could set, and then clear post bind/listen/connect
hash table insertion...

Or maybe the transition from 1->0 is valid, but from 0->1 isn't??

Or we need special per-protocol code in the SO_REUSE{ADDR,PORT} setsockopt handler to recalculate the tb cache?

Anyone have any smart ideas?

I think we probably need to make sk->sk_reuse back into a boolean. (ie. eliminate SK_FORCE_REUSE)

Then add a new tcp/udp sk->ignore_bind_conflicts boolean setting... (ie. not just for tcp, but sol_socket) [or perhaps SO_REPAIR, sk->repair or something]

What I'm not certain of is exactly what sorts of conflicts it should ignore... all? probably not, still seems utterly wrong to allow creation of 2 connected tcp sockets with identical 5-tuples.

Would it only ignore conflicts against other i_b_c sockets? ie. set it on all sockets as we're repairing, then clear it on them all once we're done?

and ignore all the fast caching when checking conflicts for an i_b_c socket?

For CRIU is it safe to assume we're restoring an entire namespace into a new namespace?

Could we perhaps instead allow a new namespace to ignore bind conflicts until we flip it into enforcing mode?

From 990baa56993827ae6f4441cf078eddf73389d6ee Mon Sep 17 00:00:00 2001

From: Maciej Żenczykowski <[email protected]>

It is not safe to do so because such sockets are already in the hash tables and changing these options can result in invalidating the tb->fastreuse(port) caching.

This can have later far reaching consequences wrt. bind conflict checks which rely on these caches (for optimization purposes).

Not to mention that you can currently end up with two identical non-reuseport listening sockets bound to the same local ip:port by clearing reuseport on them after they've already both been bound.

There is unfortunately no EISBOUND error or anything similar, and EISCONN seems to be misleading for a bound-but-not-connected socket, so use EUCLEAN 'Structure needs cleaning' which AFAICT is the closest you can get to meaning 'socket in bad state'. (although perhaps EINVAL wouldn't be a bad choice either?)

This does unfortunately run the risk of breaking buggy userspace programs...

Signed-off-by Maciej Żenczykowski <[email protected]>

Cc Eric Dumazet <[email protected]>

Change-Id I77c2b3429b2fdf42671eee0fa7a8ba721c94963b

Reviewed-by Eric Dumazet <[email protected]>

---
 net/core/sock.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

net/core/sock.c +0 -0

index 435a0ba85e52..feca4c98f8a0 100644

net/core/sock.c +9 -0

+			break;
+		}
+		sk->sk_reuse = val;
 		break;
 	case SO_REUSEPORT:
+		if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) &&
+		    inet_sk(sk)->inet_num &&
+		    (sk->sk_reuseport != valbool)) {
+			ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN;
+			break;
+		}
 		sk->sk_reuseport = valbool;
 		break;
 	case SO_TYPE:

From: Maciej Żenczykowski <[email protected]>

It is not safe to do so because such sockets are already in the hash tables and changing these options can result in invalidating the tb->fastreuse(port) caching.

This can have later far reaching consequences wrt. bind conflict checks which rely on these caches (for optimization purposes).

Not to mention that you can currently end up with two identical non-reuseport listening sockets bound to the same local ip:port by clearing reuseport on them after they've already both been bound.

There is unfortunately no EISBOUND error or anything similar, and EISCONN seems to be misleading for a bound-but-not-connected socket, so use EUCLEAN 'Structure needs cleaning' which AFAICT is the closest you can get to meaning 'socket in bad state'. (although perhaps EINVAL wouldn't be a bad choice either?)

This does unfortunately run the risk of breaking buggy userspace programs...

This change is a potential performance regression for us. Our networking code sets up multiple sockets used by multiple threads to listen on the same udp port. For the first socket, the bind is done before setting SO_REUSEPORT so that we can get an error and detect that the port is currently in use by a different process, possibly a previous incarnation of the same application.

With this change, the servers end up with a single listener socket and thread, which can have a large impact on performance for a busy server.

While we can adjust for the new behaviour going forward, this could be an unpleasant surprise for our customers who get this update when moving to a new kernel version or through a backport to stable kernels, if this is targeted for stable.

Quoted 7 lines

This change is a potential performance regression for us. Our networking code sets up multiple sockets used by multiple threads to listen on the same udp port. For the first socket, the bind is done before setting SO_REUSEPORT so that we can get an error and detect that the port is currently in use by a different process, possibly a previous incarnation of the same application.

Quoted 8 lines

With this change, the servers end up with a single listener socket and thread, which can have a large impact on performance for a busy server.

While we can adjust for the new behaviour going forward, this could be an unpleasant surprise for our customers who get this update when moving to a new kernel version or through a backport to stable kernels, if this is targeted for stable.

Probably you can:
fd1=socket()
fd2=socket()
bind(fd1,port=0)
setsockopt(fd2,REUSEPORT,1)
port = getsockname(fd1)
close(fd1)
bind(fd2,port)

Although yeah there's a slight chance of a race (ie. 2nd bind failing, in which case close() and retry).