* [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller
@ 2024-04-28 8:49 Sagi Grimberg
2024-04-29 5:13 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Sagi Grimberg @ 2024-04-28 8:49 UTC (permalink / raw
To: linux-nvme; +Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Yi Zhang
When we teardown the controller, we wait for pending I/Os to complete
(sq->ref on all queues to drop to zero) and then we go over the commands,
and free their command buffers in case they are still fetching data from
the host (e.g. processing nvme writes) and have yet to take a reference
on the sq.
However, we may miss the case where commands have failed before executing
and are queued for sending a response, but will never occur because the
queue socket is already down. In this case we may miss deallocating command
buffers.
Solve this by freeing all commands buffers as nvmet_tcp_free_cmd_buffers is
idempotent anyways.
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Tested-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/target/tcp.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index a5422e2c979a..380f22ee3ebb 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -348,6 +348,7 @@ static int nvmet_tcp_check_ddgst(struct nvmet_tcp_queue *queue, void *pdu)
return 0;
}
+/* If cmd buffers are NULL, no operation is performed */
static void nvmet_tcp_free_cmd_buffers(struct nvmet_tcp_cmd *cmd)
{
kfree(cmd->iov);
@@ -1581,13 +1582,9 @@ static void nvmet_tcp_free_cmd_data_in_buffers(struct nvmet_tcp_queue *queue)
struct nvmet_tcp_cmd *cmd = queue->cmds;
int i;
- for (i = 0; i < queue->nr_cmds; i++, cmd++) {
- if (nvmet_tcp_need_data_in(cmd))
- nvmet_tcp_free_cmd_buffers(cmd);
- }
-
- if (!queue->nr_cmds && nvmet_tcp_need_data_in(&queue->connect))
- nvmet_tcp_free_cmd_buffers(&queue->connect);
+ for (i = 0; i < queue->nr_cmds; i++, cmd++)
+ nvmet_tcp_free_cmd_buffers(cmd);
+ nvmet_tcp_free_cmd_buffers(&queue->connect);
}
static void nvmet_tcp_release_queue_work(struct work_struct *w)
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller
2024-04-28 8:49 [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller Sagi Grimberg
@ 2024-04-29 5:13 ` Christoph Hellwig
2024-04-29 8:51 ` Keith Busch
2024-05-23 9:42 ` Hannes Reinecke
2 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2024-04-29 5:13 UTC (permalink / raw
To: Sagi Grimberg
Cc: linux-nvme, Christoph Hellwig, Keith Busch, Chaitanya Kulkarni,
Yi Zhang
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller
2024-04-28 8:49 [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller Sagi Grimberg
2024-04-29 5:13 ` Christoph Hellwig
@ 2024-04-29 8:51 ` Keith Busch
2024-05-23 9:42 ` Hannes Reinecke
2 siblings, 0 replies; 4+ messages in thread
From: Keith Busch @ 2024-04-29 8:51 UTC (permalink / raw
To: Sagi Grimberg; +Cc: linux-nvme, Christoph Hellwig, Chaitanya Kulkarni, Yi Zhang
On Sun, Apr 28, 2024 at 11:49:49AM +0300, Sagi Grimberg wrote:
> When we teardown the controller, we wait for pending I/Os to complete
> (sq->ref on all queues to drop to zero) and then we go over the commands,
> and free their command buffers in case they are still fetching data from
> the host (e.g. processing nvme writes) and have yet to take a reference
> on the sq.
>
> However, we may miss the case where commands have failed before executing
> and are queued for sending a response, but will never occur because the
> queue socket is already down. In this case we may miss deallocating command
> buffers.
>
> Solve this by freeing all commands buffers as nvmet_tcp_free_cmd_buffers is
> idempotent anyways.
>
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Tested-by: Yi Zhang <yi.zhang@redhat.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
Thanks, applied to nvme-6.9.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller
2024-04-28 8:49 [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller Sagi Grimberg
2024-04-29 5:13 ` Christoph Hellwig
2024-04-29 8:51 ` Keith Busch
@ 2024-05-23 9:42 ` Hannes Reinecke
2 siblings, 0 replies; 4+ messages in thread
From: Hannes Reinecke @ 2024-05-23 9:42 UTC (permalink / raw
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Yi Zhang
On 4/28/24 10:49, Sagi Grimberg wrote:
> When we teardown the controller, we wait for pending I/Os to complete
> (sq->ref on all queues to drop to zero) and then we go over the commands,
> and free their command buffers in case they are still fetching data from
> the host (e.g. processing nvme writes) and have yet to take a reference
> on the sq.
>
> However, we may miss the case where commands have failed before executing
> and are queued for sending a response, but will never occur because the
> queue socket is already down. In this case we may miss deallocating command
> buffers.
>
> Solve this by freeing all commands buffers as nvmet_tcp_free_cmd_buffers is
> idempotent anyways.
>
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Tested-by: Yi Zhang <yi.zhang@redhat.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/target/tcp.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-23 9:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-28 8:49 [PATCH] nvmet-tcp: fix possible memory leak when tearing down a controller Sagi Grimberg
2024-04-29 5:13 ` Christoph Hellwig
2024-04-29 8:51 ` Keith Busch
2024-05-23 9:42 ` Hannes Reinecke
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.