Re: [PATCH v3 8/8] 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:
- [Critical] Calling `nvme_cleanup_cmd(rq)` and unmapping the command after `nvme_host_path_error(rq)` causes a Use-After-Free.
- [Medium] Multipath accounting and I/O stats leak when `nvme_rdma_post_send` fails with unhandled errors.
--

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

nvme-rdma: return BLK_STS_TARGET for unsupported P2P transfers

This commit updates nvme-rdma to map scatterlists using
ib_dma_map_sgtable_attrs() to properly capture -EREMOTEIO. It returns
BLK_STS_TARGET for this error to avoid infinite requeuing of unsupported
P2P transfers. Additionally, it delays starting the request until after
mapping succeeds to avoid taking multipath accounting for unmapped requests.

Cc: [email protected] # v7.1

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6909e35427942..f8edb23552ad2 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
[ ... ]
> @@ -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 error path cause a use-after-free
if err == -EIO?

Looking at nvme_host_path_error(), it synchronously completes and frees the
request back to the blk-mq tag pool:

drivers/nvme/host/core.c:nvme_host_path_error() {
	nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
	blk_mq_set_request_complete(req);
	__nvme_complete_rq(req);
	return BLK_STS_OK;
}

After it returns, the code falls through to nvme_cleanup_cmd(rq) and
unmaps the sqe.dma:

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

Could a concurrent thread allocate this freed request and map a new DMA
address before we unmap it here, leading to an IOMMU fault or corruption?

>  	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_TARGET;
>  	else
>  		ret = BLK_STS_IOERR;

[Severity: Medium]
This is a pre-existing issue, but does returning BLK_STS_IOERR for unhandled
errors bypass NVMe multipath cleanup?

If nvme_rdma_post_send() fails with an unexpected error code (like -EINVAL),
we fall through here and return BLK_STS_IOERR. The block layer will directly
complete the request via blk_mq_end_request().

Since nvme_start_request(rq) was called earlier and set NVME_MPATH_CNT_ACTIVE,
does bypassing the NVMe-specific nvme_mpath_end_request() cause multipath
accounting to leak (ns->ctrl->nr_active is never decremented)?

>  	nvme_cleanup_cmd(rq);

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