Re: [PATCH v2 7/7] 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:

Pre-existing issues:
- [High] Preexisting use-after-free and DMA unmap corruption in the `nvme_rdma_queue_rq` error path.
- [High] Preexisting resource leak of multipath `nr_active` and QoS metrics when `nvme_rdma_post_send()` fails.
--

--- Patch [7]: [PATCH v2 7/7] nvme-rdma: return BLK_STS_TARGET for unsupported P2P transfers ---
commit 8ebcd8a34c348b015bd58fe860d4af84c9a7b8c5
Author: Mykola Marzhan <[email protected]>

nvme-rdma: return BLK_STS_TARGET for unsupported P2P transfers

This commit updates nvme-rdma to correctly handle P2PDMA mapping failures
by preserving the DMA layer's error code. It translates -EREMOTEIO to
BLK_STS_TARGET to avoid endless requeue livelocks for unreachable peer
memory, and it defers nvme_start_request() until after mapping succeeds.

Cc: [email protected] # v7.1

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6909e35427942..9017d927edc41 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]
This is a pre-existing issue, but if nvme_rdma_post_send() fails further
down, does returning a block status error after nvme_start_request() has
been called bypass the NVMe completion handlers?

Since the block layer will natively complete the request in this scenario,
it looks like nvme_mpath_end_request() is bypassed, which could permanently
leak the nr_active counter and break multipath load balancing.

>  	sqe->cqe.done = nvme_rdma_send_done;
>  
>  	ib_dma_sync_single_for_device(dev, sqe->dma,
> @@ -2063,6 +2066,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 can this error path lead to a
use-after-free and DMA unmap corruption?

If nvme_rdma_post_send() fails with -EIO, nvme_host_path_error(rq) is
called, which synchronously completes and frees the request. The code then
falls through to execute nvme_cleanup_cmd(rq) and ib_dma_unmap_single() on
the already freed request.

Could this unmap the active DMA region of a completely different, newly
allocated I/O command?

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