[PATCH] nvmet-tcp: fix possible list corruption for unexpected command failure

[Keith Busch]

On Wed, Dec 08, 2021 at 03:35:06PM +0200, Sagi Grimberg wrote:
> nvmet_tcp_handle_req_failure needs to understand weather to prepare
> for incoming data or the next pdu. However if we misidentify this, we
> will wait for 0-length data, and queue the response although nvmet_req_init
> already did that.
> 
> The particular command was namespace management command with no data,
> which was incorrectly categorized as a command with incapsule data.
> 
> Also, add a code comment of what we are trying to do here.
> 
> Reported-by: Witold Baryluk
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/target/tcp.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index cb6a473c3eaf..7c1c43ce466b 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -922,7 +922,14 @@ static void nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
>  	size_t data_len = le32_to_cpu(req->cmd->common.dptr.sgl.length);
>  	int ret;
>  
> -	if (!nvme_is_write(cmd->req.cmd) ||
> +	/*
> +	 * This command has not been processed yet, hence we are trying to
> +	 * figure out if there is still pending data left to receive. If
> +	 * we don't, we can simply prepare for the next pdu and bail out,
> +	 * otherwise we will need to prepare a buffer and receive the
> +	 * stale data before continuing forward.
> +	 */
> +	if (!nvme_is_write(cmd->req.cmd) || !data_len ||
>  	    data_len > cmd->req.port->inline_data_size) {
>  		nvmet_prepare_receive_pdu(queue);
>  		return;
> -- 

Let me see if I can get this understood:

 1. nvmet_req_init() will queue a response from tcp's RECV_PDU state if
    the current command is a read, no data, or a write with data length
    exceeding inline length.
 2. nvmet_tcp_handle_req_failure(), which is called immediately after
    nvmet_req_init(), will abandon the current cmd if it's a read, a
    zero data command, or a data length exceeding inline length;
    otherwise it will later queue the response.

It sounds like all the cases are covered to ensure exactly one response
gets enqueued for this error case. I think this works, however, a little
confusing with the dependency on tcp's .queue_response special handling.
:)

Anyway,

Reviewed-by: Keith Busch <kbusch at kernel.org>