[PATCH v2 0/3] Further migration fixes

[PATCH v2 0/3] Further migration fixes

[David Gibson]

Here is some more work on the migration stuff.  Two of these are
fixes, alas, for the already merged "safe" patches.  Then there's some
extra debugging.

David Gibson (3):
  migrate: Fix several errors with passt-repair
  migrate, tcp: Report more error conditions during connection repair
  tcp: Simplify handling of getsockname()

 .gitignore     |  1 +
 Makefile       |  2 +-
 passt-repair.c |  2 +-
 tcp.c          | 35 ++++++++++++++++-------------------
 4 files changed, 19 insertions(+), 21 deletions(-)

-- 
2.48.1

[PATCH v2 1/3] migrate: Fix several errors with passt-repair

[David Gibson]

The passt-repair helper is now merged, but alas it contains several small
bugs:
 * close() is not in the seccomp profile, meaning it will immediately
   SIGSYS when you make a request of it
 * The generated header, seccomp_repair.h isn't listed in .gitignore or
   removed by "make clean"

Fixes: 8c24301462c3 ("Introduce passt-repair")

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 .gitignore     | 1 +
 Makefile       | 2 +-
 passt-repair.c | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 5824a71e..3c16adc7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,5 +7,6 @@
 /qrap
 /pasta.1
 /seccomp.h
+/seccomp_repair.h
 /c*.json
 README.plain.md
diff --git a/Makefile b/Makefile
index fde66701..d4e10967 100644
--- a/Makefile
+++ b/Makefile
@@ -117,7 +117,7 @@ valgrind: all
 
 .PHONY: clean
 clean:
-	$(RM) $(BIN) *~ *.o seccomp.h pasta.1 \
+	$(RM) $(BIN) *~ *.o seccomp.h seccomp_repair.h pasta.1 \
 		passt.tar passt.tar.gz *.deb *.rpm \
 		passt.pid README.plain.md
 
diff --git a/passt-repair.c b/passt-repair.c
index 767a8211..dd8578f5 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -46,7 +46,7 @@
  *
  * Return: 0 on success (EOF), 1 on error, 2 on usage error
  *
- * #syscalls:repair connect setsockopt write exit_group
+ * #syscalls:repair connect setsockopt write close exit_group
  * #syscalls:repair socket s390x:socketcall i686:socketcall
  * #syscalls:repair recvfrom recvmsg arm:recv ppc64le:recv
  * #syscalls:repair sendto sendmsg arm:send ppc64le:send
-- 
@@ -46,7 +46,7 @@
  *
  * Return: 0 on success (EOF), 1 on error, 2 on usage error
  *
- * #syscalls:repair connect setsockopt write exit_group
+ * #syscalls:repair connect setsockopt write close exit_group
  * #syscalls:repair socket s390x:socketcall i686:socketcall
  * #syscalls:repair recvfrom recvmsg arm:recv ppc64le:recv
  * #syscalls:repair sendto sendmsg arm:send ppc64le:send
-- 
2.48.1

Re: [PATCH v2 1/3] migrate: Fix several errors with passt-repair

[Stefano Brivio]

On Tue,  4 Feb 2025 16:42:13 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> The passt-repair helper is now merged, but alas it contains several small
> bugs:
>  * close() is not in the seccomp profile, meaning it will immediately
>    SIGSYS when you make a request of it
>  * The generated header, seccomp_repair.h isn't listed in .gitignore or
>    removed by "make clean"
> 
> Fixes: 8c24301462c3 ("Introduce passt-repair")
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied.

-- 
Stefano

[PATCH v2 2/3] migrate, tcp: Report more error conditions during connection repair

[David Gibson]

This should help debugging problems with migration.

