[PATCH 0/4] passt-repair improvements

[PATCH 0/4] passt-repair improvements

[David Gibson]

This series has a number of minor clean ups to passt-repair, plus one
more significant change.  The latter allows passt-repair to report
partial failures, but does require a small change to its protocol.

David Gibson (4):
  passt-repair: Add die() macro
  passt-repair: Consistently avoid strerror()
  passt-repair: Improve validation of anciliary data length
  passt-repair: Allow passt-repair to report partial failures

 passt-repair.c | 126 ++++++++++++++++++++++++-------------------------
 repair.c       |  11 +++--
 2 files changed, 68 insertions(+), 69 deletions(-)

-- 
2.48.1

[PATCH 1/4] passt-repair: Add die() macro

[David Gibson]

passt-repair has a frequently repeated idiom of printing an error message
then exiting with non-zero code.  Add our own version of a die() macro to
simplify this.  Probably because of confusion with passt's die() macro we
forgot to explicitly add a newline in some of those error messages.  Make
die() add this as well to be consistent.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt-repair.c | 72 ++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

diff --git a/passt-repair.c b/passt-repair.c
index e0c366e5..d785cd16 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -40,6 +40,13 @@
 
 #define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */
 
+#define die(status, ...)			\
+	do {					\
+		fprintf(stderr, __VA_ARGS__);	\
+		fprintf(stderr, "\n");		\
+		_exit(status);			\
+	} while (0)
+
 /**
  * main() - Entry point and whole program with loop
  * @argc:	Argument count, must be 2
@@ -72,10 +79,8 @@ int main(int argc, char **argv)
 				   sizeof(filter_repair[0]);
 	prog.filter = filter_repair;
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
-	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
-		fprintf(stderr, "Failed to apply seccomp filter");
-		_exit(1);
-	}
+	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog))
+		die(1, "Failed to apply seccomp filter");
 
 	iov = (struct iovec){ &cmd, sizeof(cmd) };
 	msg = (struct msghdr){ .msg_name = NULL, .msg_namelen = 0,
@@ -85,37 +90,28 @@ int main(int argc, char **argv)
 			       .msg_flags = 0 };
 	cmsg = CMSG_FIRSTHDR(&msg);
 
-	if (argc != 2) {
-		fprintf(stderr, "Usage: %s PATH\n", argv[0]);
-		_exit(2);
-	}
+	if (argc != 2)
+		die(2, "Usage: %s PATH", argv[0]);
 
 	ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[1]);
-	if (ret <= 0 || ret >= (int)sizeof(a.sun_path)) {
-		fprintf(stderr, "Invalid socket path: %s\n", argv[1]);
-		_exit(2);
-	}
+	if (ret <= 0 || ret >= (int)sizeof(a.sun_path))
+		die(2, "Invalid socket path: %s", argv[1]);
 
-	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
-		fprintf(stderr, "Failed to create AF_UNIX socket: %i\n", errno);
-		_exit(1);
-	}
+	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
+		die(1, "Failed to create AF_UNIX socket: %i", errno);
 
 	if (connect(s, (struct sockaddr *)&a, sizeof(a))) {
-		fprintf(stderr, "Failed to connect to %s: %s\n", argv[1],
-			strerror(errno));
-		_exit(1);
+		die(1, "Failed to connect to %s: %s", argv[1],
+		    strerror(errno));
 	}
 
 loop:
 	ret = recvmsg(s, &msg, 0);
 	if (ret < 0) {
-		if (errno == ECONNRESET) {
+		if (errno == ECONNRESET)
 			ret = 0;
-		} else {
-			fprintf(stderr, "Failed to read message: %i\n", errno);
-			_exit(1);
-		}
+		else
+			die(1, "Failed to read message: %i", errno);
 	}
 
 	if (!ret)	/* Done */
@@ -124,10 +120,8 @@ loop:
 	if (!cmsg ||
 	    cmsg->cmsg_len < CMSG_LEN(sizeof(int)) ||
 	    cmsg->cmsg_len > CMSG_LEN(sizeof(int) * SCM_MAX_FD) ||
-	    cmsg->cmsg_type != SCM_RIGHTS) {
-		fprintf(stderr, "No/bad ancillary data from peer\n");
-		_exit(1);
-	}
+	    cmsg->cmsg_type != SCM_RIGHTS)
+		die(1, "No/bad ancillary data from peer");
 
 	/* No inverse formula for CMSG_LEN(x), and building one with CMSG_LEN(0)
 	 * works but there's no guarantee it does. Search the whole domain.
@@ -140,27 +134,21 @@ loop:
 	}
 	if (!n) {
 		cmsg_len = cmsg->cmsg_len; /* socklen_t is 'unsigned' on musl */
-		fprintf(stderr, "Invalid ancillary data length %zu from peer\n",
-			cmsg_len);
-		_exit(1);
+		die(1, "Invalid ancillary data length %zu from peer", cmsg_len);
 	}
 
 	memcpy(fds, CMSG_DATA(cmsg), sizeof(int) * n);
 
 	if (cmd != TCP_REPAIR_ON && cmd != TCP_REPAIR_OFF &&
-	    cmd != TCP_REPAIR_OFF_NO_WP) {
-		fprintf(stderr, "Unsupported command 0x%04x\n", cmd);
-		_exit(1);
-	}
+	    cmd != TCP_REPAIR_OFF_NO_WP)
+		die(1, "Unsupported command 0x%04x", cmd);
 
 	op = cmd;
 
 	for (i = 0; i < n; i++) {
 		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
-			fprintf(stderr,
-				"Setting TCP_REPAIR to %i on socket %i: %s", op,
-				fds[i], strerror(errno));
-			_exit(1);
+			die(1, "Setting TCP_REPAIR to %i on socket %i: %s",
+			    op, fds[i], strerror(errno));
 		}
 
 		/* Close _our_ copy */
@@ -168,10 +156,8 @@ loop:
 	}
 
 	/* Confirm setting by echoing the command back */
-	if (send(s, &cmd, sizeof(cmd), 0) < 0) {
-		fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno));
-		_exit(1);
-	}
+	if (send(s, &cmd, sizeof(cmd), 0) < 0)
+		die(1, "Reply to %i: %s", op, strerror(errno));
 
 	goto loop;
 
-- 
@@ -40,6 +40,13 @@
 
 #define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */
 
+#define die(status, ...)			\
+	do {					\
+		fprintf(stderr, __VA_ARGS__);	\
+		fprintf(stderr, "\n");		\
+		_exit(status);			\
+	} while (0)
+
 /**
  * main() - Entry point and whole program with loop
  * @argc:	Argument count, must be 2
@@ -72,10 +79,8 @@ int main(int argc, char **argv)
 				   sizeof(filter_repair[0]);
 	prog.filter = filter_repair;
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
-	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
-		fprintf(stderr, "Failed to apply seccomp filter");
-		_exit(1);
-	}
+	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog))
+		die(1, "Failed to apply seccomp filter");
 
 	iov = (struct iovec){ &cmd, sizeof(cmd) };
 	msg = (struct msghdr){ .msg_name = NULL, .msg_namelen = 0,
@@ -85,37 +90,28 @@ int main(int argc, char **argv)
 			       .msg_flags = 0 };
 	cmsg = CMSG_FIRSTHDR(&msg);
 
-	if (argc != 2) {
-		fprintf(stderr, "Usage: %s PATH\n", argv[0]);
-		_exit(2);
-	}
+	if (argc != 2)
+		die(2, "Usage: %s PATH", argv[0]);
 
 	ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[1]);
-	if (ret <= 0 || ret >= (int)sizeof(a.sun_path)) {
-		fprintf(stderr, "Invalid socket path: %s\n", argv[1]);
-		_exit(2);
-	}
+	if (ret <= 0 || ret >= (int)sizeof(a.sun_path))
+		die(2, "Invalid socket path: %s", argv[1]);
 
-	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
-		fprintf(stderr, "Failed to create AF_UNIX socket: %i\n", errno);
-		_exit(1);
-	}
+	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
+		die(1, "Failed to create AF_UNIX socket: %i", errno);
 
 	if (connect(s, (struct sockaddr *)&a, sizeof(a))) {
-		fprintf(stderr, "Failed to connect to %s: %s\n", argv[1],
-			strerror(errno));
-		_exit(1);
+		die(1, "Failed to connect to %s: %s", argv[1],
+		    strerror(errno));
 	}
 
 loop:
 	ret = recvmsg(s, &msg, 0);
 	if (ret < 0) {
-		if (errno == ECONNRESET) {
+		if (errno == ECONNRESET)
 			ret = 0;
-		} else {
-			fprintf(stderr, "Failed to read message: %i\n", errno);
-			_exit(1);
-		}
+		else
+			die(1, "Failed to read message: %i", errno);
 	}
 
 	if (!ret)	/* Done */
