[PATCH 0/2] Fix some redundant and incorrect initialisation of UDP sockets

[PATCH 0/2] Fix some redundant and incorrect initialisation of UDP sockets

[David Gibson]

This fixes bug 79 and makes another clean up in the same area.

Link: https://bugs.passt.top/show_bug.cgi?id=79

David Gibson (2):
  udp: Don't prematurely (and incorrectly) set up automatic inbound
    forwards
  udp: udp_sock_init_ns() partially duplicats udp_port_rebind_outbound()

 udp.c | 90 +++++++++++++++++------------------------------------------
 1 file changed, 25 insertions(+), 65 deletions(-)

-- 
2.43.0

[PATCH 1/2] udp: Don't prematurely (and incorrectly) set up automatic inbound forwards

[David Gibson]

For automated inbound port forwarding in pasta mode we scan bound ports
within the guest namespace via /proc and bind matching ports on the host to
listen for packets.  For UDP this is usually handled by udp_timer() which
calls port_fwd_scan_udp() followed by udp_port_rebind().  However there's
one initial scan before the the UDP timer is started: we call
port_fwd_scan_udp() from port_fwd_init(), and actually bind the resulting
ports in udp_sock_init_init() called from udp_init().

Unfortunately, the version in udp_sock_init_init() isn't correct.  It
unconditionally opens a new socket for every forwarded port, even if a
socket has already been explicit created with the -u option.  If the
explicitly forwarded ports have particular configuration (such as a
specific bound address address, or one implied by the -o option) those will
not be replicated in the new socket.  We essentially leak the original
correctly configured socket, replacing it with one which might not be
right.

We could make udp_sock_init_init() use udp_port_rebind() to get that right,
but there's actually no point doing so:
 * The initial bind was introduced by ccf6d2a7b48d ("udp: Actually bind
   detected namespace ports in init namespace") at which time we didn't
   periodically scan for bound UDP ports.  Periodic scanning was introduced
   in 457ff122e ("udp,pasta: Periodically scan for ports to automatically
   forward") making the bind from udp_init() redundant.
 * At the time of udp_init(), programs in the guest namespace are likely
   not to have started yet (unless attaching a pre-existing namespace) so
   there's likely not anything to scan for anyway.

So, simply remove the initial, broken socket create/bind, allowing
automatic port forwards to be created the first time udp_timer() runs.

Reported-by: Laurent Jacquot <jk@lutty.net>
Suggested-by: Laurent Jacquot <jk@lutty.net>
Link: https://bugs.passt.top/show_bug.cgi?id=79

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/udp.c b/udp.c
index b5b8f8a7..b240185e 100644
--- a/udp.c
+++ b/udp.c
@@ -1041,22 +1041,6 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	return r4 < 0 ? r4 : r6;
 }
 
-/**
- * udp_sock_init_init() - Bind sockets in init namespace for inbound connections
- * @c:		Execution context
- */
-static void udp_sock_init_init(const struct ctx *c)
-{
-	unsigned dst;
-
-	for (dst = 0; dst < NUM_PORTS; dst++) {
-		if (!bitmap_isset(c->udp.fwd_in.f.map, dst))
-			continue;
-
-		udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, dst);
-	}
-}
-
 /**
  * udp_sock_init_ns() - Bind sockets in namespace for outbound connections
  * @arg:	Execution context
@@ -1125,7 +1109,6 @@ int udp_init(struct ctx *c)
 
 	if (c->mode == MODE_PASTA) {
 		udp_splice_iov_init();
-		udp_sock_init_init(c);
 		NS_CALL(udp_sock_init_ns, c);
 	}
 
-- 
@@ -1041,22 +1041,6 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	return r4 < 0 ? r4 : r6;
 }
 
-/**
- * udp_sock_init_init() - Bind sockets in init namespace for inbound connections
- * @c:		Execution context
- */
-static void udp_sock_init_init(const struct ctx *c)
-{
-	unsigned dst;
-
-	for (dst = 0; dst < NUM_PORTS; dst++) {
-		if (!bitmap_isset(c->udp.fwd_in.f.map, dst))
-			continue;
-
-		udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, dst);
-	}
-}
-
 /**
  * udp_sock_init_ns() - Bind sockets in namespace for outbound connections
  * @arg:	Execution context
@@ -1125,7 +1109,6 @@ int udp_init(struct ctx *c)
 
 	if (c->mode == MODE_PASTA) {
 		udp_splice_iov_init();
-		udp_sock_init_init(c);
 		NS_CALL(udp_sock_init_ns, c);
 	}
 
-- 
2.43.0

[PATCH 2/2] udp: udp_sock_init_ns() partially duplicats udp_port_rebind_outbound()

[David Gibson]

Usually automatically forwarded UDP outbound ports are set up by
udp_port_rebind_outbound() called from udp_timer().  However, the very
first time they're created and bound is by udp_sock_init_ns() called from
udp_init().  udp_sock_init_ns() is essentially an unnecessary cut down
version of udp_port_rebind_outbound(), so we can jusat remove it.

Doing so does require moving udp_init() below udp_port_rebind_outbound()'s
definition.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 73 ++++++++++++++++++++---------------------------------------
 1 file changed, 25 insertions(+), 48 deletions(-)

diff --git a/udp.c b/udp.c
index b240185e..933f24b8 100644
--- a/udp.c
+++ b/udp.c
@@ -1041,29 +1041,6 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	return r4 < 0 ? r4 : r6;
 }
 
-/**
- * udp_sock_init_ns() - Bind sockets in namespace for outbound connections
- * @arg:	Execution context
- *
- * Return: 0
- */
-int udp_sock_init_ns(void *arg)
-{
-	const struct ctx *c = (const struct ctx *)arg;
-	unsigned dst;
-
-	ns_enter(c);
-
-	for (dst = 0; dst < NUM_PORTS; dst++) {
-		if (!bitmap_isset(c->udp.fwd_out.f.map, dst))
-			continue;
-
-		udp_sock_init(c, 1, AF_UNSPEC, NULL, NULL, dst);
-	}
-
-	return 0;
-}
-
 /**
  * udp_splice_iov_init() - Set up buffers and descriptors for recvmmsg/sendmmsg
  */
