[PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids

[PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids

[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

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

-- 
2.21.0

[PATCH iproute2-next 1/5] ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed

[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 <gnault@redhat.com>
---
 ip/ipnetns.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 20110ef0..0a7912df 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -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();
-- 
2.21.0

[PATCH iproute2-next 2/5] ipnetns: fix misleading comment about 'ip monitor nsid'

[Guillaume Nault]

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

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 ip/ipnetns.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 0a7912df..b02e0a8a 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -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",
-- 
2.21.0

[PATCH iproute2-next 3/5] ipnetns: harden helper functions wrt. negative netns ids

[Guillaume Nault]

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

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 ip/ipnetns.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index b02e0a8a..77531d6c 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -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;
-- 
2.21.0

[PATCH iproute2-next 4/5] ipnetns: don't print unassigned nsid in json export

[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 <gnault@redhat.com>
---
 ip/ipnetns.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 77531d6c..4eb4a09a 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -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);
-- 
2.21.0

[PATCH iproute2-next 5/5] ipnetns: remove blank lines printed by invarg() messages

[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 <gnault@redhat.com>
---
 ip/ipnetns.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 4eb4a09a..b22e5d62 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -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);
-- 
2.21.0

Re: [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids

[David Ahern]

On 11/8/19 10:00 AM, Guillaume Nault wrote:
> 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(-)
> 

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