[PATCH 1/1] nvmet: allow setting model_number once

[PATCH 1/1] nvmet: allow setting model_number once

[Max Gurtovoy]

In case we have already established connection to nvmf target, it
shouldn't be allowed to change the model_number. E.g. if someone will
identify ctrl and get model_number of "my_model" later on will change
the model_numbel via configfs to "my_new_model" this will break the NVM
specification for "Get Log Page – Persistent Event Log" that refers to
Model Number as: "This field contains the same value as reported in the
Model Number field of the Identify Controller data structure, bytes
63:24."

Although it doesn't mentioned explicitly that this field can't be
changed, we can assume it.

So allow setting this field only once: using configfs or in the first
identify ctrl operation.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 33 +++++++++++++------
 drivers/nvme/target/configfs.c  | 57 ++++++++++++++++++---------------
 drivers/nvme/target/core.c      |  2 +-
 drivers/nvme/target/nvmet.h     |  7 +---
 4 files changed, 57 insertions(+), 42 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index dc1ea468b182..6a3341b2091f 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -319,18 +319,25 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
 }
 
-static void nvmet_id_set_model_number(struct nvme_id_ctrl *id,
+static int nvmet_id_set_model_number(struct nvme_id_ctrl *id,
 				      struct nvmet_subsys *subsys)
 {
-	const char *model = NVMET_DEFAULT_CTRL_MODEL;
-	struct nvmet_subsys_model *subsys_model;
+	int ret = 0;
 
-	rcu_read_lock();
-	subsys_model = rcu_dereference(subsys->model);
-	if (subsys_model)
-		model = subsys_model->number;
-	memcpy_and_pad(id->mn, sizeof(id->mn), model, strlen(model), ' ');
-	rcu_read_unlock();
+	mutex_lock(&subsys->lock);
+	if (!subsys->model_number) {
+		subsys->model_number = kstrdup(NVMET_DEFAULT_CTRL_MODEL,
+					       GFP_KERNEL);
+		if (!subsys->model_number) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+	}
+	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
+		       strlen(subsys->model_number), ' ');
+out_unlock:
+	mutex_unlock(&subsys->lock);
+	return ret;
 }
 
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
@@ -339,6 +346,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	struct nvme_id_ctrl *id;
 	u32 cmd_capsule_size;
 	u16 status = 0;
+	int ret;
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -353,7 +361,11 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memset(id->sn, ' ', sizeof(id->sn));
 	bin2hex(id->sn, &ctrl->subsys->serial,
 		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
-	nvmet_id_set_model_number(id, ctrl->subsys);
+	ret = nvmet_id_set_model_number(id, ctrl->subsys);
+	if (ret) {
+		status = NVME_SC_INTERNAL;
+		goto out_free;
+	}
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
@@ -461,6 +473,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
+out_free:
 	kfree(id);
 out:
 	nvmet_req_complete(req, status);
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index c61ffd767062..ad1adcf30ee5 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1120,16 +1120,14 @@ static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
 					    char *page)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
-	struct nvmet_subsys_model *subsys_model;
-	char *model = NVMET_DEFAULT_CTRL_MODEL;
+	char *model = "";
 	int ret;
 
-	rcu_read_lock();
-	subsys_model = rcu_dereference(subsys->model);
-	if (subsys_model)
-		model = subsys_model->number;
+	mutex_lock(&subsys->lock);
+	if (subsys->model_number)
+		model = subsys->model_number;
 	ret = snprintf(page, PAGE_SIZE, "%s\n", model);
-	rcu_read_unlock();
+	mutex_unlock(&subsys->lock);
 
 	return ret;
 }
@@ -1144,40 +1142,49 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 					     const char *page, size_t count)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
-	struct nvmet_subsys_model *new_model;
 	char *new_model_number;
 	int pos = 0, len;
 
+	down_write(&nvmet_config_sem);
+	mutex_lock(&subsys->lock);
+	if (subsys->model_number) {
+		pr_err("Can't set model number. %s is already assigned\n",
+		       subsys->model_number);
+		count = -EINVAL;
+		goto out_unlock;
+	}
+
 	len = strcspn(page, "\n");
-	if (!len)
-		return -EINVAL;
+	if (!len) {
+		count = -EINVAL;
+		goto out_unlock;
+	}
 
 	for (pos = 0; pos < len; pos++) {
-		if (!nvmet_is_ascii(page[pos]))
-			return -EINVAL;
+		if (!nvmet_is_ascii(page[pos])) {
+			count = -EINVAL;
+			goto out_unlock;
+		}
 	}
 
 	new_model_number = kmemdup_nul(page, len, GFP_KERNEL);
-	if (!new_model_number)
-		return -ENOMEM;
+	if (!new_model_number) {
+		count = -ENOMEM;
+		goto out_unlock;
+	}
 
-	new_model = kzalloc(sizeof(*new_model) + len + 1, GFP_KERNEL);
-	if (!new_model) {
+	subsys->model_number = kstrdup(new_model_number, GFP_KERNEL);
+	if (!subsys->model_number) {
+		count = -ENOMEM;
 		kfree(new_model_number);
-		return -ENOMEM;
+		goto out_unlock;
 	}
-	memcpy(new_model->number, new_model_number, len);
 
-	down_write(&nvmet_config_sem);
-	mutex_lock(&subsys->lock);
-	new_model = rcu_replace_pointer(subsys->model, new_model,
-					mutex_is_locked(&subsys->lock));
+	kfree(new_model_number);
+out_unlock:
 	mutex_unlock(&subsys->lock);
 	up_write(&nvmet_config_sem);
 
-	kfree_rcu(new_model, rcuhead);
-	kfree(new_model_number);
-
 	return count;
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 8ce4d59cc9e7..c7af907912f2 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1521,7 +1521,7 @@ static void nvmet_subsys_free(struct kref *ref)
 	nvmet_passthru_subsys_free(subsys);
 
 	kfree(subsys->subsysnqn);
-	kfree_rcu(subsys->model, rcuhead);
+	kfree(subsys->model_number);
 	kfree(subsys);
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 592763732065..aac741bf378c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -208,11 +208,6 @@ struct nvmet_ctrl {
 	bool			pi_support;
 };
 
-struct nvmet_subsys_model {
-	struct rcu_head		rcuhead;
-	char			number[];
-};
-
 struct nvmet_subsys {
 	enum nvme_subsys_type	type;
 
@@ -242,7 +237,7 @@ struct nvmet_subsys {
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
 
-	struct nvmet_subsys_model	__rcu *model;
+	char			*model_number;
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
-- 
2.25.4


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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Chaitanya Kulkarni]

Max,

On 2/17/21 10:12, Max Gurtovoy wrote:
> In case we have already established connection to nvmf target, it
> shouldn't be allowed to change the model_number. E.g. if someone will
> identify ctrl and get model_number of "my_model" later on will change
> the model_numbel via configfs to "my_new_model" this will break the NVM
> specification for "Get Log Page – Persistent Event Log" that refers to
> Model Number as: "This field contains the same value as reported in the
> Model Number field of the Identify Controller data structure, bytes
> 63:24."
We don't support persistent event log page since there is no use
case for target to have human readable format with timestamps,
instead we do maintain the error log page which has information
about the errors if user wants.

I'll let Christoph/Sagi decide if we really need to implement
Persistent Event (PE) log and so does this behavior.

Regarding preventing from changing the model_number when subsys is
connected to the host, that needs a fix and I sent a patch for that.

We can ignore that patch if we decide to go with this patch with
PR log page.

> Although it doesn't mentioned explicitly that this field can't be
> changed, we can assume it.
>
> So allow setting this field only once: using configfs or in the first
> identify ctrl operation.


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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Max Gurtovoy]

On 2/18/2021 5:17 AM, Chaitanya Kulkarni wrote:
> Max,
>
> On 2/17/21 10:12, Max Gurtovoy wrote:
>> In case we have already established connection to nvmf target, it
>> shouldn't be allowed to change the model_number. E.g. if someone will
>> identify ctrl and get model_number of "my_model" later on will change
>> the model_numbel via configfs to "my_new_model" this will break the NVM
>> specification for "Get Log Page – Persistent Event Log" that refers to
>> Model Number as: "This field contains the same value as reported in the
>> Model Number field of the Identify Controller data structure, bytes
>> 63:24."
> We don't support persistent event log page since there is no use
> case for target to have human readable format with timestamps,
> instead we do maintain the error log page which has information
> about the errors if user wants.
>
> I'll let Christoph/Sagi decide if we really need to implement
> Persistent Event (PE) log and so does this behavior.
>
> Regarding preventing from changing the model_number when subsys is
> connected to the host, that needs a fix and I sent a patch for that.
>
> We can ignore that patch if we decide to go with this patch with
> PR log page.

what is wrong with this patch ?

why you need the rcu logic in the driver and why do you want to preserve 
a case that initiator A connect to subsystem "my_sub" and see MN == 
Linux and after disconnect and re-connect it might see MN = New_Linux ?


>
>> Although it doesn't mentioned explicitly that this field can't be
>> changed, we can assume it.
>>
>> So allow setting this field only once: using configfs or in the first
>> identify ctrl operation.
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Christoph Hellwig]

So looking at this and Chaitanya patch I think this version simplifies
things quite nicely, and it also happens to get rid of the RCU annotation
sparse warnings.

Let me know what you think of the incremental cleanup and micro-opimization
below, though:


diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 3da285e22e9209..2ca13dd7e7b88a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -313,34 +313,40 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
 }
 
-static int nvmet_id_set_model_number(struct nvme_id_ctrl *id,
-				      struct nvmet_subsys *subsys)
+static u16 nvmet_set_model_number(struct nvme_id_ctrl *id,
+		struct nvmet_subsys *subsys)
 {
-	int ret = 0;
+	u16 status = 0;
 
 	mutex_lock(&subsys->lock);
 	if (!subsys->model_number) {
-		subsys->model_number = kstrdup(NVMET_DEFAULT_CTRL_MODEL,
-					       GFP_KERNEL);
-		if (!subsys->model_number) {
-			ret = -ENOMEM;
-			goto out_unlock;
-		}
+		subsys->model_number =
+			kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
+		if (!subsys->model_number)
+			status = NVME_SC_INTERNAL;
 	}
-	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
-		       strlen(subsys->model_number), ' ');
-out_unlock:
 	mutex_unlock(&subsys->lock);
-	return ret;
+
+	return status;
 }
 
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvmet_subsys *subsys = ctrl->subsys;
 	struct nvme_id_ctrl *id;
 	u32 cmd_capsule_size;
 	u16 status = 0;
-	int ret;
+
+	/*
+	 * If there is no model number yet, so it now.  It will then remain
+	 * stable for the life time of the subsystem.
+	 */
+	if (!subsys->model_number) {
+		status = nvmet_set_model_number(id, subsys);
+		if (status)
+			goto out;
+	}
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -355,11 +361,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memset(id->sn, ' ', sizeof(id->sn));
 	bin2hex(id->sn, &ctrl->subsys->serial,
 		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
-	ret = nvmet_id_set_model_number(id, ctrl->subsys);
-	if (ret) {
-		status = NVME_SC_INTERNAL;
-		goto out_free;
-	}
+	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
+		       strlen(subsys->model_number), ' ');
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
@@ -467,7 +470,6 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
-out_free:
 	kfree(id);
 out:
 	nvmet_req_complete(req, status);
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 71a1fe2f6330a9..e5dbd1923b7b75 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1118,13 +1118,11 @@ static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
 					    char *page)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
-	char *model = "";
 	int ret;
 
 	mutex_lock(&subsys->lock);
-	if (subsys->model_number)
-		model = subsys->model_number;
-	ret = snprintf(page, PAGE_SIZE, "%s\n", model);
+	ret = snprintf(page, PAGE_SIZE, "%s\n", subsys->model_number ?
+			subsys->model_number : NVMET_DEFAULT_CTRL_MODEL);
 	mutex_unlock(&subsys->lock);
 
 	return ret;
@@ -1136,54 +1134,45 @@ static bool nvmet_is_ascii(const char c)
 	return c >= 0x20 && c <= 0x7e;
 }
 
-static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
-					     const char *page, size_t count)
+static ssize_t nvmet_subsys_attr_model_store_locked(struct nvmet_subsys *subsys,
+		const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item);
-	char *new_model_number;
 	int pos = 0, len;
 
-	down_write(&nvmet_config_sem);
-	mutex_lock(&subsys->lock);
 	if (subsys->model_number) {
 		pr_err("Can't set model number. %s is already assigned\n",
 		       subsys->model_number);
-		count = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	len = strcspn(page, "\n");
-	if (!len) {
-		count = -EINVAL;
-		goto out_unlock;
-	}
+	if (!len)
+		return -EINVAL;
 
 	for (pos = 0; pos < len; pos++) {
-		if (!nvmet_is_ascii(page[pos])) {
-			count = -EINVAL;
-			goto out_unlock;
-		}
+		if (!nvmet_is_ascii(page[pos]))
+			return -EINVAL;
 	}
 
-	new_model_number = kmemdup_nul(page, len, GFP_KERNEL);
-	if (!new_model_number) {
-		count = -ENOMEM;
-		goto out_unlock;
-	}
+	subsys->model_number = kmemdup_nul(page, len, GFP_KERNEL);
+	if (!subsys->model_number)
+		return -ENOMEM;
+	return count;
+}
 
-	subsys->model_number = kstrdup(new_model_number, GFP_KERNEL);
-	if (!subsys->model_number) {
-		count = -ENOMEM;
-		kfree(new_model_number);
-		goto out_unlock;
-	}
+static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	ssize_t ret;
 
-	kfree(new_model_number);
-out_unlock:
+	down_write(&nvmet_config_sem);
+	mutex_lock(&subsys->lock);
+	ret = nvmet_subsys_attr_model_store_locked(subsys, page, count);
 	mutex_unlock(&subsys->lock);
 	up_write(&nvmet_config_sem);
 
-	return count;
+	return ret;
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 

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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Max Gurtovoy]

On 2/24/2021 11:43 AM, Christoph Hellwig wrote:
> So looking at this and Chaitanya patch I think this version simplifies
> things quite nicely, and it also happens to get rid of the RCU annotation
> sparse warnings.
>
> Let me know what you think of the incremental cleanup and micro-opimization
> below, though:
>
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 3da285e22e9209..2ca13dd7e7b88a 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -313,34 +313,40 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
>   	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
>   }
>   
> -static int nvmet_id_set_model_number(struct nvme_id_ctrl *id,
> -				      struct nvmet_subsys *subsys)
> +static u16 nvmet_set_model_number(struct nvme_id_ctrl *id,
> +		struct nvmet_subsys *subsys)
>   {
> -	int ret = 0;
> +	u16 status = 0;
>   
>   	mutex_lock(&subsys->lock);
>   	if (!subsys->model_number) {
> -		subsys->model_number = kstrdup(NVMET_DEFAULT_CTRL_MODEL,
> -					       GFP_KERNEL);
> -		if (!subsys->model_number) {
> -			ret = -ENOMEM;
> -			goto out_unlock;
> -		}
> +		subsys->model_number =
> +			kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
> +		if (!subsys->model_number)
> +			status = NVME_SC_INTERNAL;
>   	}
> -	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
> -		       strlen(subsys->model_number), ' ');
> -out_unlock:
>   	mutex_unlock(&subsys->lock);
> -	return ret;
> +
> +	return status;
>   }
>   
>   static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   {
>   	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +	struct nvmet_subsys *subsys = ctrl->subsys;
>   	struct nvme_id_ctrl *id;
>   	u32 cmd_capsule_size;
>   	u16 status = 0;
> -	int ret;
> +
> +	/*
> +	 * If there is no model number yet, so it now.  It will then remain
> +	 * stable for the life time of the subsystem.
> +	 */
> +	if (!subsys->model_number) {
> +		status = nvmet_set_model_number(id, subsys);

you don't need the id argument here.

Otherwise looks good and it can be squashed to my patch.



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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Christoph Hellwig]

On Wed, Feb 24, 2021 at 11:59:22AM +0200, Max Gurtovoy wrote:
> you don't need the id argument here.
>
> Otherwise looks good and it can be squashed to my patch.

Can you resend it with the updates?

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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Max Gurtovoy]

On 2/24/2021 12:00 PM, Christoph Hellwig wrote:
> On Wed, Feb 24, 2021 at 11:59:22AM +0200, Max Gurtovoy wrote:
>> you don't need the id argument here.
>>
>> Otherwise looks good and it can be squashed to my patch.
> Can you resend it with the updates?

Yes.

What is the target branch ?


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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Christoph Hellwig]

On Wed, Feb 24, 2021 at 12:02:37PM +0200, Max Gurtovoy wrote:
> What is the target branch ?

nvme-5.12 as just pushed out.

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

Re: [PATCH 1/1] nvmet: allow setting model_number once

[Max Gurtovoy]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]


On 2/24/2021 12:04 PM, Christoph Hellwig wrote:
> On Wed, Feb 24, 2021 at 12:02:37PM +0200, Max Gurtovoy wrote:
>> What is the target branch ?
> nvme-5.12 as just pushed out.

See attached.


[-- Attachment #2: 0001-nvmet-allow-setting-model_number-once.patch --]
[-- Type: text/plain, Size: 7220 bytes --]

From bcdf33f3995417aff47e55d4d06c7fc61a68cf9f Mon Sep 17 00:00:00 2001
From: Max Gurtovoy <mgurtovoy@nvidia.com>
Date: Wed, 17 Feb 2021 17:19:40 +0000
Subject: [PATCH 1/1] nvmet: allow setting model_number once
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In case we have already established connection to nvmf target, it
shouldn't be allowed to change the model_number. E.g. if someone will
identify ctrl and get model_number of "my_model" later on will change
the model_numbel via configfs to "my_new_model" this will break the NVM
specification for "Get Log Page – Persistent Event Log" that refers to
Model Number as: "This field contains the same value as reported in the
Model Number field of the Identify Controller data structure, bytes
63:24."

Although it doesn't mentioned explicitly that this field can't be
changed, we can assume it.

So allow setting this field only once: using configfs or in the first
identify ctrl operation.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/target/admin-cmd.c | 36 ++++++++++++++++--------
 drivers/nvme/target/configfs.c  | 50 +++++++++++++++------------------
 drivers/nvme/target/core.c      |  2 +-
 drivers/nvme/target/nvmet.h     |  7 +----
 4 files changed, 50 insertions(+), 45 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index bc6a774f2124..fe6b8aa90b53 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -313,27 +313,40 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
 }
 
-static void nvmet_id_set_model_number(struct nvme_id_ctrl *id,
-				      struct nvmet_subsys *subsys)
+static u16 nvmet_set_model_number(struct nvmet_subsys *subsys)
 {
-	const char *model = NVMET_DEFAULT_CTRL_MODEL;
-	struct nvmet_subsys_model *subsys_model;
+	u16 status = 0;
+
+	mutex_lock(&subsys->lock);
+	if (!subsys->model_number) {
+		subsys->model_number =
+			kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
+		if (!subsys->model_number)
+			status = NVME_SC_INTERNAL;
+	}
+	mutex_unlock(&subsys->lock);
 
-	rcu_read_lock();
-	subsys_model = rcu_dereference(subsys->model);
-	if (subsys_model)
-		model = subsys_model->number;
-	memcpy_and_pad(id->mn, sizeof(id->mn), model, strlen(model), ' ');
-	rcu_read_unlock();
+	return status;
 }
 
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvmet_subsys *subsys = ctrl->subsys;
 	struct nvme_id_ctrl *id;
 	u32 cmd_capsule_size;
 	u16 status = 0;
 
+	/*
+	 * If there is no model number yet, set it now.  It will then remain
+	 * stable for the life time of the subsystem.
+	 */
+	if (!subsys->model_number) {
+		status = nvmet_set_model_number(subsys);
+		if (status)
+			goto out;
+	}
+
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
 		status = NVME_SC_INTERNAL;
@@ -347,7 +360,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memset(id->sn, ' ', sizeof(id->sn));
 	bin2hex(id->sn, &ctrl->subsys->serial,
 		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
-	nvmet_id_set_model_number(id, ctrl->subsys);
+	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
+		       strlen(subsys->model_number), ' ');
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 635a7cb45d0b..e5dbd1923b7b 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1118,16 +1118,12 @@ static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
 					    char *page)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
-	struct nvmet_subsys_model *subsys_model;
-	char *model = NVMET_DEFAULT_CTRL_MODEL;
 	int ret;
 
-	rcu_read_lock();
-	subsys_model = rcu_dereference(subsys->model);
-	if (subsys_model)
-		model = subsys_model->number;
-	ret = snprintf(page, PAGE_SIZE, "%s\n", model);
-	rcu_read_unlock();
+	mutex_lock(&subsys->lock);
+	ret = snprintf(page, PAGE_SIZE, "%s\n", subsys->model_number ?
+			subsys->model_number : NVMET_DEFAULT_CTRL_MODEL);
+	mutex_unlock(&subsys->lock);
 
 	return ret;
 }
@@ -1138,14 +1134,17 @@ static bool nvmet_is_ascii(const char c)
 	return c >= 0x20 && c <= 0x7e;
 }
 
-static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
-					     const char *page, size_t count)
+static ssize_t nvmet_subsys_attr_model_store_locked(struct nvmet_subsys *subsys,
+		const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item);
-	struct nvmet_subsys_model *new_model;
-	char *new_model_number;
 	int pos = 0, len;
 
+	if (subsys->model_number) {
+		pr_err("Can't set model number. %s is already assigned\n",
+		       subsys->model_number);
+		return -EINVAL;
+	}
+
 	len = strcspn(page, "\n");
 	if (!len)
 		return -EINVAL;
@@ -1155,28 +1154,25 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 			return -EINVAL;
 	}
 
-	new_model_number = kmemdup_nul(page, len, GFP_KERNEL);
-	if (!new_model_number)
+	subsys->model_number = kmemdup_nul(page, len, GFP_KERNEL);
+	if (!subsys->model_number)
 		return -ENOMEM;
+	return count;
+}
 
-	new_model = kzalloc(sizeof(*new_model) + len + 1, GFP_KERNEL);
-	if (!new_model) {
-		kfree(new_model_number);
-		return -ENOMEM;
-	}
-	memcpy(new_model->number, new_model_number, len);
+static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	ssize_t ret;
 
 	down_write(&nvmet_config_sem);
 	mutex_lock(&subsys->lock);
-	new_model = rcu_replace_pointer(subsys->model, new_model,
-					mutex_is_locked(&subsys->lock));
+	ret = nvmet_subsys_attr_model_store_locked(subsys, page, count);
 	mutex_unlock(&subsys->lock);
 	up_write(&nvmet_config_sem);
 
-	kfree_rcu(new_model, rcuhead);
-	kfree(new_model_number);
-
-	return count;
+	return ret;
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 67bbf0e3b507..be6fcdaf51a7 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1532,7 +1532,7 @@ static void nvmet_subsys_free(struct kref *ref)
 	nvmet_passthru_subsys_free(subsys);
 
 	kfree(subsys->subsysnqn);
-	kfree_rcu(subsys->model, rcuhead);
+	kfree(subsys->model_number);
 	kfree(subsys);
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index cdfa537b1c0a..4b84edb49f22 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -208,11 +208,6 @@ struct nvmet_ctrl {
 	bool			pi_support;
 };
 
-struct nvmet_subsys_model {
-	struct rcu_head		rcuhead;
-	char			number[];
-};
-
 struct nvmet_subsys {
 	enum nvme_subsys_type	type;
 
@@ -242,7 +237,7 @@ struct nvmet_subsys {
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
 
-	struct nvmet_subsys_model	__rcu *model;
+	char			*model_number;
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
-- 
2.18.2


[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

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