[PATCH v1 0/3] nvme-fabrics: fix un-expected behaviour related to hostnqn and hostid

[PATCH v1 0/3] nvme-fabrics: fix un-expected behaviour related to hostnqn and hostid

[Max Gurtovoy]

Hi Christoph/Sagi/Keith,
I've noticed that we are having some un-expected behaviour in some of
the scenarios of connection establisment related to hostnqn and hostid.
This is specially around overriding hostid for a host.

For example:
-------------
1. Connect to a target with default hostnqn and hostid (after removing
the files from /etc/nvme):
    - # nvme connect -t tcp -n testsubsystem_0 -a 1.1.1.1 -s 4420
2. Check hostnqn and hostid
    - #cat /sys/class/nvme/nvme0/host*
       1791bca2-af3c-4e17-a310-6fbdde3b59cb
       nqn.2014-08.org.nvmexpress:uuid:fc4e5383-ce5c-470d-9889-4adbc2dbe969
3. Connect to a target with given hostid from cmdline:
    - # nvme connect -t tcp -n testsubsystem_0 -a 1.1.1.1 -s 4420 -I 77720af2-ce59-418b-82ff-7c9df0cea35e
        Failed to write to /dev/nvme-fabrics: Operation already in progress <--- failed on duplicate connection check
4. Check hostnqn and hostid of first controller
    - #cat /sys/class/nvme/nvme0/host*
       77720af2-ce59-418b-82ff-7c9df0cea35e <--- hostid was changed !!!!
       nqn.2014-08.org.nvmexpress:uuid:fc4e5383-ce5c-470d-9889-4adbc2dbe969
5. Connect to a target with given hostid from cmdline and allow duplicate connections:
    - # nvme connect -t tcp -n testsubsystem_0 -a 1.1.1.1 -s 4420 -I 77720af2-ce59-418b-82ff-7c9df0cea3bb -D
6. Check hostnqn and hostid of first controller again
    - #cat /sys/class/nvme/nvme0/host*
       77720af2-ce59-418b-82ff-7c9df0cea3bb  <--- hostid was changed again !!!!
       nqn.2014-08.org.nvmexpress:uuid:fc4e5383-ce5c-470d-9889-4adbc2dbe969

This seems like a wrong behaviour to me since in case we try to
reconnect the target will see that a different values after
reconnections and this change the expected behaviour to keep hostid.

According to the spec:

"This feature allows the host to register a Host Identifier with the
controller. The Host Identifier is used by the controller to determine
whether other controllers in the NVM subsystem are associated with the same
host.
"

Also:

"
A host that uses a single Host NQN may employ multiple Host Identifiers
to designate elements of the host
that access an NVM subsystem independently of each other (e.g., physical
or logical partitions of the host)
"

These patchset fixes the current behaviour and allows one to set
different hostIDs for a single hostNQN but not vice versa (as we would
like to enable the target also recognize a logical partition of the
host).

The first 2 patches are some preparations and cleanups and patch 3/3 is
the actual fix.

Max Gurtovoy (3):
  nvme-fabrics: unify common code in admin and io queue connect
  nvme-fabrics: check hostid using uuid_equal
  nvme-fabrics: prevent overriding of existing host

 drivers/nvme/host/fabrics.c | 163 +++++++++++++++++++++++-------------
 drivers/nvme/host/fabrics.h |   3 +-
 2 files changed, 106 insertions(+), 60 deletions(-)

-- 
2.18.1

[PATCH 1/3] nvme-fabrics: unify common code in admin and io queue connect

[Max Gurtovoy]

To simplify code maintenance, it is recommended to avoid duplicating
code.

Tested-by: Noam Gottlieb <ngottlieb@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 75 ++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 0069ebff85df..d11db06d293b 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -349,6 +349,46 @@ static void nvmf_log_connect_error(struct nvme_ctrl *ctrl,
 	}
 }
 
+static struct nvmf_connect_data *nvmf_connect_data_prepare(struct nvme_ctrl *ctrl,
+		u16 cntlid)
+{
+	struct nvmf_connect_data *data;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+
+	uuid_copy(&data->hostid, &ctrl->opts->host->id);
+	data->cntlid = cpu_to_le16(cntlid);
+	strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
+	strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
+
+	return data;
+}
+
+static void nvmf_connect_cmd_prepare(struct nvme_ctrl *ctrl, u16 qid,
+		struct nvme_command *cmd)
+{
+	cmd->connect.opcode = nvme_fabrics_command;
+	cmd->connect.fctype = nvme_fabrics_type_connect;
+	cmd->connect.qid = cpu_to_le16(qid);
+
+	if (qid) {
+		cmd->connect.sqsize = cpu_to_le16(ctrl->sqsize);
+	} else {
+		cmd->connect.sqsize = cpu_to_le16(NVME_AQ_DEPTH - 1);
+
+		/*
+		 * For admin queue, set keep-alive timeout in seconds
+		 * granularity (ms * 1000)
+		 */
+		cmd->connect.kato = cpu_to_le32(ctrl->kato * 1000);
+	}
+
+	if (ctrl->opts->disable_sqflow)
+		cmd->connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
+}
+
 /**
  * nvmf_connect_admin_queue() - NVMe Fabrics Admin Queue "Connect"
  *				API function.
@@ -377,28 +417,12 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 	int ret;
 	u32 result;
 
-	cmd.connect.opcode = nvme_fabrics_command;
-	cmd.connect.fctype = nvme_fabrics_type_connect;
-	cmd.connect.qid = 0;
-	cmd.connect.sqsize = cpu_to_le16(NVME_AQ_DEPTH - 1);
-
-	/*
-	 * Set keep-alive timeout in seconds granularity (ms * 1000)
-	 */
-	cmd.connect.kato = cpu_to_le32(ctrl->kato * 1000);
-
-	if (ctrl->opts->disable_sqflow)
-		cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
+	nvmf_connect_cmd_prepare(ctrl, 0, &cmd);
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = nvmf_connect_data_prepare(ctrl, 0xffff);
 	if (!data)
 		return -ENOMEM;
 
-	uuid_copy(&data->hostid, &ctrl->opts->host->id);
-	data->cntlid = cpu_to_le16(0xffff);
-	strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
-	strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
-
 	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res,
 			data, sizeof(*data), NVME_QID_ANY, 1,
 			BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
@@ -468,23 +492,12 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 	int ret;
 	u32 result;
 
-	cmd.connect.opcode = nvme_fabrics_command;
-	cmd.connect.fctype = nvme_fabrics_type_connect;
-	cmd.connect.qid = cpu_to_le16(qid);
-	cmd.connect.sqsize = cpu_to_le16(ctrl->sqsize);
+	nvmf_connect_cmd_prepare(ctrl, qid, &cmd);
 
-	if (ctrl->opts->disable_sqflow)
-		cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = nvmf_connect_data_prepare(ctrl, ctrl->cntlid);
 	if (!data)
 		return -ENOMEM;
 
-	uuid_copy(&data->hostid, &ctrl->opts->host->id);
-	data->cntlid = cpu_to_le16(ctrl->cntlid);
-	strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
-	strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
-
 	ret = __nvme_submit_sync_cmd(ctrl->connect_q, &cmd, &res,
 			data, sizeof(*data), qid, 1,
 			BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
-- 
2.18.1

Re: [PATCH 1/3] nvme-fabrics: unify common code in admin and io queue connect

[Christoph Hellwig]

On Wed, May 10, 2023 at 09:02:29PM +0300, Max Gurtovoy wrote:
> +static struct nvmf_connect_data *nvmf_connect_data_prepare(struct nvme_ctrl *ctrl,

Overly long line.  Maybe shorten the function name a little?

> +	if (qid) {
> +		cmd->connect.sqsize = cpu_to_le16(ctrl->sqsize);
> +	} else {
> +		cmd->connect.sqsize = cpu_to_le16(NVME_AQ_DEPTH - 1);
> +
> +		/*
> +		 * For admin queue, set keep-alive timeout in seconds
> +		 * granularity (ms * 1000)
> +		 */
> +		cmd->connect.kato = cpu_to_le32(ctrl->kato * 1000);
> +	}

Not sure the "For admin queue, " part here adds any value.  That's
pretty obvious from the code.

[PATCH 2/3] nvme-fabrics: check hostid using uuid_equal

[Max Gurtovoy]

Use a dedicated function to match uuids instead of duplicating it.

Tested-by: Noam Gottlieb <ngottlieb@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index dcac3df8a5f7..0f3763283222 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -181,8 +181,9 @@ nvmf_ctlr_matches_baseopts(struct nvme_ctrl *ctrl,
 	    ctrl->state == NVME_CTRL_DEAD ||
 	    strcmp(opts->subsysnqn, ctrl->opts->subsysnqn) ||
 	    strcmp(opts->host->nqn, ctrl->opts->host->nqn) ||
-	    memcmp(&opts->host->id, &ctrl->opts->host->id, sizeof(uuid_t)))
+	    !uuid_equal(&opts->host->id, &ctrl->opts->host->id)) {
 		return false;
+	}
 
 	return true;
 }
-- 
2.18.1

Re: [PATCH 2/3] nvme-fabrics: check hostid using uuid_equal

[Christoph Hellwig]

On Wed, May 10, 2023 at 09:02:30PM +0300, Max Gurtovoy wrote:
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index dcac3df8a5f7..0f3763283222 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -181,8 +181,9 @@ nvmf_ctlr_matches_baseopts(struct nvme_ctrl *ctrl,
>  	    ctrl->state == NVME_CTRL_DEAD ||
>  	    strcmp(opts->subsysnqn, ctrl->opts->subsysnqn) ||
>  	    strcmp(opts->host->nqn, ctrl->opts->host->nqn) ||
> -	    memcmp(&opts->host->id, &ctrl->opts->host->id, sizeof(uuid_t)))
> +	    !uuid_equal(&opts->host->id, &ctrl->opts->host->id)) {
>  		return false;
> +	}

Please don't add the pointless braces.  The actual change looks fine to
me, though.

Re: [PATCH 2/3] nvme-fabrics: check hostid using uuid_equal

[Max Gurtovoy]

On 11/05/2023 16:13, Christoph Hellwig wrote:
> On Wed, May 10, 2023 at 09:02:30PM +0300, Max Gurtovoy wrote:
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index dcac3df8a5f7..0f3763283222 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -181,8 +181,9 @@ nvmf_ctlr_matches_baseopts(struct nvme_ctrl *ctrl,
>>   	    ctrl->state == NVME_CTRL_DEAD ||
>>   	    strcmp(opts->subsysnqn, ctrl->opts->subsysnqn) ||
>>   	    strcmp(opts->host->nqn, ctrl->opts->host->nqn) ||
>> -	    memcmp(&opts->host->id, &ctrl->opts->host->id, sizeof(uuid_t)))
>> +	    !uuid_equal(&opts->host->id, &ctrl->opts->host->id)) {
>>   		return false;
>> +	}
> 
> Please don't add the pointless braces.  The actual change looks fine to
> me, though.

sorry. probably I forgot to remove after finished debugging..

[PATCH 3/3] nvme-fabrics: prevent overriding of existing host

[Max Gurtovoy]

When first connecting a target using the "default" host parameters,
setting the hostid from the command line during a subsequent connection
establishment would override the "default" hostid parameter. This would
cause an existing connection that is already using the host definitions
to lose its hostid.

To address this issue, the code has been modified to assign a new host
structure if hostid is given as input during the connect command.


Tested-by: Noam Gottlieb <ngottlieb@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 88 +++++++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 28 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index d11db06d293b..7aca456e94ec 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -21,35 +21,67 @@ static DEFINE_MUTEX(nvmf_hosts_mutex);
 
 static struct nvmf_host *nvmf_default_host;
 
-static struct nvmf_host *__nvmf_host_find(const char *hostnqn)
+/**
+ * __nvmf_host_find() - Find a matching to a previously created host
+ * @hostnqn: Host NQN to match
+ * @id: Host ID to match
+ *
+ * We defined a host as seen by the target, thus we must not use different
+ * Host NQNs with the same Host ID. We do allow having same Host NQN with
+ * different Host IDs.
+ *
+ * Return: Returns host pointer on success, NULL in case of no match or
+ *         ERR_PTR(-EINVAL) in case of error match.
+ */
+static struct nvmf_host *__nvmf_host_find(const char *hostnqn, uuid_t *id)
 {
 	struct nvmf_host *host;
 
+	lockdep_assert_held(&nvmf_hosts_mutex);
+
 	list_for_each_entry(host, &nvmf_hosts, list) {
-		if (!strcmp(host->nqn, hostnqn))
-			return host;
+		if (uuid_equal(&host->id, id)) {
+			if (!strcmp(host->nqn, hostnqn))
+				return host;
+			else
+				return ERR_PTR(-EINVAL);
+		}
 	}
 
 	return NULL;
 }
 
-static struct nvmf_host *nvmf_host_add(const char *hostnqn)
+static struct nvmf_host *nvmf_host_alloc(const char *hostnqn, uuid_t *id)
+{
+	struct nvmf_host *host;
+
+	host = kmalloc(sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return NULL;
+
+	kref_init(&host->ref);
+	uuid_copy(&host->id, id);
+	strscpy(host->nqn, hostnqn, NVMF_NQN_SIZE);
+
+	return host;
+}
+
+static struct nvmf_host *nvmf_host_add(const char *hostnqn, uuid_t *id)
 {
 	struct nvmf_host *host;
 
 	mutex_lock(&nvmf_hosts_mutex);
-	host = __nvmf_host_find(hostnqn);
-	if (host) {
+	host = __nvmf_host_find(hostnqn, id);
+	if (IS_ERR(host)) {
+		goto out_unlock;
+	} else if (host) {
 		kref_get(&host->ref);
 		goto out_unlock;
 	}
 
-	host = kmalloc(sizeof(*host), GFP_KERNEL);
+	host = nvmf_host_alloc(hostnqn, id);
 	if (!host)
-		goto out_unlock;
-
-	kref_init(&host->ref);
-	strscpy(host->nqn, hostnqn, NVMF_NQN_SIZE);
+		return ERR_PTR(-ENOMEM);
 
 	list_add_tail(&host->list, &nvmf_hosts);
 out_unlock:
@@ -60,16 +92,17 @@ static struct nvmf_host *nvmf_host_add(const char *hostnqn)
 static struct nvmf_host *nvmf_host_default(void)
 {
 	struct nvmf_host *host;
+	char nqn[NVMF_NQN_SIZE];
+	uuid_t id;
 
-	host = kmalloc(sizeof(*host), GFP_KERNEL);
+	uuid_gen(&id);
+	snprintf(nqn, NVMF_NQN_SIZE,
+		"nqn.2014-08.org.nvmexpress:uuid:%pUb", &id);
+
+	host = nvmf_host_alloc(nqn, &id);
 	if (!host)
 		return NULL;
 
-	kref_init(&host->ref);
-	uuid_gen(&host->id);
-	snprintf(host->nqn, NVMF_NQN_SIZE,
-		"nqn.2014-08.org.nvmexpress:uuid:%pUb", &host->id);
-
 	mutex_lock(&nvmf_hosts_mutex);
 	list_add_tail(&host->list, &nvmf_hosts);
 	mutex_unlock(&nvmf_hosts_mutex);
@@ -634,6 +667,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	size_t nqnlen  = 0;
 	int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO;
 	uuid_t hostid;
+	char hostnqn[NVMF_NQN_SIZE];
 
 	/* Set defaults */
 	opts->queue_size = NVMF_DEF_QUEUE_SIZE;
@@ -650,7 +684,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	if (!options)
 		return -ENOMEM;
 
-	uuid_gen(&hostid);
+	/* use default host if not given by user space */
+	uuid_copy(&hostid, &nvmf_default_host->id);
+	strscpy(hostnqn, nvmf_default_host->nqn, NVMF_NQN_SIZE);
 
 	while ((p = strsep(&o, ",\n")) != NULL) {
 		if (!*p)
@@ -796,12 +832,8 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 				ret = -EINVAL;
 				goto out;
 			}
-			opts->host = nvmf_host_add(p);
+			strscpy(hostnqn, p, NVMF_NQN_SIZE);
 			kfree(p);
-			if (!opts->host) {
-				ret = -ENOMEM;
-				goto out;
-			}
 			break;
 		case NVMF_OPT_RECONNECT_DELAY:
 			if (match_int(args, &token)) {
@@ -958,13 +990,13 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 				opts->fast_io_fail_tmo, ctrl_loss_tmo);
 	}
 
-	if (!opts->host) {
-		kref_get(&nvmf_default_host->ref);
-		opts->host = nvmf_default_host;
+	opts->host = nvmf_host_add(hostnqn, &hostid);
+	if (IS_ERR(opts->host)) {
+		ret = PTR_ERR(opts->host);
+		opts->host = NULL;
+		goto out;
 	}
 
-	uuid_copy(&opts->host->id, &hostid);
-
 out:
 	kfree(options);
 	return ret;
-- 
2.18.1

Re: [PATCH 3/3] nvme-fabrics: prevent overriding of existing host

[Christoph Hellwig]

On Wed, May 10, 2023 at 09:02:31PM +0300, Max Gurtovoy wrote:
> When first connecting a target using the "default" host parameters,
> setting the hostid from the command line during a subsequent connection
> establishment would override the "default" hostid parameter. This would
> cause an existing connection that is already using the host definitions
> to lose its hostid.

We need to reject that second connect - hostids and hostnqns should
have a 1:1 relationship.

Re: [PATCH 3/3] nvme-fabrics: prevent overriding of existing host

[Max Gurtovoy]

On 11/05/2023 16:16, Christoph Hellwig wrote:
> On Wed, May 10, 2023 at 09:02:31PM +0300, Max Gurtovoy wrote:
>> When first connecting a target using the "default" host parameters,
>> setting the hostid from the command line during a subsequent connection
>> establishment would override the "default" hostid parameter. This would
>> cause an existing connection that is already using the host definitions
>> to lose its hostid.
> 
> We need to reject that second connect - hostids and hostnqns should
> have a 1:1 relationship.

I first though this way too but the specification allows an NQN to have 
logical partitions marked as hostIds:

NVMe over fabrics spec 1.1a section 3.3 (p 27):

"A host that uses a single Host NQN may employ multiple Host Identifiers 
to designate elements of the host that access an NVM subsystem 
independently of each other (e.g., physical or logical partitions of the 
host). Alternatively, a host may employ multiple Host NQN values to 
cause each element to be treated as a separate host by an NVM subsystem."

And actually this was probably the idea of the initial implementation in 
the driver.

I've just fixed the bug in the implementation.

Re: [PATCH 3/3] nvme-fabrics: prevent overriding of existing host

[Christoph Hellwig]

On Thu, May 11, 2023 at 04:26:55PM +0300, Max Gurtovoy wrote:
> I first though this way too but the specification allows an NQN to have 
> logical partitions marked as hostIds:
>
> NVMe over fabrics spec 1.1a section 3.3 (p 27):

This is simply broken and we must no support it.

Re: [PATCH 3/3] nvme-fabrics: prevent overriding of existing host

[Max Gurtovoy]

On 11/05/2023 16:49, Christoph Hellwig wrote:
> On Thu, May 11, 2023 at 04:26:55PM +0300, Max Gurtovoy wrote:
>> I first though this way too but the specification allows an NQN to have
>> logical partitions marked as hostIds:
>>
>> NVMe over fabrics spec 1.1a section 3.3 (p 27):
> 
> This is simply broken and we must no support it.

I see.
So I'll re-spin it with 1:1 mapping between hostnqn and hostid. This 
means we won't allow any nqn to have multiple ids and vice versa.