Re: [PATCH 2/2] nvme/ioctl: call io_uring_cmd_set_res32() in ->end_io()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Polled NVMe uring_cmd requests completing via IRQ cause a Use-After-Free in nvme_ns_chr_uring_cmd_iopoll.
- [Critical] io_do_iopoll unconditionally overwrites req->cqe.flags, destroying IORING_CQE_F_32 for polled uring_cmd requests.
--

commit 67604b5180877187b81d959a1a1acc3b5bc52ca4
Author: Caleb Sander Mateos <csander@purestorage.com>

nvme/ioctl: call io_uring_cmd_set_res32() in ->end_io()

This commit moves io_uring_cmd_set_res32() from the task work callback
into the end_io handler for nvme uring commands. This eliminates the need
to store the result and status in the PDU, saving memory and unifying
the completion path.

> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 269afd5bc02e3..8d357c47111f8 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c

[ ... ]

> @@ -414,15 +411,17 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
>  {
>  	struct io_uring_cmd *ioucmd = req->end_io_data;
>  	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
> +	u64 result = le64_to_cpu(nvme_req(req)->result.u64);
> +	int status;
>  
>  	if (nvme_req(req)->flags & NVME_REQ_CANCELLED) {
> -		pdu->status = -EINTR;
> +		status = -EINTR;
>  	} else {
> -		pdu->status = nvme_req(req)->status;
> -		if (!pdu->status)
> -			pdu->status = blk_status_to_errno(err);
> +		status = nvme_req(req)->status;
> +		if (!status)
> +			status = blk_status_to_errno(err);
>  	}
> -	pdu->result = le64_to_cpu(nvme_req(req)->result.u64);
> +	io_uring_cmd_set_res32(ioucmd, status, result);

[Severity: Critical]
This is a pre-existing issue, but does moving this call before polling expose
a regression with IORING_CQE_F_32?

When io_uring_cmd_set_res32() posts 32-byte CQE results, it sets:
    req->cqe.flags |= IORING_CQE_F_32;

However, for polled requests, io_do_iopoll() later executes:
    req->cqe.flags = io_put_kbuf(req, max(req->cqe.res, 0), NULL);

This unconditional assignment completely overwrites the flags, erasing
IORING_CQE_F_32. As a result, when io_fill_cqe_req() processes the
completion, it allocates a 16-byte slot instead of 32 bytes and silently
drops extra1 and extra2 (the NVMe-specific result values).

>  
>  	/*
>  	 * For IOPOLL, check if this completion is happening in the context
> @@ -435,7 +434,6 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
>  	    iob->poll_ctx == io_uring_cmd_ctx_handle(ioucmd)) {
>  		if (pdu->bio)
>  			blk_rq_unmap_user(pdu->bio);
> -		io_uring_cmd_set_res32(ioucmd, pdu->status, pdu->result);
>  		io_uring_cmd_done(ioucmd, 0);
>  	} else {
>  		io_uring_cmd_do_in_task_lazy(ioucmd, nvme_uring_task_cb);
>  	}
>  	return RQ_END_IO_FREE;

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a use-after-free
regression when a polled NVMe uring_cmd completes via an interrupt?

If the completion is not in the expected poll context (e.g. hybrid polling
or a spurious interrupt), this path queues nvme_uring_task_cb() via
io_uring_cmd_do_in_task_lazy() and returns RQ_END_IO_FREE, which frees the
block request immediately.

However, the io_uring state flag req->iopoll_completed is not set to 1 until
the task_work runs. Concurrently, the io_uring polling thread in
io_do_iopoll() sees req->iopoll_completed == 0 and invokes
nvme_ns_chr_uring_cmd_iopoll().

This function then dereferences pdu->req (which points to the just-freed
block request) to check blk_rq_is_poll(req):

drivers/nvme/host/ioctl.c:nvme_ns_chr_uring_cmd_iopoll() {
    ...
    struct request *req = pdu->req;

    if (req && blk_rq_is_poll(req))
    ...
}

Can this lead to memory corruption if the unprivileged user issuing the
command continues polling while the interrupt path frees the request?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827185722.3234622-1-csander@purestorage.com?part=2