Skip to contentSkip to conversation

ipnetns: cleanup and harden processing of netns ids

PATCH In review Guillaume Nault 2 participants 7 messages Nov 8, 2019 – Nov 9, 2019

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Guillaume Nault

author
#0 lore raw

cover ipnetns: cleanup and harden processing of netns ids

It's currently hard to review ipnetns. The netns ids are inconsistently treated as signed or unsigned and most helper functions aren't prepared to use negative ids.

Netns id attributes can be negative: NETNSA_NSID_NOT_ASSIGNED == -1. So let's consistently treat nsids as signed and also reject negative values in functions that are supposed to only handle assigned netns ids.

While there, let's drop the extra blank line generated by some command line parsing errors (patch 5/5).

Guillaume Nault (5):
ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed
ipnetns: fix misleading comment about 'ip monitor nsid'
ipnetns: harden helper functions wrt. negative netns ids
ipnetns: don't print unassigned nsid in json export
ipnetns: remove blank lines printed by invarg() messages

 ip/ipnetns.c | 49 +++++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 20 deletions(-)
Signature · 1 line
2.21.0
author
#2 lore raw

PATCH 2/5 ipnetns: fix misleading comment about 'ip monitor nsid'

'ip monitor nsid' doesn't call print_nsid().

Signed-off-by Guillaume Nault <[email protected]>
---
 ip/ipnetns.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
ip/ipnetns.c +1 -1
index 0a7912df..b02e0a8a 100644
@@ -340,7 +340,7 @@ int print_nsid(struct nlmsghdr *n, void *arg)
 		netns_map_del(c);
 	}
 
