Re: [PATCH] nvme-tcp: fix incorrect h2cdata pdu offset accounting

From: Sagi Grimberg <sagi@grimberg.me>
To: Keith Busch <kbusch@kernel.org>
Cc: linux-nvme@lists.infradead.org, Christoph Hellwig <hch@lst.de>,
	Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Subject: Re: [PATCH] nvme-tcp: fix incorrect h2cdata pdu offset accounting
Date: Sun, 19 Sep 2021 10:12:21 +0300	[thread overview]
Message-ID: <a006947f-8494-4e8a-1ab6-72372018b4e0@grimberg.me> (raw)
In-Reply-To: <20210916164559.GB3908552@dhcp-10-100-145-180.wdc.com>


On 9/16/21 7:45 PM, Keith Busch wrote:
> On Tue, Sep 14, 2021 at 06:38:55PM +0300, Sagi Grimberg wrote:
>> When the controller sends us multiple r2t PDUs in a single
>> request we need to account for it correctly as our send/recv
>> context run concurrently (i.e. we get a new r2t with r2t_offset
>> before we updated our iterator and req->data_sent marker). This
>> can cause wrong offsets to be sent to the controller.
>>
>> To fix that, we will first know that this may happen only in
>> the send sequence of the last page, hence we will take
>> the r2t_offset to the h2c PDU data_offset, and in
>> nvme_tcp_try_send_data loop, we make sure to increment
>> the request markers also when we completed a PDU but
>> we are expecting more r2t PDUs as we still did not send
>> the entire data of the request.
>>
>> Fixes: 825619b09ad3 ("nvme-tcp: fix possible use-after-completion")
>> Reported-by: Nowak, Lukasz <Lukasz.Nowak@Dell.com>
>> Tested-by: Nowak, Lukasz <Lukasz.Nowak@Dell.com>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>> - Keith, can you ask the WD team to test this as well?
>>
>>   drivers/nvme/host/tcp.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 645025620154..87b73eb94041 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -607,7 +607,7 @@ static int nvme_tcp_setup_h2c_data_pdu(struct nvme_tcp_request *req,
>>   		cpu_to_le32(data->hdr.hlen + hdgst + req->pdu_len + ddgst);
>>   	data->ttag = pdu->ttag;
>>   	data->command_id = nvme_cid(rq);
>> -	data->data_offset = cpu_to_le32(req->data_sent);
>> +	data->data_offset = pdu->r2t_offset;
> 
> Shouldn't this be "le32_to_cpu(pdu->r2t_offset)"?

No, data is a wire payload, so it is le32

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme