[PATCH 0/2] nvmet: improve 'read-only' handling

[PATCH 0/2] nvmet: improve 'read-only' handling

[Hannes Reinecke]

Hi all,

there was a request (Hi Lennart) to implement 'readonly' handling
for nvme target. Turns out to be relatively easy as we already have
a 'readonly' flag for the namespace.
So all what's left is to make this flag settable via a new configfs
attribute. And while at it we can as well check if the underlying
device/file is read-only, and implement a 'persistent_ro' flag to
mark them as persistently read-only.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  nvmet: make 'readonly' setting configurable
  nvmet: implement persistent read-only namespace feature

 drivers/nvme/target/admin-cmd.c   | 15 +++++++++++----
 drivers/nvme/target/configfs.c    | 32 +++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c        |  2 ++
 drivers/nvme/target/io-cmd-bdev.c | 11 +++++++++++
 drivers/nvme/target/io-cmd-file.c | 21 ++++++++++++++++----
 drivers/nvme/target/nvmet.h       |  1 +
 6 files changed, 74 insertions(+), 8 deletions(-)

-- 
2.35.3

[PATCH 1/2] nvmet: make 'readonly' setting configurable

[Hannes Reinecke]

The namespace already has a 'readonly' setting, which is controlled
by the 'write protect' feature. This patch introduces a namespace
configfs attribute 'readonly' to make it settable by the admin.

Suggested-by: Lennart Poettering <lennart@poettering.net>
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/configfs.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 00fd6ab046dd..cb03b448ae6d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -820,6 +820,32 @@ static ssize_t nvmet_ns_resv_enable_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_ns_, resv_enable);
 