[This is a candidate to be folded into the earlier patch]

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tcp.c b/tcp.c
index 3760e67a..7ce0f4cb 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1011,10 +1011,17 @@ int tcp_flow_repair_connect(struct ctx *c, struct tcp_tap_conn *conn)
 {
 	struct flowside *tgt = &conn->f.side[TGTSIDE];
 	struct tcp_repair_opt opts[2];
+	int rc;
 
-	tcp_flow_repair_seq(c, conn, true);
+	rc = tcp_flow_repair_seq(c, conn, true);
+	if (rc)
+		return rc;
 
-	flowside_connect(c, conn->sock, PIF_HOST, tgt);
+	rc = flowside_connect(c, conn->sock, PIF_HOST, tgt);
+	if (rc) {
+		err("Failed to connect repaired socket: %s", strerror_(errno));
+		return rc;
+	}
 
 	/* FIXME: Fetch those with TCP_REPAIR_OPTIONS and store in migration
 	 * data. These hardcoded values just happen to be good enough.
-- 
@@ -1011,10 +1011,17 @@ int tcp_flow_repair_connect(struct ctx *c, struct tcp_tap_conn *conn)
 {
 	struct flowside *tgt = &conn->f.side[TGTSIDE];
 	struct tcp_repair_opt opts[2];
+	int rc;
 
-	tcp_flow_repair_seq(c, conn, true);
+	rc = tcp_flow_repair_seq(c, conn, true);
+	if (rc)
+		return rc;
 
-	flowside_connect(c, conn->sock, PIF_HOST, tgt);
+	rc = flowside_connect(c, conn->sock, PIF_HOST, tgt);
+	if (rc) {
+		err("Failed to connect repaired socket: %s", strerror_(errno));
+		return rc;
+	}
 
 	/* FIXME: Fetch those with TCP_REPAIR_OPTIONS and store in migration
 	 * data. These hardcoded values just happen to be good enough.
-- 
2.48.1

[PATCH v2 3/3] tcp: Simplify handling of getsockname()

[David Gibson]

For migration we need to get the specific local address and port for
connected sockets with getsockname().  We currently open code marshalling
the results into the flow entry.

However, we already have inany_from_sockaddr() which handles the fiddly
parts of this, so use it.  Also report failures, which may make debugging
problems easier.

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

diff --git a/tcp.c b/tcp.c
index 7ce0f4cb..dbdaa9d9 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1712,24 +1712,14 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 	tcp_epoll_ctl(c, conn);
 
 	if (c->mode == MODE_VU) { /* To rebind to same oport after migration */
-		if (af == AF_INET) {
-			struct sockaddr_in s_in;
-
-			sl = sizeof(s_in);
-			if (!getsockname(s, (struct sockaddr *)&s_in, &sl)) {
-				/* NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) */
-				tgt->oport = ntohs(s_in.sin_port);
-				tgt->oaddr = inany_from_v4(s_in.sin_addr);
-			}
-		} else {
-			struct sockaddr_in6 s_in6;
+		union sockaddr_inany sa;
+		socklen_t sl = sizeof(sa);
 
-			sl = sizeof(s_in6);
-			if (!getsockname(s, (struct sockaddr *)&s_in6, &sl)) {
-				/* NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) */
-				tgt->oport = ntohs(s_in6.sin6_port);
-				tgt->oaddr.a6 = s_in6.sin6_addr;
-			}
+		if (!getsockname(s, &sa.sa, &sl)) {
+			inany_from_sockaddr(&tgt->oaddr, &tgt->oport, &sa);
+		} else {
+			err("Failed to get local address for socket: %s",
+			    strerror_(errno));
 		}
 	}
 
-- 
@@ -1712,24 +1712,14 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 	tcp_epoll_ctl(c, conn);
 
 	if (c->mode == MODE_VU) { /* To rebind to same oport after migration */
-		if (af == AF_INET) {
-			struct sockaddr_in s_in;
-
-			sl = sizeof(s_in);
-			if (!getsockname(s, (struct sockaddr *)&s_in, &sl)) {
-				/* NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) */
-				tgt->oport = ntohs(s_in.sin_port);
-				tgt->oaddr = inany_from_v4(s_in.sin_addr);
-			}
-		} else {
-			struct sockaddr_in6 s_in6;
+		union sockaddr_inany sa;
+		socklen_t sl = sizeof(sa);
 
-			sl = sizeof(s_in6);
-			if (!getsockname(s, (struct sockaddr *)&s_in6, &sl)) {
-				/* NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) */
-				tgt->oport = ntohs(s_in6.sin6_port);
-				tgt->oaddr.a6 = s_in6.sin6_addr;
-			}
+		if (!getsockname(s, &sa.sa, &sl)) {
+			inany_from_sockaddr(&tgt->oaddr, &tgt->oport, &sa);
+		} else {
+			err("Failed to get local address for socket: %s",
+			    strerror_(errno));
 		}
 	}
 
-- 
2.48.1

Re: [PATCH v2 3/3] tcp: Simplify handling of getsockname()

[Stefano Brivio]

On Tue,  4 Feb 2025 16:42:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> For migration we need to get the specific local address and port for
> connected sockets with getsockname().  We currently open code marshalling
> the results into the flow entry.
> 
> However, we already have inany_from_sockaddr() which handles the fiddly
> parts of this, so use it.  Also report failures, which may make debugging
> problems easier.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 7ce0f4cb..dbdaa9d9 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1712,24 +1712,14 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
>  	tcp_epoll_ctl(c, conn);
>  
>  	if (c->mode == MODE_VU) { /* To rebind to same oport after migration */
> -		if (af == AF_INET) {
> -			struct sockaddr_in s_in;
> -
> -			sl = sizeof(s_in);
> -			if (!getsockname(s, (struct sockaddr *)&s_in, &sl)) {
> -				/* NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) */
> -				tgt->oport = ntohs(s_in.sin_port);
> -				tgt->oaddr = inany_from_v4(s_in.sin_addr);
> -			}
> -		} else {
> -			struct sockaddr_in6 s_in6;
> +		union sockaddr_inany sa;
> +		socklen_t sl = sizeof(sa);

This shadows the previous declaration of 'sl'. Turning into an
assignment.

> -			sl = sizeof(s_in6);
> -			if (!getsockname(s, (struct sockaddr *)&s_in6, &sl)) {
> -				/* NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) */
> -				tgt->oport = ntohs(s_in6.sin6_port);
> -				tgt->oaddr.a6 = s_in6.sin6_addr;
> -			}
> +		if (!getsockname(s, &sa.sa, &sl)) {
> +			inany_from_sockaddr(&tgt->oaddr, &tgt->oport, &sa);
> +		} else {
> +			err("Failed to get local address for socket: %s",
> +			    strerror_(errno));
>  		}
>  	}

-- 
Stefano

Re: [PATCH v2 3/3] tcp: Simplify handling of getsockname()

[Stefano Brivio]

On Tue,  4 Feb 2025 16:42:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> For migration we need to get the specific local address and port for
> connected sockets with getsockname().  We currently open code marshalling
> the results into the flow entry.
> 
> However, we already have inany_from_sockaddr() which handles the fiddly
> parts of this, so use it.  Also report failures, which may make debugging
> problems easier.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied with re-declarations of 'sa' and 'sl' dropped.

-- 
Stefano