@@ -124,10 +120,8 @@ loop:
 	if (!cmsg ||
 	    cmsg->cmsg_len < CMSG_LEN(sizeof(int)) ||
 	    cmsg->cmsg_len > CMSG_LEN(sizeof(int) * SCM_MAX_FD) ||
-	    cmsg->cmsg_type != SCM_RIGHTS) {
-		fprintf(stderr, "No/bad ancillary data from peer\n");
-		_exit(1);
-	}
+	    cmsg->cmsg_type != SCM_RIGHTS)
+		die(1, "No/bad ancillary data from peer");
 
 	/* No inverse formula for CMSG_LEN(x), and building one with CMSG_LEN(0)
 	 * works but there's no guarantee it does. Search the whole domain.
@@ -140,27 +134,21 @@ loop:
 	}
 	if (!n) {
 		cmsg_len = cmsg->cmsg_len; /* socklen_t is 'unsigned' on musl */
-		fprintf(stderr, "Invalid ancillary data length %zu from peer\n",
-			cmsg_len);
-		_exit(1);
+		die(1, "Invalid ancillary data length %zu from peer", cmsg_len);
 	}
 
 	memcpy(fds, CMSG_DATA(cmsg), sizeof(int) * n);
 
 	if (cmd != TCP_REPAIR_ON && cmd != TCP_REPAIR_OFF &&
-	    cmd != TCP_REPAIR_OFF_NO_WP) {
-		fprintf(stderr, "Unsupported command 0x%04x\n", cmd);
-		_exit(1);
-	}
+	    cmd != TCP_REPAIR_OFF_NO_WP)
+		die(1, "Unsupported command 0x%04x", cmd);
 
 	op = cmd;
 
 	for (i = 0; i < n; i++) {
 		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
-			fprintf(stderr,
-				"Setting TCP_REPAIR to %i on socket %i: %s", op,
-				fds[i], strerror(errno));
-			_exit(1);
+			die(1, "Setting TCP_REPAIR to %i on socket %i: %s",
+			    op, fds[i], strerror(errno));
 		}
 
 		/* Close _our_ copy */
@@ -168,10 +156,8 @@ loop:
 	}
 
 	/* Confirm setting by echoing the command back */
-	if (send(s, &cmd, sizeof(cmd), 0) < 0) {
-		fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno));
-		_exit(1);
-	}
+	if (send(s, &cmd, sizeof(cmd), 0) < 0)
+		die(1, "Reply to %i: %s", op, strerror(errno));
 
 	goto loop;
 
-- 
2.48.1

[PATCH 2/4] passt-repair: Consistently avoid strerror()

[David Gibson]

In a0b7f56b3a3c ("passt-repair: Don't use perror(), accept ECONNRESET as
termination") we altered passt-repair to avoid perror() since the glibc
version used a number of syscalls we didn't really want to add to our
seccomp filter.  We replaced the perror() calls with explicit messages just
printing the errno.

However, there are a number of other places we still explicitly use
strerror(errno).  As we discovered in passt, at least the glibc version is
rather more complex than you'd expect since it deals with locales.  Since
passt-repair is supposed to be minimal, and might be suid we want to avoid
this.

Consistently avoid strerror() with the help of a new ie_errno() macro which
prints errno as an integer instead.

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

diff --git a/passt-repair.c b/passt-repair.c
index d785cd16..3c358e27 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -24,7 +24,6 @@
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <string.h>
 #include <limits.h>
 #include <unistd.h>
 #include <netdb.h>
@@ -47,6 +46,14 @@
 		_exit(status);			\
 	} while (0)
 