+static ssize_t nvmet_ns_readonly_show(struct config_item *item, char *page)
+{
+	return sysfs_emit(page, "%d\n", to_nvmet_ns(item)->readonly);
+}
+
+static ssize_t nvmet_ns_readonly_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	bool val;
+
+	if (kstrtobool(page, &val))
+		return -EINVAL;
+
+	mutex_lock(&ns->subsys->lock);
+	if (ns->enabled) {
+		pr_err("the ns:%d is already enabled.\n", ns->nsid);
+		mutex_unlock(&ns->subsys->lock);
+		return -EINVAL;
+	}
+	ns->readonly = val;
+	mutex_unlock(&ns->subsys->lock);
+	return count;
+}
+CONFIGFS_ATTR(nvmet_ns_, readonly);
+
 static struct configfs_attribute *nvmet_ns_attrs[] = {
 	&nvmet_ns_attr_device_path,
 	&nvmet_ns_attr_device_nguid,
@@ -830,6 +856,7 @@ static struct configfs_attribute *nvmet_ns_attrs[] = {
 	&nvmet_ns_attr_buffered_io,
 	&nvmet_ns_attr_revalidate_size,
 	&nvmet_ns_attr_resv_enable,
+	&nvmet_ns_attr_readonly,
 #ifdef CONFIG_PCI_P2PDMA
 	&nvmet_ns_attr_p2pmem,
 #endif
-- 
2.35.3

Re: [PATCH 1/2] nvmet: make 'readonly' setting configurable

[Keith Busch]

On Thu, Apr 03, 2025 at 04:47:46PM +0200, Hannes Reinecke wrote:
> +static ssize_t nvmet_ns_readonly_store(struct config_item *item,
> +		const char *page, size_t count)
> +{
> +	struct nvmet_ns *ns = to_nvmet_ns(item);
> +	bool val;
> +
> +	if (kstrtobool(page, &val))
> +		return -EINVAL;
> +
> +	mutex_lock(&ns->subsys->lock);
> +	if (ns->enabled) {
> +		pr_err("the ns:%d is already enabled.\n", ns->nsid);
> +		mutex_unlock(&ns->subsys->lock);
> +		return -EINVAL;
> +	}
> +	ns->readonly = val;
> +	mutex_unlock(&ns->subsys->lock);

Not sure how people feel about the "cleanup" constructs. If we're okay
using them in this driver, this is a simple use case for the guard to
manage the mutex:

	guard(mutex)(&ns->subsys->lock);

And then you don't need to worry about unlocking.

> +	return count;
> +}

Re: [PATCH 1/2] nvmet: make 'readonly' setting configurable

[Hannes Reinecke]

On 4/3/25 17:44, Keith Busch wrote:
> On Thu, Apr 03, 2025 at 04:47:46PM +0200, Hannes Reinecke wrote:
>> +static ssize_t nvmet_ns_readonly_store(struct config_item *item,
>> +		const char *page, size_t count)
>> +{
>> +	struct nvmet_ns *ns = to_nvmet_ns(item);
>> +	bool val;
>> +
>> +	if (kstrtobool(page, &val))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&ns->subsys->lock);
>> +	if (ns->enabled) {
>> +		pr_err("the ns:%d is already enabled.\n", ns->nsid);
>> +		mutex_unlock(&ns->subsys->lock);
>> +		return -EINVAL;
>> +	}
>> +	ns->readonly = val;
>> +	mutex_unlock(&ns->subsys->lock);
> 
> Not sure how people feel about the "cleanup" constructs. If we're okay
> using them in this driver, this is a simple use case for the guard to
> manage the mutex:
> 
> 	guard(mutex)(&ns->subsys->lock);
> 
> And then you don't need to worry about unlocking.
> 
Sure, can do that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 1/2] nvmet: make 'readonly' setting configurable

[Christoph Hellwig]

On Thu, Apr 03, 2025 at 09:44:55AM -0600, Keith Busch wrote:
> > +	mutex_lock(&ns->subsys->lock);
> > +	if (ns->enabled) {
> > +		pr_err("the ns:%d is already enabled.\n", ns->nsid);
> > +		mutex_unlock(&ns->subsys->lock);
> > +		return -EINVAL;
> > +	}
> > +	ns->readonly = val;
> > +	mutex_unlock(&ns->subsys->lock);
> 
> Not sure how people feel about the "cleanup" constructs. If we're okay
> using them in this driver, this is a simple use case for the guard to
> manage the mutex:
> 
> 	guard(mutex)(&ns->subsys->lock);
> 
> And then you don't need to worry about unlocking.

I really hate it with passion as the syntax is so butt ugly and has
no explicit unlock point.  Why couldn't folks at least do the simple
python thing:

	with (mutex(&ns->subsys->lock)) {
		if (ns->enabled) {
			pr_err("the ns:%d is already enabled.\n",
					ns->nsid);
			mutex_unlock(&ns->subsys->lock);
			return -EINVAL;
		}
		ns->readonly = val;
	}

> 
> > +	return count;
> > +}
---end quoted text---

Re: [PATCH 1/2] nvmet: make 'readonly' setting configurable

[Keith Busch]

On Fri, Apr 04, 2025 at 08:21:26AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 03, 2025 at 09:44:55AM -0600, Keith Busch wrote:
> > > +	mutex_lock(&ns->subsys->lock);
> > > +	if (ns->enabled) {
> > > +		pr_err("the ns:%d is already enabled.\n", ns->nsid);
> > > +		mutex_unlock(&ns->subsys->lock);
> > > +		return -EINVAL;
> > > +	}
> > > +	ns->readonly = val;
> > > +	mutex_unlock(&ns->subsys->lock);
> > 
> > Not sure how people feel about the "cleanup" constructs. If we're okay
> > using them in this driver, this is a simple use case for the guard to
> > manage the mutex:
> > 
> > 	guard(mutex)(&ns->subsys->lock);
> > 
> > And then you don't need to worry about unlocking.
> 
> I really hate it with passion as the syntax is so butt ugly and has
> no explicit unlock point.

Okay, that's why I asked. Some subsystems have embraced it, but not
others. Since we haven't considered it for nvme yet, this looked like an
opportunity to feel that out, and it's totally fine with me if we avoid
the guard model.

Re: [PATCH 1/2] nvmet: make 'readonly' setting configurable

[Christoph Hellwig]

On Thu, Apr 03, 2025 at 04:47:46PM +0200, Hannes Reinecke wrote:
> The namespace already has a 'readonly' setting, which is controlled
> by the 'write protect' feature.
> This patch introduces a namespace
> configfs attribute 'readonly' to make it settable by the admin.

