[PATCH rfc 0/3] Fix a possible infinite loop when iterating over a discovery tree

[PATCH rfc 0/3] Fix a possible infinite loop when iterating over a discovery tree

[Sagi Grimberg]

Today we can easily get to an infinite loop if the discovery tree itself
contains a loop (via referrals). So if we have a discovery controller
A referring to discovery controller B, and in turn B will also refer
to A we will never finish the discovery loop.

The reason is that we blindly connect and discover to every discovery
log page entry that we find (i.e. referral to another discovery controller).
However, we are not able to detect loops, and we just continue indefinitely.

Fix this by tracking every controller we are connecting to, and then
if we see a discovery log entry that we already saw while iterating the
discovery tree recursion (or should I say graph in this case).

Sagi Grimberg (3):
  nvme: add fabrics discovery controller default port number
  fabrics: move connect_args extraction to a dedicated routine
  fabrics: fix infinite loop is discovery recursion contains a loop

 common.h     |   3 ++
 fabrics.c    | 114 ++++++++++++++++++++++++++++++++++++++++++++-------
 linux/nvme.h |   1 +
 3 files changed, 103 insertions(+), 15 deletions(-)

-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH rfc 1/3] nvme: add fabrics discovery controller default port number

[Sagi Grimberg]

The IANA port number for a discovery controller is 8009
for NVMe/TCP and 4420 for any NVMe/RDMA port number. So make
sure to fill it and pass it down, it will help us as we
track and match connection arguments for filtering purposes.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 common.h     |  3 +++
 fabrics.c    | 17 +++++++++++++++++
 linux/nvme.h |  1 +
 3 files changed, 21 insertions(+)

diff --git a/common.h b/common.h
index aed2a9918b23..1c214a447fd3 100644
--- a/common.h
+++ b/common.h
@@ -9,4 +9,7 @@
 #define min(x, y) ((x) > (y) ? (y) : (x))
 #define max(x, y) ((x) > (y) ? (x) : (y))
 
+#define __stringify_1(x...) #x
+#define __stringify(x...)  __stringify_1(x)
+
 #endif
diff --git a/fabrics.c b/fabrics.c
index 78ed6251f8a6..17d969b17dd5 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -863,6 +863,17 @@ static int build_options(char *argstr, int max_len, bool discover)
 	return 0;
 }
 
+static void discovery_trsvcid(struct config *cfg)
+{
+	if (!strcmp(cfg->transport, "tcp")) {
+		/* Default port for NVMe/TCP discovery controllers */
+		cfg->trsvcid = __stringify(NVME_DISC_IP_PORT);
+	} else if (!strcmp(cfg->transport, "rdma")) {
+		/* Default port for NVMe/RDMA controllers */
+		cfg->trsvcid = __stringify(NVME_RDMA_IP_PORT);
+	}
+}
+
 static bool traddr_is_hostname(struct config *cfg)
 {
 	char addrstr[NVMF_TRADDR_SIZE];
@@ -1320,6 +1331,9 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 				goto out;
 		}
 
+		if (!cfg.trsvcid)
+			discovery_trsvcid(&cfg);
+
 		err = build_options(argstr, BUF_SIZE, true);
 		if (err) {
 			ret = err;
@@ -1392,6 +1406,9 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect)
 				goto out;
 		}
 
+		if (!cfg.trsvcid)
+			discovery_trsvcid(&cfg);
+
 		ret = build_options(argstr, BUF_SIZE, true);
 		if (ret)
 			goto out;
diff --git a/linux/nvme.h b/linux/nvme.h
index a7ab85d03067..de2502929ebb 100644
--- a/linux/nvme.h
+++ b/linux/nvme.h
@@ -70,6 +70,7 @@ static inline uint64_t le64_to_cpu(__le64 x)
 #define NVME_DISC_SUBSYS_NAME	"nqn.2014-08.org.nvmexpress.discovery"
 
 #define NVME_RDMA_IP_PORT	4420
+#define NVME_DISC_IP_PORT	8009
 
 #define NVME_NSID_ALL		0xffffffff
 
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH rfc 2/3] fabrics: move connect_args extraction to a dedicated routine

[Sagi Grimberg]

We will need it to track controllers as we are iterating over
nested discovery log pages recursion.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 fabrics.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 17d969b17dd5..6d6b3b5a3ca7 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -360,6 +360,31 @@ cleanup_devices:
 	return devname;
 }
 
