Re: [PATCH 1/3] nvmet: pci-epf: Always fully initialize completion entries

[Niklas Cassel <cassel@kernel.org>]

On Tue, Apr 08, 2025 at 11:47:31AM +0900, Damien Le Moal wrote:
> For a command that is normally processed through the command request
> execute() function, the completion entry for the command is initialized
> by __nvmet_req_complete() and nvmet_pci_epf_cq_work() only needs to set
> the status field and the phase of the completion entry before posting
> the entry to the completion queue.
> 
> However, for commands that are failed due to an internal error (e.g. the
> command data buffer allocation fails), the command request execute()
> function is not called and __nvmet_req_complete() is never executed for
> the command, leaving the command completion entry uninitialized. For
> such command failed before calling req->execute(), the host ends up
> seeing completion entries with an invalid submission queue ID and
> command ID.
> 
> Avoid such issue by always fully initilizing a command completion entry
> in nvmet_pci_epf_cq_work(), setting the entry submission queue ID and
> command ID.
> 
> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 51c27b32248d..f6b22ef4c267 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1763,6 +1763,8 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
>  
>  		/* Post the IOD completion entry. */
>  		cqe = &iod->cqe;
> +		cqe->sq_id = cpu_to_le16(iod->sq->qid);
> +		cqe->command_id = iod->cmd.common.command_id;
>  		cqe->status = cpu_to_le16((iod->status << 1) | cq->phase);

Looking at e.g. __nvmet_req_complete() and __nvmet_fc_fcp_nvme_cmd_done()
in addition to setting sq_id, command_id, and status, they also set:
cqe->sq_head.

(__nvmet_req_complete() -> nvmet_update_sq_head() -> req->cqe->sq_head = ...)

Should we perhaps set sq_head too?


Kind regards,
Niklas