nvme-pci: add missing condition check for existence of mapped data

[PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Leon Romanovsky 1 year, 4 months ago

From: Leon Romanovsky <leonro@nvidia.com>

nvme_map_data() is called when request has physical segments, hence
the nvme_unmap_data() should have same condition to avoid dereference.

Fixes: 4aedb705437f ("nvme-pci: split metadata handling from nvme_map_data / nvme_unmap_data")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/nvme/host/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 102a9fb0c65f..cdc0b25091e9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -863,7 +863,8 @@ static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
 	nvme_start_request(req);
 	return BLK_STS_OK;
 out_unmap_data:
-	nvme_unmap_data(dev, req);
+	if (blk_rq_nr_phys_segments(req))
+		nvme_unmap_data(dev, req);
 out_free_cmd:
 	nvme_cleanup_cmd(req);
 	return ret;
-- 
2.45.2

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Keith Busch 1 year, 4 months ago

On Wed, Jul 24, 2024 at 01:31:14PM +0300, Leon Romanovsky wrote:
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 102a9fb0c65f..cdc0b25091e9 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -863,7 +863,8 @@ static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
>  	nvme_start_request(req);
>  	return BLK_STS_OK;
>  out_unmap_data:
> -	nvme_unmap_data(dev, req);
> +	if (blk_rq_nr_phys_segments(req))
> +		nvme_unmap_data(dev, req);

This is already applied, but it is kind of strange. We get here only if
metadata mapping fails. Is there actually a command that has metadata
without data?

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Christoph Hellwig 1 year, 4 months ago

On Tue, Jul 30, 2024 at 11:09:04AM -0600, Keith Busch wrote:
> > +	if (blk_rq_nr_phys_segments(req))
> > +		nvme_unmap_data(dev, req);
> 
> This is already applied, but it is kind of strange. We get here only if
> metadata mapping fails. Is there actually a command that has metadata
> without data?

Well, passthrough can always set metadata to map without data even
if there is no NVMe defined command that works that way, so we should
handle the error.

But I suspect this is due to Leon's dma-mapping work, and it probably
points to a bug in that :)

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Leon Romanovsky 1 year, 4 months ago

On Tue, Jul 30, 2024 at 07:21:11PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 30, 2024 at 11:09:04AM -0600, Keith Busch wrote:
> > > +	if (blk_rq_nr_phys_segments(req))
> > > +		nvme_unmap_data(dev, req);
> > 
> > This is already applied, but it is kind of strange. We get here only if
> > metadata mapping fails. Is there actually a command that has metadata
> > without data?
> 
> Well, passthrough can always set metadata to map without data even
> if there is no NVMe defined command that works that way, so we should
> handle the error.
> 
> But I suspect this is due to Leon's dma-mapping work, and it probably
> points to a bug in that :)

Yeah, something like that. I had a bug in my code and saw this
asymmetry while reviewed all paths which lead to nvme_unmap_data().

Thanks

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Keith Busch 1 year, 4 months ago

On Tue, Jul 30, 2024 at 07:21:11PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 30, 2024 at 11:09:04AM -0600, Keith Busch wrote:
> > > +	if (blk_rq_nr_phys_segments(req))
> > > +		nvme_unmap_data(dev, req);
> > 
> > This is already applied, but it is kind of strange. We get here only if
> > metadata mapping fails. Is there actually a command that has metadata
> > without data?
> 
> Well, passthrough can always set metadata to map without data even
> if there is no NVMe defined command that works that way, so we should
> handle the error.

That's what I initially thought too, but nvme passthrough maps metadata
only if there's also user data. It doesn't look like you can even build
a request to have metadata if it doesn't have user data: where does the
bio come from in that case?

And generic block stack maps it only for READ or WRITE commands, which
must have payloads too, so I didn't find a path to reach this condition.

Not a big deal, the patch is fine, but I was wondering if we need to
change something else to allow the conditions it proposes to fix.

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Keith Busch 1 year, 4 months ago

On Wed, Jul 24, 2024 at 01:31:14PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> nvme_map_data() is called when request has physical segments, hence
> the nvme_unmap_data() should have same condition to avoid dereference.

Thanks, applied to nvme-6.11.

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Christoph Hellwig 1 year, 4 months ago

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] nvme-pci: add missing condition check for existence of mapped data

Posted by Nitesh Shetty 1 year, 4 months ago

On 24/07/24 01:31PM, Leon Romanovsky wrote:
>From: Leon Romanovsky <leonro@nvidia.com>
>
>nvme_map_data() is called when request has physical segments, hence
>the nvme_unmap_data() should have same condition to avoid dereference.
>
>Fixes: 4aedb705437f ("nvme-pci: split metadata handling from nvme_map_data / nvme_unmap_data")
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---

Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>