* [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again)
@ 2021-10-24 7:43 Sagi Grimberg
2021-10-25 7:18 ` Sagi Grimberg
2021-10-25 14:38 ` Keith Busch
0 siblings, 2 replies; 6+ messages in thread
From: Sagi Grimberg @ 2021-10-24 7:43 UTC (permalink / raw)
To: linux-nvme, Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
We should not access request members after the last send, even to
determine if indeed it was the last data payload send. The reason is
that a completion could have arrived and trigger a new execution of the
request which overridden these members. This was fixed by commit
825619b09ad3 ("nvme-tcp: fix possible use-after-completion").
Commit e371af033c56 broke that assumption again to address cases where
multiple r2t pdus are sent per request. To fix it, we need to record the
request data_sent and data_len and after the payload network send we
reference these counters to determine weather we should advance the
request iterator.
Fixes: e371af033c56 ("nvme-tcp: fix incorrect h2cdata pdu offset accounting")
Reported-by: Keith Busch <kbusch@kernel.org>
Cc: stable@vger.kernel.org # 5.10+
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/tcp.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 78966b8ddb1e..42b58eb1ba62 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -926,15 +926,17 @@ static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
{
struct nvme_tcp_queue *queue = req->queue;
+ int req_data_len = req->data_len;
while (true) {
struct page *page = nvme_tcp_req_cur_page(req);
size_t offset = nvme_tcp_req_cur_offset(req);
size_t len = nvme_tcp_req_cur_length(req);
- bool last = nvme_tcp_pdu_last_send(req, len);
+ bool pdu_last = nvme_tcp_pdu_last_send(req, len);
+ int req_data_sent = req->data_sent;
int ret, flags = MSG_DONTWAIT;
- if (last && !queue->data_digest && !nvme_tcp_queue_more(queue))
+ if (pdu_last && !queue->data_digest && !nvme_tcp_queue_more(queue))
flags |= MSG_EOR;
else
flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST;
@@ -958,11 +960,11 @@ static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
* in the request where we don't want to modify it as we may
* compete with the RX path completing the request.
*/
- if (req->data_sent + ret < req->data_len)
+ if (req_data_sent + ret < req_data_len)
nvme_tcp_advance_req(req, ret);
/* fully successful last send in current PDU */
- if (last && ret == len) {
+ if (pdu_last && ret == len) {
if (queue->data_digest) {
nvme_tcp_ddgst_final(queue->snd_hash,
&req->ddgst);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again)
2021-10-24 7:43 [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again) Sagi Grimberg
@ 2021-10-25 7:18 ` Sagi Grimberg
2021-10-25 14:38 ` Keith Busch
1 sibling, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2021-10-25 7:18 UTC (permalink / raw)
To: linux-nvme, Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
> We should not access request members after the last send, even to
> determine if indeed it was the last data payload send. The reason is
> that a completion could have arrived and trigger a new execution of the
> request which overridden these members. This was fixed by commit
> 825619b09ad3 ("nvme-tcp: fix possible use-after-completion").
>
> Commit e371af033c56 broke that assumption again to address cases where
> multiple r2t pdus are sent per request. To fix it, we need to record the
> request data_sent and data_len and after the payload network send we
> reference these counters to determine weather we should advance the
> request iterator.
Keith, can I get a reviewed/tested tag here?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again)
2021-10-24 7:43 [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again) Sagi Grimberg
2021-10-25 7:18 ` Sagi Grimberg
@ 2021-10-25 14:38 ` Keith Busch
2021-10-25 20:35 ` Sagi Grimberg
2021-10-26 8:43 ` Christoph Hellwig
1 sibling, 2 replies; 6+ messages in thread
From: Keith Busch @ 2021-10-25 14:38 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: linux-nvme, Christoph Hellwig, Chaitanya Kulkarni
On Sun, Oct 24, 2021 at 10:43:31AM +0300, Sagi Grimberg wrote:
> We should not access request members after the last send, even to
> determine if indeed it was the last data payload send. The reason is
> that a completion could have arrived and trigger a new execution of the
> request which overridden these members. This was fixed by commit
> 825619b09ad3 ("nvme-tcp: fix possible use-after-completion").
>
> Commit e371af033c56 broke that assumption again to address cases where
> multiple r2t pdus are sent per request. To fix it, we need to record the
> request data_sent and data_len and after the payload network send we
> reference these counters to determine weather we should advance the
> request iterator.
>
> Fixes: e371af033c56 ("nvme-tcp: fix incorrect h2cdata pdu offset accounting")
> Reported-by: Keith Busch <kbusch@kernel.org>
> Cc: stable@vger.kernel.org # 5.10+
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/tcp.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 78966b8ddb1e..42b58eb1ba62 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -926,15 +926,17 @@ static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
> static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
> {
> struct nvme_tcp_queue *queue = req->queue;
> + int req_data_len = req->data_len;
>
> while (true) {
> struct page *page = nvme_tcp_req_cur_page(req);
> size_t offset = nvme_tcp_req_cur_offset(req);
> size_t len = nvme_tcp_req_cur_length(req);
> - bool last = nvme_tcp_pdu_last_send(req, len);
> + bool pdu_last = nvme_tcp_pdu_last_send(req, len);
I'm not sure why you opted to change the name of this variable here, but
the patch is testing successfully: no observed r2t errors since apply
this, so I think we're good to go for that regression.
Reviewed-by: Keith Busch <kbusch@kernel.org>
> + int req_data_sent = req->data_sent;
> int ret, flags = MSG_DONTWAIT;
>
> - if (last && !queue->data_digest && !nvme_tcp_queue_more(queue))
> + if (pdu_last && !queue->data_digest && !nvme_tcp_queue_more(queue))
> flags |= MSG_EOR;
> else
> flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST;
> @@ -958,11 +960,11 @@ static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
> * in the request where we don't want to modify it as we may
> * compete with the RX path completing the request.
> */
> - if (req->data_sent + ret < req->data_len)
> + if (req_data_sent + ret < req_data_len)
> nvme_tcp_advance_req(req, ret);
>
> /* fully successful last send in current PDU */
> - if (last && ret == len) {
> + if (pdu_last && ret == len) {
> if (queue->data_digest) {
> nvme_tcp_ddgst_final(queue->snd_hash,
> &req->ddgst);
> --
> 2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again)
2021-10-25 14:38 ` Keith Busch
@ 2021-10-25 20:35 ` Sagi Grimberg
2021-10-26 8:43 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2021-10-25 20:35 UTC (permalink / raw)
To: Keith Busch; +Cc: linux-nvme, Christoph Hellwig, Chaitanya Kulkarni
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 78966b8ddb1e..42b58eb1ba62 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -926,15 +926,17 @@ static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
>> static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
>> {
>> struct nvme_tcp_queue *queue = req->queue;
>> + int req_data_len = req->data_len;
>>
>> while (true) {
>> struct page *page = nvme_tcp_req_cur_page(req);
>> size_t offset = nvme_tcp_req_cur_offset(req);
>> size_t len = nvme_tcp_req_cur_length(req);
>> - bool last = nvme_tcp_pdu_last_send(req, len);
>> + bool pdu_last = nvme_tcp_pdu_last_send(req, len);
>
> I'm not sure why you opted to change the name of this variable here,
It's designed to clarify that this flag is for the last send in the pdu
and not in the request (each request may consist of multiple pdus).
> but the patch is testing successfully: no observed r2t errors since apply
> this, so I think we're good to go for that regression.
>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again)
2021-10-25 14:38 ` Keith Busch
2021-10-25 20:35 ` Sagi Grimberg
@ 2021-10-26 8:43 ` Christoph Hellwig
2021-10-26 13:24 ` Sagi Grimberg
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2021-10-26 8:43 UTC (permalink / raw)
To: Keith Busch
Cc: Sagi Grimberg, linux-nvme, Christoph Hellwig, Chaitanya Kulkarni
On Mon, Oct 25, 2021 at 07:38:38AM -0700, Keith Busch wrote:
> I'm not sure why you opted to change the name of this variable here, but
> the patch is testing successfully: no observed r2t errors since apply
> this, so I think we're good to go for that regression.
I've dropped the variable rename and applied the patch to nvme-5.15,
thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again)
2021-10-26 8:43 ` Christoph Hellwig
@ 2021-10-26 13:24 ` Sagi Grimberg
0 siblings, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2021-10-26 13:24 UTC (permalink / raw)
To: Christoph Hellwig, Keith Busch; +Cc: linux-nvme, Chaitanya Kulkarni
>> I'm not sure why you opted to change the name of this variable here, but
>> the patch is testing successfully: no observed r2t errors since apply
>> this, so I think we're good to go for that regression.
>
> I've dropped the variable rename and applied the patch to nvme-5.15,
> thanks.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-26 13:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-24 7:43 [PATCH] nvme-tcp: Fix H2CData PDU send accounting (again) Sagi Grimberg
2021-10-25 7:18 ` Sagi Grimberg
2021-10-25 14:38 ` Keith Busch
2021-10-25 20:35 ` Sagi Grimberg
2021-10-26 8:43 ` Christoph Hellwig
2021-10-26 13:24 ` Sagi Grimberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.