[PATCH v2] nvmet-tcp: fix a segmentation fault during io parsing error

[Grupi, Elad]

Sure. Done.

Also avoided queueing failed REQ as discussed with Hou.

Elad

-----Original Message-----
From: Sagi Grimberg <sagi at grimberg.me> 
Sent: Saturday, 27 March 2021 0:03
To: Grupi, Elad; linux-nvme at lists.infradead.org
Subject: Re: [PATCH v2] nvmet-tcp: fix a segmentation fault during io parsing error


[EXTERNAL EMAIL] 



On 3/26/21 10:59 AM, elad.grupi at dell.com wrote:
> From: Elad Grupi <elad.grupi at dell.com>
> 
> In case there is an io that contains inline data and it goes to 
> parsing error flow, command response will free command and iov before 
> clearing the data on the socket buffer.
> This will delay the command response until receive flow is completed.
> 
> Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
> Signed-off-by: Elad Grupi <elad.grupi at dell.com>
> ---
>   drivers/nvme/target/tcp.c | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c 
> index 70cc507d1565..3bc0caa47873 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -702,6 +702,17 @@ static int nvmet_tcp_try_send_one(struct nvmet_tcp_queue *queue,
>   			return 0;
>   	}
>   
> +	if (unlikely((cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
> +			nvmet_tcp_has_data_in(cmd) &&
> +			nvmet_tcp_has_inline_data(cmd))) {
> +		/*
> +		 * wait for inline data before processing the response
> +		 * so the iov will not be freed
> +		 */
> +		queue->snd_cmd = NULL;
> +		goto done_send;
> +	}
> +
>   	if (cmd->state == NVMET_TCP_SEND_DATA_PDU) {
>   		ret = nvmet_try_send_data_pdu(cmd);
>   		if (ret <= 0)
> @@ -1103,9 +1114,11 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
>   		return 0;
>   	}
>   
> -	if (!(cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
> -	    cmd->rbytes_done == cmd->req.transfer_len) {
> -		cmd->req.execute(&cmd->req);
> +	if (cmd->rbytes_done == cmd->req.transfer_len) {
> +		if (unlikely(cmd->flags & NVMET_TCP_F_INIT_FAILED))
> +			nvmet_tcp_queue_response(&cmd->req);
> +		else
> +			cmd->req.execute(&cmd->req);

Given that this is called twice, care to move it to a new helper nvme_tcp_execute_request?