-	/* During 'ip monitor nsid', no chance to have new nsid in cache. */
+	/* nsid might not be in cache */
 	if (c == NULL && n->nlmsg_type == RTM_NEWNSID)
 		if (netns_get_name(nsid, name) == 0) {
 			print_string(PRINT_ANY, "name",
Signature · 1 line
2.21.0
author
#1 lore raw

PATCH 1/5 ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed

In reply to Guillaume Nault

These attributes are signed (with -1 meaning NETNSA_NSID_NOT_ASSIGNED). So let's use rta_getattr_s32() and print_int() instead of their unsigned counterpart to avoid confusion.

Signed-off-by Guillaume Nault <[email protected]>
---
 ip/ipnetns.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
ip/ipnetns.c +7 -8
index 20110ef0..0a7912df 100644
@@ -137,7 +137,7 @@ int get_netnsid_from_name(const char *name)
 	parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len);
 
 	if (tb[NETNSA_NSID]) {
-		ret = rta_getattr_u32(tb[NETNSA_NSID]);
+		ret = rta_getattr_s32(tb[NETNSA_NSID]);
 	}
 
 out:

@@ -317,20 +317,20 @@ int print_nsid(struct nlmsghdr *n, void *arg)
 	if (n->nlmsg_type == RTM_DELNSID)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
-	nsid = rta_getattr_u32(tb[NETNSA_NSID]);
+	nsid = rta_getattr_s32(tb[NETNSA_NSID]);
 	if (nsid < 0)
 		print_string(PRINT_ANY, "nsid", "nsid %s ", "not-assigned");
 	else
-		print_uint(PRINT_ANY, "nsid", "nsid %u ", nsid);
+		print_int(PRINT_ANY, "nsid", "nsid %d ", nsid);
 
 	if (tb[NETNSA_CURRENT_NSID]) {
-		current = rta_getattr_u32(tb[NETNSA_CURRENT_NSID]);
+		current = rta_getattr_s32(tb[NETNSA_CURRENT_NSID]);
 		if (current < 0)
 			print_string(PRINT_ANY, "current-nsid",
 				     "current-nsid %s ", "not-assigned");
 		else
-			print_uint(PRINT_ANY, "current-nsid",
-				   "current-nsid %u ", current);
+			print_int(PRINT_ANY, "current-nsid",
+				  "current-nsid %d ", current);
 	}
 
 	c = netns_map_get_by_nsid(tb[NETNSA_CURRENT_NSID] ? current : nsid);

@@ -491,8 +491,7 @@ static int netns_list(int argc, char **argv)
 		if (ipnetns_have_nsid()) {
 			id = get_netnsid_from_name(entry->d_name);
 			if (id >= 0)
-				print_uint(PRINT_ANY, "id",
-					   " (id: %d)", id);
+				print_int(PRINT_ANY, "id", " (id: %d)", id);
 		}
 		print_string(PRINT_FP, NULL, "\n", NULL);
 		close_json_object();
Signature · 1 line
2.21.0
author
#3 lore raw

PATCH 3/5 ipnetns: harden helper functions wrt. negative netns ids

In reply to Guillaume Nault

Negative values are invalid netns ids. Ensure that helper functions don't accidentally try to process them.

Signed-off-by Guillaume Nault <[email protected]>
---
 ip/ipnetns.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
ip/ipnetns.c +12 -2
index b02e0a8a..77531d6c 100644
@@ -161,9 +161,13 @@ static struct hlist_head	name_head[NSIDMAP_SIZE];
 
 static struct nsid_cache *netns_map_get_by_nsid(int nsid)
 {
-	uint32_t h = NSID_HASH_NSID(nsid);
 	struct hlist_node *n;
+	uint32_t h;
+
+	if (nsid < 0)
+		return NULL;
 
+	h = NSID_HASH_NSID(nsid);
 	hlist_for_each(n, &nsid_head[h]) {
 		struct nsid_cache *c = container_of(n, struct nsid_cache,
 						    nsid_hash);

@@ -178,6 +182,9 @@ char *get_name_from_nsid(int nsid)
 {
 	struct nsid_cache *c;
 
+	if (nsid < 0)
+		return NULL;
+
 	netns_nsid_socket_init();
 	netns_map_init();
 

@@ -266,6 +273,9 @@ static int netns_get_name(int nsid, char *name)
 	DIR *dir;
 	int id;
 
+	if (nsid < 0)
+		return -EINVAL;
+
 	dir = opendir(NETNS_RUN_DIR);
 	if (!dir)
 		return -ENOENT;

@@ -277,7 +287,7 @@ static int netns_get_name(int nsid, char *name)
 			continue;
 		id = get_netnsid_from_name(entry->d_name);
 
-		if (nsid == id) {
+		if (id >= 0 && nsid == id) {
 			strcpy(name, entry->d_name);
 			closedir(dir);
 			return 0;
Signature · 1 line
2.21.0
author
#4 lore raw

PATCH 4/5 ipnetns: don't print unassigned nsid in json export

In reply to Guillaume Nault

Don't output the nsid and current-nsid json keys if they're not set. Otherwise a parser would have to special case the "not-assigned" string.

Signed-off-by Guillaume Nault <[email protected]>
---
 ip/ipnetns.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
ip/ipnetns.c +3 -3
index 77531d6c..4eb4a09a 100644
@@ -329,15 +329,15 @@ int print_nsid(struct nlmsghdr *n, void *arg)
 
 	nsid = rta_getattr_s32(tb[NETNSA_NSID]);
 	if (nsid < 0)
-		print_string(PRINT_ANY, "nsid", "nsid %s ", "not-assigned");
+		print_string(PRINT_FP, NULL, "nsid unassigned ", NULL);
 	else
 		print_int(PRINT_ANY, "nsid", "nsid %d ", nsid);
 
 	if (tb[NETNSA_CURRENT_NSID]) {
 		current = rta_getattr_s32(tb[NETNSA_CURRENT_NSID]);
 		if (current < 0)
-			print_string(PRINT_ANY, "current-nsid",
-				     "current-nsid %s ", "not-assigned");
+			print_string(PRINT_FP, NULL,
+				     "current-nsid unassigned ", NULL);
 		else
 			print_int(PRINT_ANY, "current-nsid",
 				  "current-nsid %d ", current);
Signature · 1 line
2.21.0
author
#5 lore raw

PATCH 5/5 ipnetns: remove blank lines printed by invarg() messages

In reply to Guillaume Nault

Since invarg() automatically adds a '\n' character, having one in the error message generates an extra blank line.

Signed-off-by Guillaume Nault <[email protected]>
---
 ip/ipnetns.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
ip/ipnetns.c +6 -6
index 4eb4a09a..b22e5d62 100644
@@ -439,10 +439,10 @@ static int netns_list_id(int argc, char **argv)
 			NEXT_ARG();
 
 			if (get_integer(&filter.target_nsid, *argv, 0))
-				invarg("\"target-nsid\" value is invalid\n",
+				invarg("\"target-nsid\" value is invalid",
 				       *argv);
 			else if (filter.target_nsid < 0)
-				invarg("\"target-nsid\" value should be >= 0\n",
+				invarg("\"target-nsid\" value should be >= 0",
 				       argv[1]);
 		} else if (strcmp(*argv, "nsid") == 0) {
 			if (nsid >= 0)

@@ -450,9 +450,9 @@ static int netns_list_id(int argc, char **argv)
 			NEXT_ARG();
 
 			if (get_integer(&nsid, *argv, 0))
-				invarg("\"nsid\" value is invalid\n", *argv);
+				invarg("\"nsid\" value is invalid", *argv);
 			else if (nsid < 0)
-				invarg("\"nsid\" value should be >= 0\n",
+				invarg("\"nsid\" value should be >= 0",
 				       argv[1]);
 		} else
 			usage();

@@ -932,9 +932,9 @@ static int netns_set(int argc, char **argv)
 	if (strcmp(argv[1], "auto") == 0)
 		nsid = -1;
 	else if (get_integer(&nsid, argv[1], 0))
-		invarg("Invalid \"netnsid\" value\n", argv[1]);
+		invarg("Invalid \"netnsid\" value", argv[1]);
 	else if (nsid < 0)
-		invarg("\"netnsid\" value should be >= 0\n", argv[1]);
+		invarg("\"netnsid\" value should be >= 0", argv[1]);
 
 	snprintf(netns_path, sizeof(netns_path), "%s/%s", NETNS_RUN_DIR, name);
 	netns = open(netns_path, O_RDONLY | O_CLOEXEC);
Signature · 1 line
2.21.0
#6 lore raw
In reply to Guillaume Nault
Quoted 19 lines · Guillaume Nault

It's currently hard to review ipnetns. The netns ids are inconsistently treated as signed or unsigned and most helper functions aren't prepared to use negative ids.

Netns id attributes can be negative: NETNSA_NSID_NOT_ASSIGNED == -1. So let's consistently treat nsids as signed and also reject negative values in functions that are supposed to only handle assigned netns ids.

While there, let's drop the extra blank line generated by some command line parsing errors (patch 5/5).

Guillaume Nault (5):
ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed
ipnetns: fix misleading comment about 'ip monitor nsid'
ipnetns: harden helper functions wrt. negative netns ids
ipnetns: don't print unassigned nsid in json export
ipnetns: remove blank lines printed by invarg() messages

Quoted 3 lines
 ip/ipnetns.c | 49 +++++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 20 deletions(-)

looks good. applied to iproute2-next. Thanks for doing the cleanup.