Re: [PATCH v5 10/10] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers

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

New issues:
- [High] Modifying the PI metadata buffer via nvme_start_request() after it has been mapped for DMA causes cache coherency issues on non-coherent architectures.

Pre-existing issues:
- [High] Accessing the block request and its associated PDU after being freed on the error handling path when mapping or post_send fails with -EIO.
- [High] Returning BLK_STS_RESOURCE after the request has been started triggers kernel warnings and false timeouts.
- [Medium] Returning a hard error from queue_rq after calling nvme_start_request() bypasses nvme_mpath_end_request(), leaking the nr_active counter.
--

--- Patch [10]: [PATCH v5 10/10] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers ---
commit 2111f8ba9b5738dc4d1ef4e08339058c75231ebd
Author: Mykola Marzhan <[email protected]>

nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers

> A P2P transfer the PCIe topology cannot route fails DMA mapping
> with -EREMOTEIO.  nvme-rdma folds every mapping error into -EIO, a
> retryable host-path error: multipath requeues the I/O forever, a
> single path burns its whole retry budget.
> 
> Propagate the real error code and return the unroutable case as the
> non-retryable BLK_STS_P2PDMA.  As in nvme-pci, -ENOMEM now requeues
> (BLK_STS_RESOURCE) and -EINVAL fails (BLK_STS_IOERR); -EIO stays a
> retryable host-path error.  While at it, start the request only
> after mapping succeeds -- nvme-pci's order -- and ratelimit the
> map-failure message.

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 63830334c73e2..2f50509a7a616 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> [ ... ]
> @@ -2033,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 ||
> @@ -2046,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]
Since the patch moves nvme_start_request() to execute after
nvme_rdma_map_data() has mapped the metadata buffer for streaming DMA, can this
cause cache coherency issues on non-coherent architectures?

nvme_start_request() invokes blk_integrity_prepare(), which writes to the PI
tags directly in the CPU cache. 

Because this now happens after the memory is mapped, those modifications might
not be synced to main memory, causing the RDMA NIC to read stale PI tags and
leading to silent data corruption and I/O failures.

> +
>  	sqe->cqe.done = nvme_rdma_send_done;
>  
>  	ib_dma_sync_single_for_device(dev, sqe->dma,
> [ ... ]
> @@ -2070,6 +2066,9 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  err_unmap:
>  	nvme_rdma_unmap_data(queue, rq);
>  err:
>  	if (err == -EIO)
>  		ret = nvme_host_path_error(rq);

[Severity: High]
This is a pre-existing issue, but does this lead to a use-after-free when
nvme_rdma_map_data() or nvme_rdma_post_send() fails with -EIO?

nvme_host_path_error() synchronously completes the request via
blk_mq_set_request_complete() and __nvme_complete_rq(). This frees the request
and its PDU back to the tag allocator, where it can be immediately reallocated.

The code then falls through and accesses the freed request by calling
nvme_cleanup_cmd(rq) and reading req->sqe.dma during ib_dma_unmap_single().

>  	else if (err == -ENOMEM || err == -EAGAIN)
>  		ret = BLK_STS_RESOURCE;

[Severity: High]
This is a pre-existing issue, but does returning BLK_STS_RESOURCE here cause
false I/O timeouts?

At this point, nvme_start_request() has already been called, placing the
request in the MQ_RQ_IN_FLIGHT state and starting its timeout timer. 

When returning BLK_STS_RESOURCE, the block layer requeues the request without
reverting its state to MQ_RQ_IDLE. This leaves the timer ticking while the
request is on the dispatch queue, eventually triggering a false timeout and
controller reset.

> +	/* 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: Medium]
This is a pre-existing issue, but does returning a hard error here leak the
multipath nr_active counter?

When a hard error is returned to the block layer from nvme_rdma_queue_rq(),
it directly completes the request via blk_mq_end_request(), bypassing the
driver's .complete callback. 

Since nvme_start_request() was already called and incremented the multipath
nr_active counter, bypassing the completion callback means
nvme_mpath_end_request() is never invoked to balance the counter.

>  unmap_qe:
>  	ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
>  			    DMA_TO_DEVICE);
>  	return ret;
>  }

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