[PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[Puranjay Mohan]

On an NVMe namespace that does not support metadata, it is possible to
send an IO command with metadata through io-passthru.
nvme_map_user_request() doesn't check if the namespace supports metadata
before sending it forward.

Reject an IO command with metadata when the NVMe namespace doesn't
support it.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Puranjay Mohan <pjy@amazon.com>
---
 drivers/nvme/host/ioctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index f1d58e70933f..4f49523b8a41 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2017-2021 Christoph Hellwig.
  */
 #include <linux/bio-integrity.h>
+#include <linux/blk-integrity.h>
 #include <linux/ptrace.h>	/* for force_successful_syscall_return */
 #include <linux/nvme_ioctl.h>
 #include <linux/io_uring/cmd.h>
@@ -122,6 +123,9 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 	struct bio *bio = NULL;
 	int ret;
 
+	if (meta_buffer && meta_len && bdev && !blk_get_integrity(bdev->bd_disk))
+		return -EINVAL;
+
 	if (ioucmd && (ioucmd->flags & IORING_URING_CMD_FIXED)) {
 		struct iov_iter iter;
 
-- 
2.40.1

Re: [PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 12:17:01PM +0000, Puranjay Mohan wrote:
> +	if (meta_buffer && meta_len && bdev && !blk_get_integrity(bdev->bd_disk))
> +		return -EINVAL;

Overly long line here.  If we go past my initial RFC I'd probably
restructure the function a little bit, i.e. add a new

	bool has_metadata = bdev && meta_buffer && meta_len;

and then use that both for the support check and the actualy mapping
below.

Re: [PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[Puranjay Mohan]

Christoph Hellwig <hch@lst.de> writes:

> On Tue, Aug 27, 2024 at 12:17:01PM +0000, Puranjay Mohan wrote:
>> +	if (meta_buffer && meta_len && bdev && !blk_get_integrity(bdev->bd_disk))
>> +		return -EINVAL;
>
> Overly long line here.  If we go past my initial RFC I'd probably
> restructure the function a little bit, i.e. add a new
>
> 	bool has_metadata = bdev && meta_buffer && meta_len;
>
> and then use that both for the support check and the actualy mapping
> below.

Sure,
I will send v2 with these changes now.

P.S. - It looks like we will need manual backports for stable kernels as
this won't apply directly. I will send them after this is accepted.

Thanks,
Puranjay

Re: [PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[Keith Busch]

On Tue, Aug 27, 2024 at 12:28:45PM +0000, Puranjay Mohan wrote:
> 
> P.S. - It looks like we will need manual backports for stable kernels as
> this won't apply directly. I will send them after this is accepted.

BTW, the reason for your bug observation on older stable but not on
newer ones looks like was originally "fixed" with:

  d4aa57a1cac3c99 ("block: don't bother iter advancing a fully done bio")

So while your change is fine, the above commit possibly inadvertently
fixes the inappropriate "advance" because all successful nvme
completions are "fully done".

It still probably doesn't make sense to attempt metadata on request
queues that didn't register with the interface though, so your patch is
also fine.

Re: [PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[Keith Busch]

On Tue, Aug 27, 2024 at 12:17:01PM +0000, Puranjay Mohan wrote:
> @@ -122,6 +123,9 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
>  	struct bio *bio = NULL;
>  	int ret;
>  
> +	if (meta_buffer && meta_len && bdev && !blk_get_integrity(bdev->bd_disk))
> +		return -EINVAL;
> +

Should we also fail if there's no bdev? The driver won't use the
requested metadata for admin commands either, so that is also an invalid
user request.

Re: [PATCH] nvme: check if the namespace supports metadata in nvme_map_user_request()

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 09:42:08AM -0600, Keith Busch wrote:
> On Tue, Aug 27, 2024 at 12:17:01PM +0000, Puranjay Mohan wrote:
> > @@ -122,6 +123,9 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
> >  	struct bio *bio = NULL;
> >  	int ret;
> >  
> > +	if (meta_buffer && meta_len && bdev && !blk_get_integrity(bdev->bd_disk))
> > +		return -EINVAL;
> > +
> 
> Should we also fail if there's no bdev? The driver won't use the
> requested metadata for admin commands either, so that is also an invalid
> user request.

Yes, the last version gets the right.