Please describe the spec feature this corresponds to, and why setting
the NSATTR CWP flag without a formal write protect is fine.  From
looking at the language in the spec it probably is, but that really
needs to go into a commit log.

[PATCH 2/2] nvmet: implement persistent read-only namespace feature

[Hannes Reinecke]

Per default we try to open the 'device_path' read-write, which might
fail if the device is read-only. So retry with opening read-only, and
implement a new 'persistent_ro' flag if we did so.
And map that flag onto the 'Permanently Write Protect' setting in the
write protection feature.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/admin-cmd.c   | 15 +++++++++++----
 drivers/nvme/target/configfs.c    |  5 +++++
 drivers/nvme/target/core.c        |  2 ++
 drivers/nvme/target/io-cmd-bdev.c | 11 +++++++++++
 drivers/nvme/target/io-cmd-file.c | 21 +++++++++++++++++----
 drivers/nvme/target/nvmet.h       |  1 +
 6 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 43f819b0f89b..2fea7ab05408 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -1098,7 +1098,7 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
 	id->nstat = NVME_NSTAT_NRDY;
 	id->anagrpid = cpu_to_le32(req->ns->anagrpid);
 	id->nmic = NVME_NS_NMIC_SHARED;
-	if (req->ns->readonly)
+	if (req->ns->readonly || req->ns->persistent_ro)
 		id->nsattr |= NVME_NS_ATTR_RO;
 	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
 		id->nsfeat |= NVME_NS_ROTATIONAL;
@@ -1225,8 +1225,13 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 			req->ns->readonly = false;
 		break;
 	case NVME_NS_NO_WRITE_PROTECT:
-		req->ns->readonly = false;
-		status = 0;
+		if (req->ns->persistent_ro)
+			status = NVME_SC_FEATURE_NOT_CHANGEABLE | \
+				NVME_STATUS_DNR;
+		else {
+			req->ns->readonly = false;
+			status = 0;
+		}
 		break;
 	default:
 		break;
@@ -1418,7 +1423,9 @@ static u16 nvmet_get_feat_write_protect(struct nvmet_req *req)
 		return result;
 
 	mutex_lock(&subsys->lock);
-	if (req->ns->readonly == true)
+	if (req->ns->persistent_ro == true)
+		result = NVME_NS_WRITE_PROTECT_PERMANENT;
+	else if (req->ns->readonly == true)
 		result = NVME_NS_WRITE_PROTECT;
 	else
 		result = NVME_NS_NO_WRITE_PROTECT;
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index cb03b448ae6d..1efe1ed2ae50 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -840,6 +840,11 @@ static ssize_t nvmet_ns_readonly_store(struct config_item *item,
 		mutex_unlock(&ns->subsys->lock);
 		return -EINVAL;
 	}
+	if (ns->persistent_ro && val == false) {
+		pr_err("the ns:%d is permanently read-only\n", ns->nsid);
+		mutex_unlock(&ns->subsys->lock);
+		return -EACCES;
+	}
 	ns->readonly = val;
 	mutex_unlock(&ns->subsys->lock);
 	return count;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 2e741696f371..42605c67cbeb 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -725,6 +725,8 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 
 	uuid_gen(&ns->uuid);
 	ns->buffered_io = false;
