[PATCH 2/3] nvme-tcp: sanitize request list handling

[Sagi Grimberg]

On 03/04/2025 9:55, Hannes Reinecke wrote:
> Validate the request in nvme_tcp_handle_r2t() to ensure it's not
> part of any list, otherwise a malicious R2T PDU might inject a
> loop in request list processing.

Not clear what do you mean by "malicious R2T PDU".
Can you please clarify what you have seen/observed in the commit msg 
(i.e. what led you
to this patch)?

>
> Signed-off-by: Hannes Reinecke <hare at kernel.org>
> ---
>   drivers/nvme/host/tcp.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index b2f2aef2e2f2..1a319cb86453 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -454,6 +454,7 @@ nvme_tcp_fetch_request(struct nvme_tcp_queue *queue)
>   	}
>   
>   	list_del(&req->entry);
> +	init_llist_node(&req->lentry);
>   	return req;
>   }
>   
> @@ -561,6 +562,8 @@ static int nvme_tcp_init_request(struct blk_mq_tag_set *set,
>   	req->queue = queue;
>   	nvme_req(rq)->ctrl = &ctrl->ctrl;
>   	nvme_req(rq)->cmd = &pdu->cmd;
> +	init_llist_node(&req->lentry);
> +	INIT_LIST_HEAD(&req->entry);
>   
>   	return 0;
>   }
> @@ -765,6 +768,15 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue,
>   		return -EPROTO;
>   	}
>   
> +	if (queue->request == req ||
> +	    llist_on_list(&req->lentry) ||
> +	    !list_empty(&req->entry)) {
> +		dev_err(queue->ctrl->ctrl.device,
> +			"req %d unexpected r2t while processing request\n",
> +			rq->tag);
> +		return -EPROTO;
> +	}
> +
>   	req->pdu_len = 0;
>   	req->h2cdata_left = r2t_length;
>   	req->h2cdata_offset = r2t_offset;
> @@ -773,7 +785,8 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue,
>   	nvme_tcp_setup_h2c_data_pdu(req);
>   
>   	llist_add(&req->lentry, &queue->req_list);
> -	queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
> +	if (list_empty(&queue->send_list))
> +		queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);

Is this change mandatory? looks out of place.