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

[Sagi Grimberg]

On 14/04/2025 15:35, Hannes Reinecke wrote:
> On 4/14/25 01:00, Sagi Grimberg wrote:
>>
>>
>> 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)?
>>
> This is coming from code inspection only.
> In nvme_tcp_handle_r2t() we are looking up a request by the 'command_id'
> value in the pdu, and then add it to 'queue->req_list' without further
> checking.
> So if a malicious R2T packet is received containing the command_id of
> a command currently on 'queue->req_list' we'll end up with duplicate
> entries on the list.

I think this falls into a bucket of a bunch checks that the host does 
not do. But ok.

>
> [ .. ]
>
>>> @@ -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.
>>
> Arguably an optimisation. I can leave it out.

When it comes in this context it is not clear what it is designed to do. 
Don't mind having it,
but lets separate it from this patch.