Re: [PATCH 6/6] nvme-rdma: return BLK_STS_TARGET for unsupported P2P transfers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Data corruption and PI validation failure due to modifying integrity metadata after DMA mapping.

Pre-existing issues:
- [High] Use-After-Free of request and PDU on the -EIO error path.
--

--- Patch [6]: [PATCH 6/6] nvme-rdma: return BLK_STS_TARGET for unsupported P2P transfers ---
commit efc6b55af9178f85e847e999c50cc6efb530a8b9
Author: Mykola Marzhan <[email protected]>

nvme-rdma: return BLK_STS_TARGET for unsupported P2P transfers

This patch addresses an issue where mapping failures for peer memory are
misreported as path errors instead of target errors. It changes DMA
mapping to use ib_dma_map_sgtable_attrs() to preserve error codes and maps
-EREMOTEIO to BLK_STS_TARGET. It also reorders nvme_start_request() to
execute after the mapping succeeds.

Cc: [email protected] # v7.1

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6909e35427942..b8642cd2fb793 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
[ ... ]
> @@ -2039,11 +2038,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	err = nvme_rdma_map_data(queue, rq, c);
>  	if (unlikely(err < 0)) {
> -		dev_err(queue->ctrl->ctrl.device,
> -			     "Failed to map data (%d)\n", err);
> +		dev_err_ratelimited(queue->ctrl->ctrl.device,
> +				    "Failed to map data (%d)\n", err);
>  		goto err;
>  	}
>  
> +	nvme_start_request(rq);
> +

[Severity: High]
Does moving nvme_start_request() after nvme_rdma_map_data() cause Protection
Information (PI) validation failures?

nvme_start_request() calls blk_mq_start_request(), which eventually calls
blk_integrity_prepare() to insert reference tags into the metadata payload.

Since nvme_rdma_map_data() has already mapped the metadata for DMA
(potentially using SWIOTLB or syncing caches), wouldn't the device receive
stale metadata without the newly generated tags?

>  	sqe->cqe.done = nvme_rdma_send_done;
>  
>  	ib_dma_sync_single_for_device(dev, sqe->dma,
[ ... ]
> @@ -2063,6 +2064,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		ret = nvme_host_path_error(rq);
>  	else if (err == -ENOMEM || err == -EAGAIN)
>  		ret = BLK_STS_RESOURCE;
> +	/*
> +	 * The DMA layer refused to map peer memory to this device: a
> +	 * property of the pairing, not a path failure.  Match nvme-pci
> +	 * and do not retry (see blk_path_error()).
> +	 */
> +	else if (err == -EREMOTEIO)
> +		ret = BLK_STS_TARGET;
>  	else
>  		ret = BLK_STS_IOERR;
>  	nvme_cleanup_cmd(rq);

[Severity: High]
This isn't a bug introduced by this patch, but does the -EIO error path
trigger a use-after-free of rq and req?

If nvme_rdma_map_data() fails with -EIO, we call nvme_host_path_error(), which
synchronously completes and frees rq via blk_mq_end_request().

The code then falls through to nvme_cleanup_cmd(rq) and ib_dma_unmap_single(),
which access the freed rq and its PDU (req).

If the request was immediately re-allocated for another command, could this
read the wrong DMA address and unmap an active DMA mapping?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6