+#define die_errno(...)					\
+	do {						\
+		int err_ = errno;			\
+		fprintf(stderr, __VA_ARGS__);		\
+		fprintf(stderr, ": %d\n", err_);	\
+		_exit(1);				\
+	} while (0)
+
 /**
  * main() - Entry point and whole program with loop
  * @argc:	Argument count, must be 2
@@ -80,7 +87,7 @@ int main(int argc, char **argv)
 	prog.filter = filter_repair;
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
 	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog))
-		die(1, "Failed to apply seccomp filter");
+		die_errno("Failed to apply seccomp filter");
 
 	iov = (struct iovec){ &cmd, sizeof(cmd) };
 	msg = (struct msghdr){ .msg_name = NULL, .msg_namelen = 0,
@@ -98,12 +105,10 @@ int main(int argc, char **argv)
 		die(2, "Invalid socket path: %s", argv[1]);
 
 	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
-		die(1, "Failed to create AF_UNIX socket: %i", errno);
+		die_errno("Failed to create AF_UNIX socket");
 
-	if (connect(s, (struct sockaddr *)&a, sizeof(a))) {
-		die(1, "Failed to connect to %s: %s", argv[1],
-		    strerror(errno));
-	}
+	if (connect(s, (struct sockaddr *)&a, sizeof(a)))
+		die_errno("Failed to connect to %s", argv[1]);
 
 loop:
 	ret = recvmsg(s, &msg, 0);
@@ -111,7 +116,7 @@ loop:
 		if (errno == ECONNRESET)
 			ret = 0;
 		else
-			die(1, "Failed to read message: %i", errno);
+			die_errno("Failed to read message");
 	}
 
 	if (!ret)	/* Done */
@@ -147,8 +152,8 @@ loop:
 
 	for (i = 0; i < n; i++) {
 		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
-			die(1, "Setting TCP_REPAIR to %i on socket %i: %s",
-			    op, fds[i], strerror(errno));
+			die_errno("Setting TCP_REPAIR to %i on socket %i",
+				   op, fds[i]);
 		}
 
 		/* Close _our_ copy */
@@ -157,7 +162,7 @@ loop:
 
 	/* Confirm setting by echoing the command back */
 	if (send(s, &cmd, sizeof(cmd), 0) < 0)
-		die(1, "Reply to %i: %s", op, strerror(errno));
+		die_errno("Reply to %i", op);
 
 	goto loop;
 
-- 
@@ -24,7 +24,6 @@
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <string.h>
 #include <limits.h>
 #include <unistd.h>
 #include <netdb.h>
@@ -47,6 +46,14 @@
 		_exit(status);			\
 	} while (0)
 
+#define die_errno(...)					\
+	do {						\
+		int err_ = errno;			\
+		fprintf(stderr, __VA_ARGS__);		\
+		fprintf(stderr, ": %d\n", err_);	\
+		_exit(1);				\
+	} while (0)
+
 /**
  * main() - Entry point and whole program with loop
  * @argc:	Argument count, must be 2
@@ -80,7 +87,7 @@ int main(int argc, char **argv)
 	prog.filter = filter_repair;
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
 	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog))
-		die(1, "Failed to apply seccomp filter");
+		die_errno("Failed to apply seccomp filter");
 
 	iov = (struct iovec){ &cmd, sizeof(cmd) };
 	msg = (struct msghdr){ .msg_name = NULL, .msg_namelen = 0,
@@ -98,12 +105,10 @@ int main(int argc, char **argv)
 		die(2, "Invalid socket path: %s", argv[1]);
 
 	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
-		die(1, "Failed to create AF_UNIX socket: %i", errno);
+		die_errno("Failed to create AF_UNIX socket");
 
-	if (connect(s, (struct sockaddr *)&a, sizeof(a))) {
-		die(1, "Failed to connect to %s: %s", argv[1],
-		    strerror(errno));
-	}
+	if (connect(s, (struct sockaddr *)&a, sizeof(a)))
+		die_errno("Failed to connect to %s", argv[1]);
 
 loop:
 	ret = recvmsg(s, &msg, 0);
@@ -111,7 +116,7 @@ loop:
 		if (errno == ECONNRESET)
 			ret = 0;
 		else
-			die(1, "Failed to read message: %i", errno);
+			die_errno("Failed to read message");
 	}
 
 	if (!ret)	/* Done */
@@ -147,8 +152,8 @@ loop:
 
 	for (i = 0; i < n; i++) {
 		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
-			die(1, "Setting TCP_REPAIR to %i on socket %i: %s",
-			    op, fds[i], strerror(errno));
+			die_errno("Setting TCP_REPAIR to %i on socket %i",
+				   op, fds[i]);
 		}
 
 		/* Close _our_ copy */
@@ -157,7 +162,7 @@ loop:
 
 	/* Confirm setting by echoing the command back */
 	if (send(s, &cmd, sizeof(cmd), 0) < 0)
-		die(1, "Reply to %i: %s", op, strerror(errno));
+		die_errno("Reply to %i", op);
 
 	goto loop;
 
-- 
2.48.1

[PATCH 3/4] passt-repair: Improve validation of anciliary data length

[David Gibson]

At present we use a rather awkward loop to invert CMSG_LEN() in order to
determine how many fds we have been passed as anciliary data.  We can do
a bit better with some pointer trickery.  This also lets us validate the
number of fds we've been passed a bit more naturally.

While we're there, allow an empty message (n == 0) because why not.

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

diff --git a/passt-repair.c b/passt-repair.c
index 3c358e27..64b926bd 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -77,7 +77,7 @@ int main(int argc, char **argv)
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
 	struct iovec iov;
-	size_t cmsg_len;
+	size_t fdlen;
 	int op;
 
 	prctl(PR_SET_DUMPABLE, 0);
@@ -122,27 +122,15 @@ loop:
 	if (!ret)	/* Done */
 		_exit(0);
 
-	if (!cmsg ||
-	    cmsg->cmsg_len < CMSG_LEN(sizeof(int)) ||
-	    cmsg->cmsg_len > CMSG_LEN(sizeof(int) * SCM_MAX_FD) ||
-	    cmsg->cmsg_type != SCM_RIGHTS)
+	if (!cmsg || cmsg->cmsg_type != SCM_RIGHTS)
 		die(1, "No/bad ancillary data from peer");
 
-	/* No inverse formula for CMSG_LEN(x), and building one with CMSG_LEN(0)
-	 * works but there's no guarantee it does. Search the whole domain.
-	 */
-	for (i = 1; i <= SCM_MAX_FD; i++) {
-		if (CMSG_LEN(sizeof(int) * i) == cmsg->cmsg_len) {
-			n = i;
-			break;
-		}
-	}
-	if (!n) {
-		cmsg_len = cmsg->cmsg_len; /* socklen_t is 'unsigned' on musl */
-		die(1, "Invalid ancillary data length %zu from peer", cmsg_len);
-	}
+	fdlen = ((char *)cmsg + cmsg->cmsg_len) - (char *)CMSG_DATA(cmsg);
+	if (fdlen % sizeof(int) != 0 || fdlen > sizeof(fds))
+		die(1, "Invalid SCM_RIGHTS payload length %zu from peer", fdlen);
+	n = fdlen / sizeof(int);
 
-	memcpy(fds, CMSG_DATA(cmsg), sizeof(int) * n);
+	memcpy(fds, CMSG_DATA(cmsg), fdlen);
 
 	if (cmd != TCP_REPAIR_ON && cmd != TCP_REPAIR_OFF &&
 	    cmd != TCP_REPAIR_OFF_NO_WP)
-- 
@@ -77,7 +77,7 @@ int main(int argc, char **argv)
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
 	struct iovec iov;
-	size_t cmsg_len;
+	size_t fdlen;
 	int op;
 
 	prctl(PR_SET_DUMPABLE, 0);
@@ -122,27 +122,15 @@ loop:
 	if (!ret)	/* Done */
 		_exit(0);
 
-	if (!cmsg ||
-	    cmsg->cmsg_len < CMSG_LEN(sizeof(int)) ||
-	    cmsg->cmsg_len > CMSG_LEN(sizeof(int) * SCM_MAX_FD) ||
-	    cmsg->cmsg_type != SCM_RIGHTS)
+	if (!cmsg || cmsg->cmsg_type != SCM_RIGHTS)
 		die(1, "No/bad ancillary data from peer");
 
-	/* No inverse formula for CMSG_LEN(x), and building one with CMSG_LEN(0)
-	 * works but there's no guarantee it does. Search the whole domain.
-	 */
-	for (i = 1; i <= SCM_MAX_FD; i++) {
-		if (CMSG_LEN(sizeof(int) * i) == cmsg->cmsg_len) {
-			n = i;
-			break;
-		}
-	}
-	if (!n) {
-		cmsg_len = cmsg->cmsg_len; /* socklen_t is 'unsigned' on musl */
-		die(1, "Invalid ancillary data length %zu from peer", cmsg_len);
-	}
+	fdlen = ((char *)cmsg + cmsg->cmsg_len) - (char *)CMSG_DATA(cmsg);
+	if (fdlen % sizeof(int) != 0 || fdlen > sizeof(fds))
+		die(1, "Invalid SCM_RIGHTS payload length %zu from peer", fdlen);
+	n = fdlen / sizeof(int);
 
-	memcpy(fds, CMSG_DATA(cmsg), sizeof(int) * n);
+	memcpy(fds, CMSG_DATA(cmsg), fdlen);
 
 	if (cmd != TCP_REPAIR_ON && cmd != TCP_REPAIR_OFF &&
 	    cmd != TCP_REPAIR_OFF_NO_WP)
-- 
2.48.1

[PATCH 4/4] passt-repair: Allow passt-repair to report partial failures

[David Gibson]

Although it's unlikely it's possible that passt-repair could encounter an
error on some but not all the fds in a batch it is given.  At present,
passt-repair will die in this situation, meaning that passt knows something
went wrong, but doesn't know the state of the fds it passed in the last
batch.

Change the passt-repair protocol, so that instead of replying with a copy
of the command byte, it replies with the number of fds that were
successfuly handled.  We always process the fds in the order given, and
bail out on the first error, so this is sufficient to tell passt the state
of all the fds it passed.

For now we don't use that extra information in passt, we just update it to
understand the new protocol.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt-repair.c | 29 +++++++++++++++++++++++------
 repair.c       | 11 +++++++----
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/passt-repair.c b/passt-repair.c
index 64b926bd..f35e74ba 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -16,6 +16,7 @@
  * off. Reply by echoing the command. Exit on EOF.
  */
 
+#include <assert.h>
 #include <sys/prctl.h>
 #include <sys/types.h>
 #include <sys/socket.h>
@@ -38,6 +39,7 @@
 #include "seccomp_repair.h"
 
 #define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */
+static_assert(SCM_MAX_FD < UCHAR_MAX, "Batch sizes must fit in 8 bits");
 
 #define die(status, ...)			\
 	do {					\
@@ -77,6 +79,7 @@ int main(int argc, char **argv)
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
 	struct iovec iov;
+	uint8_t reply;
 	size_t fdlen;
 	int op;
 
@@ -140,16 +143,30 @@ loop:
 
 	for (i = 0; i < n; i++) {
 		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
-			die_errno("Setting TCP_REPAIR to %i on socket %i",
-				   op, fds[i]);
+			fprintf(stderr,
+				"Setting TCP_REPAIR to %i on socket %i: %d\n",
+				op, fds[i], errno);
+			break;
 		}
+	}
+
+	if (i < n)
+		fprintf(stderr, "Failed to handle %d/%d sockets", n - i, n);
+
+	reply = i;
 
