Re: [PATCH v4 9/9] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers

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

New issues:
- [High] Calling nvme_start_request() after mapping the data for DMA causes stale metadata to be read by the RDMA device on non-coherent architectures.

Pre-existing issues:
- [Critical] Accessing `rq` and `req->sqe.dma` in the error path after `nvme_host_path_error(rq)` completes and frees the request.
- [High] Returning BLK_STS_IOERR or BLK_STS_P2PDMA directly to the block layer after nvme_start_request() has been called leaks the multipath nr_active counter and I/O stats.
--

--- Patch [9]: [PATCH v4 9/9] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers ---
commit 53acbc82938f48638ba2fe9095582a69fb22439e
Author: Mykola Marzhan <[email protected]>

nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers

When the PCIe topology cannot route a P2P transfer between two devices,
the DMA layer fails the mapping with -EREMOTEIO, but ib_dma_map_sg()
returns 0 and the error is lost. nvme-rdma then fails the I/O with a
retryable status, so multipath requeues it forever and a single path
wastes its whole retry budget on an I/O that can never succeed.

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6909e35427942..2f50509a7a616 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
[ ... ]
> @@ -2026,8 +2029,6 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	if (ret)
>  		goto unmap_qe;
>  
> -	nvme_start_request(rq);
> -
>  	if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
>  	    queue->pi_support &&
>  	    (c->common.opcode == nvme_cmd_write ||
> @@ -2039,11 +2040,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() here cause stale metadata to be read on
non-coherent architectures?

nvme_start_request() invokes blk_integrity_prepare(), which writes to the PI
metadata buffer via the CPU. Since this now happens after the metadata has
already been mapped for DMA in nvme_rdma_map_data(), these CPU writes won't
be flushed to main memory before the RDMA device accesses them.

>  	sqe->cqe.done = nvme_rdma_send_done;
>  
>  	ib_dma_sync_single_for_device(dev, sqe->dma,
> @@ -2063,6 +2066,9 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		ret = nvme_host_path_error(rq);

[Severity: Critical]
This is a pre-existing issue, but does this lead to a use-after-free?

nvme_host_path_error() synchronously completes the request via
__nvme_complete_rq(), freeing it back to the block layer where it can be
immediately reallocated.

The error path then continues to dereference rq and req->sqe.dma:

	nvme_cleanup_cmd(rq);
unmap_qe:
	ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
			    DMA_TO_DEVICE);

Could this corrupt the newly reallocated request if it's reused by another CPU?

>  	else if (err == -ENOMEM || err == -EAGAIN)
>  		ret = BLK_STS_RESOURCE;
> +	/* Peer memory unreachable from this device: don't retry. */
> +	else if (err == -EREMOTEIO)
> +		ret = BLK_STS_P2PDMA;
>  	else
>  		ret = BLK_STS_IOERR;
>  	nvme_cleanup_cmd(rq);

[Severity: High]
This isn't a bug introduced by this patch, but does returning an error status
directly to the block layer after nvme_start_request() leak multipath
counters?

When nvme_start_request() is executed, it increments the queue depth counter
(nr_active) and starts I/O accounting via nvme_mpath_start_request().

If the driver then returns an error status (like BLK_STS_P2PDMA or
BLK_STS_IOERR) to blk-mq, blk-mq completes the request directly, bypassing the
driver's nvme_mpath_end_request() hook. This permanently leaks the nr_active
counter.

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