+static struct connect_args *extract_connect_args(char *argstr)
+{
+	struct connect_args *cargs;
+
+	cargs = calloc(1, sizeof(*cargs));
+	if (!cargs)
+		return NULL;
+	cargs->subsysnqn = parse_conn_arg(argstr, ',', conarg_nqn);
+	cargs->transport = parse_conn_arg(argstr, ',', conarg_transport);
+	cargs->traddr = parse_conn_arg(argstr, ',', conarg_traddr);
+	cargs->trsvcid = parse_conn_arg(argstr, ',', conarg_trsvcid);
+	cargs->host_traddr = parse_conn_arg(argstr, ',', conarg_host_traddr);
+	return cargs;
+}
+
+static void free_connect_args(struct connect_args *cargs)
+{
+	free(cargs->subsysnqn);
+	free(cargs->transport);
+	free(cargs->traddr);
+	free(cargs->trsvcid);
+	free(cargs->host_traddr);
+	free(cargs);
+}
+
 static int add_ctrl(const char *argstr)
 {
 	substring_t args[MAX_OPT_ARGS];
@@ -1192,14 +1217,11 @@ static int do_discover(char *argstr, bool connect)
 	int status = 0;
 
 	if (cfg.device) {
-		struct connect_args cargs;
+		struct connect_args *cargs;
 
-		memset(&cargs, 0, sizeof(cargs));
-		cargs.subsysnqn = parse_conn_arg(argstr, ',', conarg_nqn);
-		cargs.transport = parse_conn_arg(argstr, ',', conarg_transport);
-		cargs.traddr = parse_conn_arg(argstr, ',', conarg_traddr);
-		cargs.trsvcid = parse_conn_arg(argstr, ',', conarg_trsvcid);
-		cargs.host_traddr = parse_conn_arg(argstr, ',', conarg_host_traddr);
+		cargs = extract_connect_args(argstr);
+		if (!cargs)
+			return -ENOMEM;
 
 		/*
 		 * if the cfg.device passed in matches the connect args
@@ -1212,14 +1234,10 @@ static int do_discover(char *argstr, bool connect)
 		 *    create a new ctrl.
 		 * endif
 		 */
-		if (!ctrl_matches_connectargs(cfg.device, &cargs))
-			cfg.device = find_ctrl_with_connectargs(&cargs);
-
-		free(cargs.subsysnqn);
-		free(cargs.transport);
-		free(cargs.traddr);
-		free(cargs.trsvcid);
-		free(cargs.host_traddr);
+		if (!ctrl_matches_connectargs(cfg.device, cargs))
+			cfg.device = find_ctrl_with_connectargs(cargs);
+
+		free_connect_args(cargs);
 	}
 
 	if (!cfg.device) {
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH rfc 3/3] fabrics: fix infinite loop is discovery recursion contains a loop

[Sagi Grimberg]

It's possible that different discovery controllers may refer to
each other. In this case, we will get into an infinite loop as
we don't track that we have already connected and seen this.

The kernel doesn't protect us from this for discovery controllers
because it always allows duplicate discovery controllers.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 fabrics.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/fabrics.c b/fabrics.c
index 6d6b3b5a3ca7..73bb37529448 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -93,8 +93,12 @@ struct connect_args {
 	char *traddr;
 	char *trsvcid;
 	char *host_traddr;
+	struct connect_args *next;
+	struct connect_args *tail;
 };
 
+struct connect_args *tracked_ctrls;
+
 #define BUF_SIZE		4096
 #define PATH_NVME_FABRICS	"/dev/nvme-fabrics"
 #define PATH_NVMF_DISC		"/etc/nvme/discovery.conf"
@@ -385,6 +389,21 @@ static void free_connect_args(struct connect_args *cargs)
 	free(cargs);
 }
 
+static void track_ctrl(char *argstr)
+{
+	struct connect_args *cargs;
+
+	cargs = extract_connect_args(argstr);
+	if (!cargs)
+		return;
+
+	if (!tracked_ctrls)
+		tracked_ctrls = cargs;
+	else
+		tracked_ctrls->tail->next = cargs;
+	tracked_ctrls->tail = cargs;
+}
+
 static int add_ctrl(const char *argstr)
 {
 	substring_t args[MAX_OPT_ARGS];
@@ -428,6 +447,7 @@ static int add_ctrl(const char *argstr)
 			if (match_int(args, &token))
 				goto out_fail;
 			ret = token;
+			track_ctrl((char *)argstr);
 			goto out_close;
 		default:
 			/* ignore */
@@ -1148,10 +1168,39 @@ retry:
 	return ret;
 }
 
+static bool cargs_match_found(struct nvmf_disc_rsp_page_entry *entry)
+{
+	struct connect_args cargs = {};
+	struct connect_args *c = tracked_ctrls;
+
+	cargs.traddr = strdup(entry->traddr);
+	cargs.transport = strdup(trtype_str(entry->trtype));
+	cargs.subsysnqn = strdup(entry->subnqn);
+	cargs.trsvcid = strdup(entry->trsvcid);
+	cargs.host_traddr = strdup(cfg.host_traddr ?: "\0");
+
+	/* check if we have a match in the discovery recursion */
+	while (c) {
+		if (!strcmp(cargs.subsysnqn, c->subsysnqn) &&
+		    !strcmp(cargs.transport, c->transport) &&
+		    !strcmp(cargs.traddr, c->traddr) &&
+		    !strcmp(cargs.trsvcid, c->trsvcid) &&
+		    !strcmp(cargs.host_traddr, c->host_traddr))
+			return true;
+		c = c->next;
+	}
+
+	/* check if we have a matching existing controller */
+	return find_ctrl_with_connectargs(&cargs) != NULL;
+}
+
 static bool should_connect(struct nvmf_disc_rsp_page_entry *entry)
 {
 	int len;
 
+	if (cargs_match_found(entry))
+		return false;
+
 	if (!cfg.matching_only || !cfg.traddr)
 		return true;
 
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme