[PATCH] nvmet: don't change model_number on live subsys

[PATCH] nvmet: don't change model_number on live subsys

[Chaitanya Kulkarni]

When subsys is connected to the controller(s) right now user can change
the model_number. Add a check to make sure subsys is not connected
before we allow the change in model number from configfs.

Reported-by: Max Gurtovoy <mgurtovoy@nvidia.com> 
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---

Without this patch :-

# echo  "transport=loop,nqn=x" > /dev/nvme-fabrics
# nvme list | tr -s ' ' ' '
Node SN Model Namespace Usage Format FW Rev 
/dev/nvme1n1 d86964f000f47541 Linux 1 1.07 GB / 1.07 GB 512 B + 0 B 5.11.0-r
#
# echo "NEW MODEL" >  /sys/kernel/config/nvmet/subsystems/x/attr_model
# nvme list | tr -s ' ' ' '
Node SN Model Namespace Usage Format FW Rev 
/dev/nvme1n1 d86964f000f47541 NEW MODEL 1 1.07 GB / 1.07 GB 512 B + 0 B 5.11.0-r
                              ^^^^^^^^^
                               Problem above.
# nvme disconnect -n x
NQN:x disconnected 1 controller(s)

With this patch :-

# echo "transport=loop,nqn=x" > /dev/nvme-fabrics
# nvme list | tr -s ' ' ' ' 
Node SN Model Namespace Usage Format FW Rev 
/dev/nvme1n1 decfcb968deb004d Linux 1 1.07 GB / 1.07 GB 512 B + 0 B 5.11.0-r
# 
# echo "NEW MODEL" >  /sys/kernel/config/nvmet/subsystems/x/attr_model
bash: echo: write error: Invalid argument
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Don't allow new value for the model to be set when connected.

# nvme disconnect -n x
NQN:x disconnected 1 controller(s)
# echo "NEW MODEL" >  /sys/kernel/config/nvmet/subsystems/x/attr_model 
# echo  "transport=loop,nqn=x" > /dev/nvme-fabrics
# nvme list | tr -s ' ' ' ' 
Node SN Model Namespace Usage Format FW Rev 
/dev/nvme1n1 decfcb968deb004d NEW MODEL 1 1.07 GB / 1.07 GB 512 B + 0 B 5.11.0-r
---

 drivers/nvme/target/configfs.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 635a7cb45d0b..e0140a793d14 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1138,6 +1138,21 @@ static bool nvmet_is_ascii(const char c)
 	return c >= 0x20 && c <= 0x7e;
 }
 
+static bool nvmet_subsys_is_connected(struct nvmet_subsys *subsys)
+{
+	struct nvmet_ctrl *ctrl;
+	bool connected = false;
+
+	mutex_lock(&subsys->lock);
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
+		connected = true;
+		break;
+	}
+	mutex_unlock(&subsys->lock);
+
+	return connected;
+}
+
 static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 					     const char *page, size_t count)
 {
@@ -1146,6 +1161,9 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 	char *new_model_number;
 	int pos = 0, len;
 
+	if (nvmet_subsys_is_connected(subsys))
+		return -EINVAL;
+
 	len = strcspn(page, "\n");
 	if (!len)
 		return -EINVAL;
-- 
2.22.1


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

Re: [PATCH] nvmet: don't change model_number on live subsys

[Christoph Hellwig]

> +	mutex_lock(&subsys->lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		connected = true;
> +		break;
> +	}
> +	mutex_unlock(&subsys->lock);

This could just use list_empty.  But we'd also need to hold the lock
over the actual change of the model number to avoid a race window.

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

Re: [PATCH] nvmet: don't change model_number on live subsys

[Chaitanya Kulkarni]

On 2/17/21 23:05, Christoph Hellwig wrote:
>> +	mutex_lock(&subsys->lock);
>> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
>> +		connected = true;
>> +		break;
>> +	}
>> +	mutex_unlock(&subsys->lock);
> This could just use list_empty.  But we'd also need to hold the lock
> over the actual change of the model number to avoid a race window.
>
Since subsys model number actual change occurs later in the
nvmet_subsys_attr_model_store() are you saying something like this we need
instead of having a separate helper ?

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 635a7cb45d0b..c9fd98080b55 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1145,6 +1145,7 @@ static ssize_t
nvmet_subsys_attr_model_store(struct config_item *item,
        struct nvmet_subsys_model *new_model;
        char *new_model_number;
        int pos = 0, len;
+       int ret = 0;
 
        len = strcspn(page, "\n");
        if (!len)
@@ -1168,15 +1169,23 @@ static ssize_t
nvmet_subsys_attr_model_store(struct config_item *item,
 
        down_write(&nvmet_config_sem);
        mutex_lock(&subsys->lock);
+       /* make sure this subsys is not connected */
+       if (!list_empty(&subsys->ctrls)) {
+               ret = -EINVAL;
+               goto out_unlock;
+       }
+
        new_model = rcu_replace_pointer(subsys->model, new_model,
                                        mutex_is_locked(&subsys->lock));
+
+out_unlock:
        mutex_unlock(&subsys->lock);
        up_write(&nvmet_config_sem);
 
        kfree_rcu(new_model, rcuhead);
        kfree(new_model_number);
 
-       return count;
+       return ret ? ret : count;
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 


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