public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject: [PATCH 6/8] More deterministic detection of whether argument is a PID, PATH or NAME
Date: Fri, 26 Aug 2022 14:58:37 +1000	[thread overview]
Message-ID: <20220826045839.1112152-7-david@gibson.dropbear.id.au> (raw)
In-Reply-To: <20220826045839.1112152-1-david@gibson.dropbear.id.au>

[-- Attachment #1: Type: text/plain, Size: 8229 bytes --]

pasta takes as its only non-option argument either a PID to attach to the
namespaces of, a PATH to a network namespace or a NAME of a network
namespace (relative to /run/netns).  Currently to determine which it is
we try all 3 in that order, and if anything goes wrong we move onto the
next.

This has the potential to cause very confusing failure modes.  e.g. if the
argument is intended to be a network namespace name, but a (non-namespace)
file of the same name exists in the current directory.

Make behaviour more predictable by choosing how to treat the argument based
only on the argument's contents, not anything else on the system:
  - If it's a decimal integer treat it as a PID
  - Otherwise, if it has no '/' characters, treat it as a netns name
    (ip-netns doesn't allow '/' in netns names)
  - Otherwise, treat it as a netns path

If you want to open a persistent netns in the current directory, you can
use './netns'.

This also allows us to split the parsing of the PID|PATH|NAME option from
the actual opening of the namespaces.  In turn that allows us to put the
opening of existing namespaces next to the opening of new namespaces in
pasta_start_ns.  That makes the logical flow easier to follow and will
enable later cleanups.

Caveats:
 - The separation of functions mean we will always generate the basename
   and dirname for the netns_quit system, even when using PID namespaces.
   This is pointless, since the netns_quit system doesn't work for non
   persistent namespaces, but is harmless.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 172 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 90 insertions(+), 82 deletions(-)

diff --git a/conf.c b/conf.c
index 47f3e41..10b7b9f 100644
--- a/conf.c
+++ b/conf.c
@@ -489,6 +489,51 @@ out:
 		warn("Couldn't get any nameserver address");
 }
 
+/**
+ * conf_ns_opt() - Parse non-option argument to namespace paths
+ * @userns:	buffer of size PATH_MAX, initially contains --userns
+ *		argument (may be empty), updated with userns path
+ * @netns:	buffer of size PATH_MAX, updated with netns path
+ * @arg:	PID, path or name of network namespace
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+static int conf_ns_opt(char *userns, char *netns, const char *arg)
+{
+	char *endptr;
+	long pidval;
+	int ret;
+
+	pidval = strtol(arg, &endptr, 10);
+	if (!*endptr) {
+		/* Looks like a pid */
+		if (pidval < 0 || pidval > INT_MAX) {
+			err("Invalid PID %s", arg);
+			return -EINVAL;
+		}
+
+		snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
+		if (!*userns)
+			snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", pidval);
+		return 0;
+	}
+
+	if (!strchr(arg, '/')) {
+		/* looks like a netns name */
+		ret = snprintf(netns, PATH_MAX, "%s/%s", NETNS_RUN_DIR, arg);
+	} else {
+		/* otherwise assume it's a netns path */
+		ret = snprintf(netns, PATH_MAX, "%s", arg);
+	}
+
+	if (ret <= 0 || ret > PATH_MAX) {
+		err("Network namespace name/path %s too long");
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
 /**
  * conf_ns_check() - Check if we can enter configured namespaces
  * @arg:	Execution context
@@ -508,102 +553,58 @@ static int conf_ns_check(void *arg)
 }
 
 /**
- * conf_ns_opt() - Open network, user namespaces descriptors from configuration
- * @c:			Execution context
- * @conf_userns:	--userns argument, can be an empty string
- * @optarg:		PID, path or name of namespace
+ * conf_ns_open() - Open network, user namespaces descriptors from configuration
+ * @c:		Execution context
+ * @userns:	--userns argument, can be an empty string
+ * @netns:	network namespace path
  *
  * Return: 0 on success, negative error code otherwise
  */
-static int conf_ns_opt(struct ctx *c,
-		       const char *conf_userns, const char *optarg)
+static int conf_ns_open(struct ctx *c, const char *userns, const char *netns)
 {
-	int ufd = -1, nfd = -1, try, ret, netns_only_reset = c->netns_only;
-	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX];
-	char *endptr;
-	long pid_arg;
-	pid_t pid;
+	int ufd = -1, nfd = -1;
 
-	if (c->netns_only && *conf_userns) {
+	if (c->netns_only && *userns) {
 		err("Both --userns and --netns-only given");
 		return -EINVAL;
 	}
 
-	/* It might be a PID, a netns path, or a netns name */
-	for (try = 0; try < 3; try++) {
-		if (try == 0) {
-			pid_arg = strtol(optarg, &endptr, 10);
-			if (*endptr || pid_arg < 0 || pid_arg > INT_MAX)
-				continue;
-
-			pid = pid_arg;
-
-			if (!*conf_userns && !c->netns_only) {
-				ret = snprintf(userns, PATH_MAX,
-					       "/proc/%i/ns/user", pid);
-				if (ret <= 0 || ret > (int)sizeof(userns))
-					continue;
-			}
-			ret = snprintf(netns, PATH_MAX, "/proc/%i/ns/net", pid);
-			if (ret <= 0 || ret > (int)sizeof(netns))
-				continue;
-		} else if (try == 1) {
-			if (!*conf_userns)
-				c->netns_only = 1;
-
-			ret = snprintf(netns, PATH_MAX, "%s", optarg);
-			if (ret <= 0 || ret > (int)sizeof(userns))
-				continue;
-		} else if (try == 2) {
-			ret = snprintf(netns, PATH_MAX, "%s/%s",
-				       NETNS_RUN_DIR, optarg);
-			if (ret <= 0 || ret > (int)sizeof(netns))
-				continue;
-		}
-
-		if (!c->netns_only) {
-			if (*conf_userns)
-				ufd = open(conf_userns, O_RDONLY | O_CLOEXEC);
-			else if (*userns)
-				ufd = open(userns, O_RDONLY | O_CLOEXEC);
-		}
-
-		nfd = open(netns, O_RDONLY | O_CLOEXEC);
-
-		if (nfd == -1 || (ufd == -1 && !c->netns_only)) {
-			if (nfd >= 0)
-				close(nfd);
-
-			if (ufd >= 0)
-				close(ufd);
+	nfd = open(netns, O_RDONLY | O_CLOEXEC);
+	if (nfd < 0) {
+		err("Couldn't open network namespace %s", netns);
+		return -ENOENT;
+	}
 
-			continue;
+	if (!c->netns_only && *userns) {
+		ufd = open(userns, O_RDONLY | O_CLOEXEC);
+		if (ufd < 0) {
+			close(nfd);
+			err("Couldn't open user namespace %s", userns);
+			return -ENOENT;
 		}
+	}
 
-		c->pasta_netns_fd = nfd;
-		c->pasta_userns_fd = ufd;
-
-		NS_CALL(conf_ns_check, c);
+	c->pasta_netns_fd = nfd;
+	c->pasta_userns_fd = ufd;
+	c->netns_only = !*userns;
 
-		if (c->pasta_netns_fd >= 0) {
-			char buf[PATH_MAX];
+	NS_CALL(conf_ns_check, c);
 
-			if (try == 0 || c->no_netns_quit)
-				return 0;
+	if (c->pasta_netns_fd < 0) {
+		err("Couldn't switch to pasta namespaces");
+		return -ENOENT;
+	}
 
-			strncpy(buf, netns, PATH_MAX);
-			strncpy(c->netns_base, basename(buf), PATH_MAX - 1);
-			strncpy(buf, netns, PATH_MAX);
-			strncpy(c->netns_dir, dirname(buf), PATH_MAX - 1);
+	if (!c->no_netns_quit) {
+		char buf[PATH_MAX];
 
-			return 0;
-		}
+		strncpy(buf, netns, PATH_MAX);
+		strncpy(c->netns_base, basename(buf), PATH_MAX - 1);
+		strncpy(buf, netns, PATH_MAX);
+		strncpy(c->netns_dir, dirname(buf), PATH_MAX - 1);
 	}
 
-	c->netns_only = netns_only_reset;
-
-	err("Namespace %s not found", optarg);
-	return -ENOENT;
+	return 0;
 }
 
 /**
@@ -1051,7 +1052,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{ 0 },
 	};
 	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
-	char userns[PATH_MAX] = { 0 };
+	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
 	enum conf_port_type tcp_tap = 0, tcp_init = 0;
 	enum conf_port_type udp_tap = 0, udp_init = 0;
 	bool v4_only = false, v6_only = false;
@@ -1446,7 +1447,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	check_root(c);
 
 	if (c->mode == MODE_PASTA && optind + 1 == argc) {
-		ret = conf_ns_opt(c, userns, argv[optind]);
+		ret = conf_ns_opt(userns, netns, argv[optind]);
 		if (ret < 0)
 			usage(argv[0]);
 	} else if (c->mode == MODE_PASTA && *userns && optind == argc) {
@@ -1459,8 +1460,15 @@ void conf(struct ctx *c, int argc, char **argv)
 	if (c->pasta_conf_ns)
 		c->no_ra = 1;
 
-	if (c->mode == MODE_PASTA && c->pasta_netns_fd == -1)
-		pasta_start_ns(c);
+	if (c->mode == MODE_PASTA) {
+		if (*netns) {
+			ret = conf_ns_open(c, userns, netns);
+			if (ret < 0)
+				usage(argv[0]);
+		} else {
+			pasta_start_ns(c);
+		}
+	}
 
 	if (nl_sock_init(c)) {
 		err("Failed to get netlink socket");
-- 
@@ -489,6 +489,51 @@ out:
 		warn("Couldn't get any nameserver address");
 }
 
+/**
+ * conf_ns_opt() - Parse non-option argument to namespace paths
+ * @userns:	buffer of size PATH_MAX, initially contains --userns
+ *		argument (may be empty), updated with userns path
+ * @netns:	buffer of size PATH_MAX, updated with netns path
+ * @arg:	PID, path or name of network namespace
+ *
+ * Return: 0 on success, negative error code otherwise
+ */
+static int conf_ns_opt(char *userns, char *netns, const char *arg)
+{
+	char *endptr;
+	long pidval;
+	int ret;
+
+	pidval = strtol(arg, &endptr, 10);
+	if (!*endptr) {
+		/* Looks like a pid */
+		if (pidval < 0 || pidval > INT_MAX) {
+			err("Invalid PID %s", arg);
+			return -EINVAL;
+		}
+
+		snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
+		if (!*userns)
+			snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", pidval);
+		return 0;
+	}
+
+	if (!strchr(arg, '/')) {
+		/* looks like a netns name */
+		ret = snprintf(netns, PATH_MAX, "%s/%s", NETNS_RUN_DIR, arg);
+	} else {
+		/* otherwise assume it's a netns path */
+		ret = snprintf(netns, PATH_MAX, "%s", arg);
+	}
+
+	if (ret <= 0 || ret > PATH_MAX) {
+		err("Network namespace name/path %s too long");
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
 /**
  * conf_ns_check() - Check if we can enter configured namespaces
  * @arg:	Execution context
@@ -508,102 +553,58 @@ static int conf_ns_check(void *arg)
 }
 
 /**
- * conf_ns_opt() - Open network, user namespaces descriptors from configuration
- * @c:			Execution context
- * @conf_userns:	--userns argument, can be an empty string
- * @optarg:		PID, path or name of namespace
+ * conf_ns_open() - Open network, user namespaces descriptors from configuration
+ * @c:		Execution context
+ * @userns:	--userns argument, can be an empty string
+ * @netns:	network namespace path
  *
  * Return: 0 on success, negative error code otherwise
  */
-static int conf_ns_opt(struct ctx *c,
-		       const char *conf_userns, const char *optarg)
+static int conf_ns_open(struct ctx *c, const char *userns, const char *netns)
 {
-	int ufd = -1, nfd = -1, try, ret, netns_only_reset = c->netns_only;
-	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX];
-	char *endptr;
-	long pid_arg;
-	pid_t pid;
+	int ufd = -1, nfd = -1;
 
-	if (c->netns_only && *conf_userns) {
+	if (c->netns_only && *userns) {
 		err("Both --userns and --netns-only given");
 		return -EINVAL;
 	}
 
-	/* It might be a PID, a netns path, or a netns name */
-	for (try = 0; try < 3; try++) {
-		if (try == 0) {
-			pid_arg = strtol(optarg, &endptr, 10);
-			if (*endptr || pid_arg < 0 || pid_arg > INT_MAX)
-				continue;
-
-			pid = pid_arg;
-
-			if (!*conf_userns && !c->netns_only) {
-				ret = snprintf(userns, PATH_MAX,
-					       "/proc/%i/ns/user", pid);
-				if (ret <= 0 || ret > (int)sizeof(userns))
-					continue;
-			}
-			ret = snprintf(netns, PATH_MAX, "/proc/%i/ns/net", pid);
-			if (ret <= 0 || ret > (int)sizeof(netns))
-				continue;
-		} else if (try == 1) {
-			if (!*conf_userns)
-				c->netns_only = 1;
-
-			ret = snprintf(netns, PATH_MAX, "%s", optarg);
-			if (ret <= 0 || ret > (int)sizeof(userns))
-				continue;
-		} else if (try == 2) {
-			ret = snprintf(netns, PATH_MAX, "%s/%s",
-				       NETNS_RUN_DIR, optarg);
-			if (ret <= 0 || ret > (int)sizeof(netns))
-				continue;
-		}
-
-		if (!c->netns_only) {
-			if (*conf_userns)
-				ufd = open(conf_userns, O_RDONLY | O_CLOEXEC);
-			else if (*userns)
-				ufd = open(userns, O_RDONLY | O_CLOEXEC);
-		}
-
-		nfd = open(netns, O_RDONLY | O_CLOEXEC);
-
-		if (nfd == -1 || (ufd == -1 && !c->netns_only)) {
-			if (nfd >= 0)
-				close(nfd);
-
-			if (ufd >= 0)
-				close(ufd);
+	nfd = open(netns, O_RDONLY | O_CLOEXEC);
+	if (nfd < 0) {
+		err("Couldn't open network namespace %s", netns);
+		return -ENOENT;
+	}
 
-			continue;
+	if (!c->netns_only && *userns) {
+		ufd = open(userns, O_RDONLY | O_CLOEXEC);
+		if (ufd < 0) {
+			close(nfd);
+			err("Couldn't open user namespace %s", userns);
+			return -ENOENT;
 		}
+	}
 
-		c->pasta_netns_fd = nfd;
-		c->pasta_userns_fd = ufd;
-
-		NS_CALL(conf_ns_check, c);
+	c->pasta_netns_fd = nfd;
+	c->pasta_userns_fd = ufd;
+	c->netns_only = !*userns;
 
-		if (c->pasta_netns_fd >= 0) {
-			char buf[PATH_MAX];
+	NS_CALL(conf_ns_check, c);
 
-			if (try == 0 || c->no_netns_quit)
-				return 0;
+	if (c->pasta_netns_fd < 0) {
+		err("Couldn't switch to pasta namespaces");
+		return -ENOENT;
+	}
 
-			strncpy(buf, netns, PATH_MAX);
-			strncpy(c->netns_base, basename(buf), PATH_MAX - 1);
-			strncpy(buf, netns, PATH_MAX);
-			strncpy(c->netns_dir, dirname(buf), PATH_MAX - 1);
+	if (!c->no_netns_quit) {
+		char buf[PATH_MAX];
 
-			return 0;
-		}
+		strncpy(buf, netns, PATH_MAX);
+		strncpy(c->netns_base, basename(buf), PATH_MAX - 1);
+		strncpy(buf, netns, PATH_MAX);
+		strncpy(c->netns_dir, dirname(buf), PATH_MAX - 1);
 	}
 
-	c->netns_only = netns_only_reset;
-
-	err("Namespace %s not found", optarg);
-	return -ENOENT;
+	return 0;
 }
 
 /**
@@ -1051,7 +1052,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{ 0 },
 	};
 	struct get_bound_ports_ns_arg ns_ports_arg = { .c = c };
-	char userns[PATH_MAX] = { 0 };
+	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
 	enum conf_port_type tcp_tap = 0, tcp_init = 0;
 	enum conf_port_type udp_tap = 0, udp_init = 0;
 	bool v4_only = false, v6_only = false;
@@ -1446,7 +1447,7 @@ void conf(struct ctx *c, int argc, char **argv)
 	check_root(c);
 
 	if (c->mode == MODE_PASTA && optind + 1 == argc) {
-		ret = conf_ns_opt(c, userns, argv[optind]);
+		ret = conf_ns_opt(userns, netns, argv[optind]);
 		if (ret < 0)
 			usage(argv[0]);
 	} else if (c->mode == MODE_PASTA && *userns && optind == argc) {
@@ -1459,8 +1460,15 @@ void conf(struct ctx *c, int argc, char **argv)
 	if (c->pasta_conf_ns)
 		c->no_ra = 1;
 
-	if (c->mode == MODE_PASTA && c->pasta_netns_fd == -1)
-		pasta_start_ns(c);
+	if (c->mode == MODE_PASTA) {
+		if (*netns) {
+			ret = conf_ns_open(c, userns, netns);
+			if (ret < 0)
+				usage(argv[0]);
+		} else {
+			pasta_start_ns(c);
+		}
+	}
 
 	if (nl_sock_init(c)) {
 		err("Failed to get netlink socket");
-- 
2.37.2


  parent reply	other threads:[~2022-08-26  4:58 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-26  4:58 [PATCH 0/8] Allow pasta to take a command to spawn instead of shell David Gibson
2022-08-26  4:58 ` [PATCH 1/8] conf: Make the argument to --pcap option mandatory David Gibson
2022-08-26  4:58 ` [PATCH 2/8] conf: Use "-D none" and "-S none" instead of missing empty option arguments David Gibson
2022-08-30 17:41   ` Stefano Brivio
2022-08-26  4:58 ` [PATCH 3/8] Correct manpage for --userns David Gibson
2022-08-26  4:58 ` [PATCH 4/8] Remove --nsrun-dir option David Gibson
2022-08-26  4:58 ` [PATCH 5/8] Move ENOENT error message into conf_ns_opt() David Gibson
2022-08-26  4:58 ` David Gibson [this message]
2022-08-26  4:58 ` [PATCH 7/8] Use explicit --netns option rather than multiplexing with PID David Gibson
2022-08-29 19:16   ` Stefano Brivio
2022-08-30  1:12     ` David Gibson
2022-08-30  8:25       ` Stefano Brivio
2022-08-26  4:58 ` [PATCH 8/8] Allow pasta to take a command to execute David Gibson
2022-08-29 19:16   ` Stefano Brivio
2022-08-30  1:16     ` David Gibson
2022-08-30  8:26       ` Stefano Brivio
2022-08-30 17:41         ` Stefano Brivio
2022-09-01 10:07 ` [PATCH 0/8] Allow pasta to take a command to spawn instead of shell Stefano Brivio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220826045839.1112152-7-david@gibson.dropbear.id.au \
    --to=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).