@@ -1090,31 +1067,6 @@ static void udp_splice_iov_init(void)
 	}
 }
 
-/**
- * udp_init() - Initialise per-socket data, and sockets in namespace
- * @c:		Execution context
- *
- * Return: 0
- */
-int udp_init(struct ctx *c)
-{
-	if (c->ifi4)
-		udp_sock4_iov_init(c);
-
-	if (c->ifi6)
-		udp_sock6_iov_init(c);
-
-	udp_invert_portmap(&c->udp.fwd_in);
-	udp_invert_portmap(&c->udp.fwd_out);
-
-	if (c->mode == MODE_PASTA) {
-		udp_splice_iov_init();
-		NS_CALL(udp_sock_init_ns, c);
-	}
-
-	return 0;
-}
-
 /**
  * udp_timer_one() - Handler for timed events on one port
  * @c:		Execution context
@@ -1272,3 +1224,28 @@ v6:
 		goto v6;
 	}
 }
+
+/**
+ * udp_init() - Initialise per-socket data, and sockets in namespace
+ * @c:		Execution context
+ *
+ * Return: 0
+ */
+int udp_init(struct ctx *c)
+{
+	if (c->ifi4)
+		udp_sock4_iov_init(c);
+
+	if (c->ifi6)
+		udp_sock6_iov_init(c);
+
+	udp_invert_portmap(&c->udp.fwd_in);
+	udp_invert_portmap(&c->udp.fwd_out);
+
+	if (c->mode == MODE_PASTA) {
+		udp_splice_iov_init();
+		NS_CALL(udp_port_rebind_outbound, c);
+	}
+
+	return 0;
+}
-- 
@@ -1041,29 +1041,6 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	return r4 < 0 ? r4 : r6;
 }
 
-/**
- * udp_sock_init_ns() - Bind sockets in namespace for outbound connections
- * @arg:	Execution context
- *
- * Return: 0
- */
-int udp_sock_init_ns(void *arg)
-{
-	const struct ctx *c = (const struct ctx *)arg;
-	unsigned dst;
-
-	ns_enter(c);
-
-	for (dst = 0; dst < NUM_PORTS; dst++) {
-		if (!bitmap_isset(c->udp.fwd_out.f.map, dst))
-			continue;
-
-		udp_sock_init(c, 1, AF_UNSPEC, NULL, NULL, dst);
-	}
-
-	return 0;
-}
-
 /**
  * udp_splice_iov_init() - Set up buffers and descriptors for recvmmsg/sendmmsg
  */
@@ -1090,31 +1067,6 @@ static void udp_splice_iov_init(void)
 	}
 }
 
-/**
- * udp_init() - Initialise per-socket data, and sockets in namespace
- * @c:		Execution context
- *
- * Return: 0
- */
-int udp_init(struct ctx *c)
-{
-	if (c->ifi4)
-		udp_sock4_iov_init(c);
-
-	if (c->ifi6)
-		udp_sock6_iov_init(c);
-
-	udp_invert_portmap(&c->udp.fwd_in);
-	udp_invert_portmap(&c->udp.fwd_out);
-
-	if (c->mode == MODE_PASTA) {
-		udp_splice_iov_init();
-		NS_CALL(udp_sock_init_ns, c);
-	}
-
-	return 0;
-}
-
 /**
  * udp_timer_one() - Handler for timed events on one port
  * @c:		Execution context
@@ -1272,3 +1224,28 @@ v6:
 		goto v6;
 	}
 }
+
+/**
+ * udp_init() - Initialise per-socket data, and sockets in namespace
+ * @c:		Execution context
+ *
+ * Return: 0
+ */
+int udp_init(struct ctx *c)
+{
+	if (c->ifi4)
+		udp_sock4_iov_init(c);
+
+	if (c->ifi6)
+		udp_sock6_iov_init(c);
+
+	udp_invert_portmap(&c->udp.fwd_in);
+	udp_invert_portmap(&c->udp.fwd_out);
+
+	if (c->mode == MODE_PASTA) {
+		udp_splice_iov_init();
+		NS_CALL(udp_port_rebind_outbound, c);
+	}
+
+	return 0;
+}
-- 
2.43.0

Re: [PATCH 0/2] Fix some redundant and incorrect initialisation of UDP sockets

[Stefano Brivio]

On Mon, 12 Feb 2024 17:06:56 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This fixes bug 79 and makes another clean up in the same area.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=79
> 
> David Gibson (2):
>   udp: Don't prematurely (and incorrectly) set up automatic inbound
>     forwards
>   udp: udp_sock_init_ns() partially duplicats udp_port_rebind_outbound()
> 
>  udp.c | 90 +++++++++++++++++------------------------------------------
>  1 file changed, 25 insertions(+), 65 deletions(-)

Applied.

-- 
Stefano