Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids
@ 2019-11-08 17:00 Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 1/5] ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed Guillaume Nault
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Guillaume Nault @ 2019-11-08 17:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Nicolas Dichtel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH iproute2-next 1/5] ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed
  2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
@ 2019-11-08 17:00 ` Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 2/5] ipnetns: fix misleading comment about 'ip monitor nsid' Guillaume Nault
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Guillaume Nault @ 2019-11-08 17:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Nicolas Dichtel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH iproute2-next 2/5] ipnetns: fix misleading comment about 'ip monitor nsid'
  2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 1/5] ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed Guillaume Nault
@ 2019-11-08 17:00 ` Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 3/5] ipnetns: harden helper functions wrt. negative netns ids Guillaume Nault
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Guillaume Nault @ 2019-11-08 17:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Nicolas Dichtel

'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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH iproute2-next 3/5] ipnetns: harden helper functions wrt. negative netns ids
  2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 1/5] ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 2/5] ipnetns: fix misleading comment about 'ip monitor nsid' Guillaume Nault
@ 2019-11-08 17:00 ` Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 4/5] ipnetns: don't print unassigned nsid in json export Guillaume Nault
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Guillaume Nault @ 2019-11-08 17:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Nicolas Dichtel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH iproute2-next 4/5] ipnetns: don't print unassigned nsid in json export
  2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
                   ` (2 preceding siblings ...)
  2019-11-08 17:00 ` [PATCH iproute2-next 3/5] ipnetns: harden helper functions wrt. negative netns ids Guillaume Nault
@ 2019-11-08 17:00 ` Guillaume Nault
  2019-11-08 17:00 ` [PATCH iproute2-next 5/5] ipnetns: remove blank lines printed by invarg() messages Guillaume Nault
  2019-11-09  1:36 ` [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids David Ahern
  5 siblings, 0 replies; 7+ messages in thread
From: Guillaume Nault @ 2019-11-08 17:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Nicolas Dichtel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH iproute2-next 5/5] ipnetns: remove blank lines printed by invarg() messages
  2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
                   ` (3 preceding siblings ...)
  2019-11-08 17:00 ` [PATCH iproute2-next 4/5] ipnetns: don't print unassigned nsid in json export Guillaume Nault
@ 2019-11-08 17:00 ` Guillaume Nault
  2019-11-09  1:36 ` [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids David Ahern
  5 siblings, 0 replies; 7+ messages in thread
From: Guillaume Nault @ 2019-11-08 17:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Nicolas Dichtel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids
  2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
                   ` (4 preceding siblings ...)
  2019-11-08 17:00 ` [PATCH iproute2-next 5/5] ipnetns: remove blank lines printed by invarg() messages Guillaume Nault
@ 2019-11-09  1:36 ` David Ahern
  5 siblings, 0 replies; 7+ messages in thread
From: David Ahern @ 2019-11-09  1:36 UTC (permalink / raw)
  To: Guillaume Nault; +Cc: netdev, Nicolas Dichtel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 17:00 [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids Guillaume Nault
2019-11-08 17:00 ` [PATCH iproute2-next 1/5] ipnetns: treat NETNSA_NSID and NETNSA_CURRENT_NSID as signed Guillaume Nault
2019-11-08 17:00 ` [PATCH iproute2-next 2/5] ipnetns: fix misleading comment about 'ip monitor nsid' Guillaume Nault
2019-11-08 17:00 ` [PATCH iproute2-next 3/5] ipnetns: harden helper functions wrt. negative netns ids Guillaume Nault
2019-11-08 17:00 ` [PATCH iproute2-next 4/5] ipnetns: don't print unassigned nsid in json export Guillaume Nault
2019-11-08 17:00 ` [PATCH iproute2-next 5/5] ipnetns: remove blank lines printed by invarg() messages Guillaume Nault
2019-11-09  1:36 ` [PATCH iproute2-next 0/5] ipnetns: cleanup and harden processing of netns ids David Ahern

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git