-		/* Close _our_ copy */
-		close(fds[i]);
+	/* Close all _our_, even if we failed to setsockopt() on some */
+	for (i = 0; i < n; i++) {
+		if (close(fds[i]))
+			/* This is unlikely, but would be painful to debug if it
+			 * ever did happen and we didn't report it.
+			 */
+			die_errno("Couldn't close socket %i", fds[i]);
 	}
 
-	/* Confirm setting by echoing the command back */
-	if (send(s, &cmd, sizeof(cmd), 0) < 0)
+
+	/* Confirm by sending number of fds succesfully handled back */
+	if (send(s, &reply, sizeof(reply), 0) < 0)
 		die_errno("Reply to %i", op);
 
 	goto loop;
diff --git a/repair.c b/repair.c
index 3ee089f0..343faa22 100644
--- a/repair.c
+++ b/repair.c
@@ -34,7 +34,7 @@ static int repair_fds[SCM_MAX_FD];
 static int8_t repair_cmd;
 
 /* Number of pending file descriptors set in @repair_fds */
-static int repair_nfds;
+static unsigned repair_nfds;
 
 /**
  * repair_sock_init() - Start listening for connections on helper socket
@@ -151,7 +151,8 @@ int repair_flush(struct ctx *c)
 	struct iovec iov = { &repair_cmd, sizeof(repair_cmd) };
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
-	int8_t reply;
+	unsigned nfds;
+	uint8_t reply;
 
 	if (!repair_nfds)
 		return 0;
@@ -169,6 +170,7 @@ int repair_flush(struct ctx *c)
 	cmsg->cmsg_len = CMSG_LEN(sizeof(int) * repair_nfds);
 	memcpy(CMSG_DATA(cmsg), repair_fds, sizeof(int) * repair_nfds);
 
+	nfds = repair_nfds;
 	repair_nfds = 0;
 
 	if (sendmsg(c->fd_repair, &msg, 0) < 0) {
@@ -185,8 +187,9 @@ int repair_flush(struct ctx *c)
 		return ret;
 	}
 
-	if (reply != repair_cmd) {
-		err("Unexpected reply from TCP_REPAIR helper: %d", reply);
+	if (reply != nfds) {
+		err("TCP_REPAIR helper was unable to process %d fds",
+		    nfds - reply);
 		repair_close(c);
 		return -ENXIO;
 	}
-- 
@@ -34,7 +34,7 @@ static int repair_fds[SCM_MAX_FD];
 static int8_t repair_cmd;
 
 /* Number of pending file descriptors set in @repair_fds */
-static int repair_nfds;
+static unsigned repair_nfds;
 
 /**
  * repair_sock_init() - Start listening for connections on helper socket
@@ -151,7 +151,8 @@ int repair_flush(struct ctx *c)
 	struct iovec iov = { &repair_cmd, sizeof(repair_cmd) };
 	struct cmsghdr *cmsg;
 	struct msghdr msg;
-	int8_t reply;
+	unsigned nfds;
+	uint8_t reply;
 
 	if (!repair_nfds)
 		return 0;
@@ -169,6 +170,7 @@ int repair_flush(struct ctx *c)
 	cmsg->cmsg_len = CMSG_LEN(sizeof(int) * repair_nfds);
 	memcpy(CMSG_DATA(cmsg), repair_fds, sizeof(int) * repair_nfds);
 
+	nfds = repair_nfds;
 	repair_nfds = 0;
 
 	if (sendmsg(c->fd_repair, &msg, 0) < 0) {
@@ -185,8 +187,9 @@ int repair_flush(struct ctx *c)
 		return ret;
 	}
 
-	if (reply != repair_cmd) {
-		err("Unexpected reply from TCP_REPAIR helper: %d", reply);
+	if (reply != nfds) {
+		err("TCP_REPAIR helper was unable to process %d fds",
+		    nfds - reply);
 		repair_close(c);
 		return -ENXIO;
 	}
-- 
2.48.1