+	ns->readonly = false;
+	ns->persistent_ro = false;
 	ns->csi = NVME_CSI_NVM;
 
 	return ns;
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 83be0657e6df..f28dfac2b9fe 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -91,6 +91,17 @@ int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
 				BLK_OPEN_READ | BLK_OPEN_WRITE, NULL, NULL);
 	if (IS_ERR(ns->bdev_file)) {
 		ret = PTR_ERR(ns->bdev_file);
+		if (ret == -EACCES) {
+			ns->bdev_file = bdev_file_open_by_path(ns->device_path,
+						BLK_OPEN_READ, NULL, NULL);
+			if (IS_ERR(ns->bdev_file)) {
+				ret = PTR_ERR(ns->bdev_file);
+			} else {
+				ns->readonly = true;
+				ns->persistent_ro = true;
+				ret = 0;
+			}
+		}
 		if (ret != -ENOTBLK) {
 			pr_err("failed to open block device %s: (%d)\n",
 					ns->device_path, ret);
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 2d068439b129..770dec653bab 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -41,10 +41,23 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	ns->file = filp_open(ns->device_path, flags, 0);
 	if (IS_ERR(ns->file)) {
 		ret = PTR_ERR(ns->file);
-		pr_err("failed to open file %s: (%d)\n",
-			ns->device_path, ret);
-		ns->file = NULL;
-		return ret;
+		if (ret == -EACCES) {
+			flags = O_RDONLY | O_LARGEFILE;
+			ns->file = filp_open(ns->device_path, flags, 0);
+			if (IS_ERR(ns->file)) {
+				ret = PTR_ERR(ns->file);
+			} else {
+				ns->readonly = true;
+				ns->persistent_ro = true;
+				ret = 0;
+			}
+		}
+		if (ret) {
+			pr_err("failed to open file %s: (%d)\n",
+			       ns->device_path, ret);
+			ns->file = NULL;
+			return ret;
+		}
 	}
 
 	nvmet_file_ns_revalidate(ns);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 29ae0657ba8f..166add3c119f 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -103,6 +103,7 @@ struct nvmet_ns {
 	struct block_device	*bdev;
 	struct file		*file;
 	bool			readonly;
+	bool			persistent_ro;
 	u32			nsid;
 	u32			blksize_shift;
 	loff_t			size;
-- 
2.35.3

Re: [PATCH 2/2] nvmet: implement persistent read-only namespace feature

[Christoph Hellwig]

On Thu, Apr 03, 2025 at 04:47:47PM +0200, Hannes Reinecke wrote:
> Per default we try to open the 'device_path' read-write, which might
> fail if the device is read-only. So retry with opening read-only, and
> implement a new 'persistent_ro' flag if we did so.
> And map that flag onto the 'Permanently Write Protect' setting in the
> write protection feature.

This sounds like a really horrible interface, but maybe there is logic
for it so explain why we don't require the explicit read-only attribute
to open read only.

Re: [PATCH 2/2] nvmet: implement persistent read-only namespace feature

[Hannes Reinecke]

On 4/4/25 08:26, Christoph Hellwig wrote:
> On Thu, Apr 03, 2025 at 04:47:47PM +0200, Hannes Reinecke wrote:
>> Per default we try to open the 'device_path' read-write, which might
>> fail if the device is read-only. So retry with opening read-only, and
>> implement a new 'persistent_ro' flag if we did so.
>> And map that flag onto the 'Permanently Write Protect' setting in the
>> write protection feature.
> 
> This sounds like a really horrible interface, but maybe there is logic
> for it so explain why we don't require the explicit read-only attribute
> to open read only.
> 
I do agree, interface needs to be reworked.
But while playing around I found that apparently 'filp_open()'
ignores the file mode; I can easily call 'filp_open()' with
O_RDWR on a file with mode 0444.
Expected?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 0/2] nvmet: improve 'read-only' handling

[Christoph Hellwig]

On Thu, Apr 03, 2025 at 04:47:45PM +0200, Hannes Reinecke wrote:
> Hi all,
> 
> there was a request (Hi Lennart)

which Lennart?  There's none on Cc.  Not that it really matters, but
I'd love to hear a bit more about the use case.

Re: [PATCH 0/2] nvmet: improve 'read-only' handling

[Hannes Reinecke]

On 4/4/25 08:19, Christoph Hellwig wrote:
> On Thu, Apr 03, 2025 at 04:47:45PM +0200, Hannes Reinecke wrote:
>> Hi all,
>>
>> there was a request (Hi Lennart)
> 
> which Lennart?  There's none on Cc.  Not that it really matters, but
> I'd love to hear a bit more about the use case.
> 
Lennart Poettering has a simple nvmet service in systemd, turning the 
entire system into an nvmet-tcp target exporting the root device.
(He needs it for testing and debugging his systemd changes).
But obviously that would expose his HDD to _anyone_, and so to prevent
$RANDOM_PERSON to access and possibly delete files on his HDD he would
like to have a 'readonly' setting.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich