[PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[elad.grupi]

From: Elad Grupi <elad.grupi@dell.com>

Some nvme commands might get completed during the execution of
nvmet_tcp_release_queue_work. Those commands are being added to
resp_list on the queue and never processed after cancel_work_sync
is done, causing memory leak of the resources allocated for those commands.

Signed-off-by: Elad Grupi <elad.grupi@dell.com>
---
 drivers/nvme/target/tcp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index f9f34f6caf5e..3c6e36c6ed4a 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1426,6 +1426,33 @@ static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue)
 	}
 }
 
+static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue)
+{
+	struct list_head *p;
+	struct nvmet_tcp_cmd *cmd;
+	int c = 0;
+
+	nvmet_tcp_process_resp_list(queue);
+
+	list_for_each(p, &queue->resp_send_list) {
+		cmd = list_entry(p, struct nvmet_tcp_cmd, entry);
+		kfree(cmd->iov);
+		sgl_free(cmd->req.sg);
+		c++;
+	}
+
+	WARN_ON(c != queue->send_list_len);
+
+	if (queue->snd_cmd) {
+		kfree(queue->snd_cmd->iov);
+		sgl_free(queue->snd_cmd->req.sg);
+		c++;
+	}
+
+	pr_debug("qid %u send_list_len %d, free %d unprocessed commands\n",
+			queue->nvme_sq.qid, queue->send_list_len, c);
+}
+
 static void nvmet_tcp_release_queue_work(struct work_struct *w)
 {
 	struct nvmet_tcp_queue *queue =
@@ -1441,6 +1468,7 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
 	nvmet_tcp_uninit_data_in_cmds(queue);
 	nvmet_sq_destroy(&queue->nvme_sq);
 	cancel_work_sync(&queue->io_work);
+	nvmet_tcp_free_resp_list(queue);
 	sock_release(queue->sock);
 	nvmet_tcp_free_cmds(queue);
 	if (queue->hdr_digest || queue->data_digest)
-- 
2.18.2


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

Re: [PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[Sagi Grimberg]

> +static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue)
> +{
> +	struct list_head *p;
> +	struct nvmet_tcp_cmd *cmd;
> +	int c = 0;
> +
> +	nvmet_tcp_process_resp_list(queue);
> +
> +	list_for_each(p, &queue->resp_send_list) {
> +		cmd = list_entry(p, struct nvmet_tcp_cmd, entry);
> +		kfree(cmd->iov);

PDUs need to be unmapped as well in case of writes. That is not
handled in nvmet_tcp_uninit_data_in_cmds ? Or is it just reads (data-out
commands)?

> +		sgl_free(cmd->req.sg);
> +		c++;
> +	}
> +
> +	WARN_ON(c != queue->send_list_len);
> +
> +	if (queue->snd_cmd) {
> +		kfree(queue->snd_cmd->iov);
> +		sgl_free(queue->snd_cmd->req.sg);
> +		c++;
> +	}
> +
> +	pr_debug("qid %u send_list_len %d, free %d unprocessed commands\n",
> +			queue->nvme_sq.qid, queue->send_list_len, c);
> +}
> +
>   static void nvmet_tcp_release_queue_work(struct work_struct *w)
>   {
>   	struct nvmet_tcp_queue *queue =
> @@ -1441,6 +1468,7 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
>   	nvmet_tcp_uninit_data_in_cmds(queue);
>   	nvmet_sq_destroy(&queue->nvme_sq);
>   	cancel_work_sync(&queue->io_work);
> +	nvmet_tcp_free_resp_list(queue);
>   	sock_release(queue->sock);
>   	nvmet_tcp_free_cmds(queue);
>   	if (queue->hdr_digest || queue->data_digest)
> 

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

Re: [PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[Christoph Hellwig]

On Mon, May 24, 2021 at 02:24:41PM +0300, elad.grupi@dell.com wrote:
>  }
>  
> +static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue)
> +{
> +	struct list_head *p;
> +	struct nvmet_tcp_cmd *cmd;
> +	int c = 0;
> +
> +	nvmet_tcp_process_resp_list(queue);
> +
> +	list_for_each(p, &queue->resp_send_list) {
> +		cmd = list_entry(p, struct nvmet_tcp_cmd, entry);

Please use list_for_each_entry instead.

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

RE: [PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[Grupi, Elad]

Sure. I will prepare an update to the patch today.

Thanks,
Elad

-----Original Message-----
From: Christoph Hellwig <hch@infradead.org> 
Sent: Wednesday, 26 May 2021 17:05
To: Grupi, Elad
Cc: sagi@grimberg.me; linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect


[EXTERNAL EMAIL] 

On Mon, May 24, 2021 at 02:24:41PM +0300, elad.grupi@dell.com wrote:
>  }
>  
> +static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue) {
> +	struct list_head *p;
> +	struct nvmet_tcp_cmd *cmd;
> +	int c = 0;
> +
> +	nvmet_tcp_process_resp_list(queue);
> +
> +	list_for_each(p, &queue->resp_send_list) {
> +		cmd = list_entry(p, struct nvmet_tcp_cmd, entry);

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

Re: [PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[Sagi Grimberg]

> From: Elad Grupi <elad.grupi@dell.com>
> 
> Some nvme commands might get completed during the execution of
> nvmet_tcp_release_queue_work. Those commands are being added to
> resp_list on the queue and never processed after cancel_work_sync
> is done, causing memory leak of the resources allocated for those commands.
> 
> Signed-off-by: Elad Grupi <elad.grupi@dell.com>
> ---
>   drivers/nvme/target/tcp.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index f9f34f6caf5e..04d53b532ae2 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -1426,6 +1426,31 @@ static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue)
>   	}
>   }
>   
> +static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue)
> +{
> +	struct nvmet_tcp_cmd *cmd;
> +	int c = 0;
> +
> +	nvmet_tcp_process_resp_list(queue);
> +
> +	list_for_each_entry(cmd, &queue->resp_send_list, entry) {
> +		kfree(cmd->iov);
> +		sgl_free(cmd->req.sg);
> +		c++;
> +	}
> +
> +	WARN_ON(c != queue->send_list_len);
> +
> +	if (queue->snd_cmd) {
> +		kfree(queue->snd_cmd->iov);
> +		sgl_free(queue->snd_cmd->req.sg);
> +		c++;
> +	}
> +
> +	pr_debug("qid %u send_list_len %d, free %d unprocessed commands\n",
> +			queue->nvme_sq.qid, queue->send_list_len, c);
> +}
> +
>   static void nvmet_tcp_release_queue_work(struct work_struct *w)
>   {
>   	struct nvmet_tcp_queue *queue =
> @@ -1441,6 +1466,7 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
>   	nvmet_tcp_uninit_data_in_cmds(queue);
>   	nvmet_sq_destroy(&queue->nvme_sq);
>   	cancel_work_sync(&queue->io_work);
> +	nvmet_tcp_free_resp_list(queue);

Can't this be combined with nvme_tcp_free_cmds? Maybe consolidate
the sgl and iov free to something like nvme_tcp_free_cmd_data that
will free these and make it null-free-safe?

>   	sock_release(queue->sock);
>   	nvmet_tcp_free_cmds(queue);
>   	if (queue->hdr_digest || queue->data_digest)
> 

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

[PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[elad.grupi]

From: Elad Grupi <elad.grupi@dell.com>

Some nvme commands might get completed during the execution of
nvmet_tcp_release_queue_work. Those commands are being added to
resp_list on the queue and never processed after cancel_work_sync
is done, causing memory leak of the resources allocated for those commands.

Signed-off-by: Elad Grupi <elad.grupi@dell.com>
---
 drivers/nvme/target/tcp.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index f9f34f6caf5e..04d53b532ae2 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1426,6 +1426,31 @@ static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue)
 	}
 }
 
+static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue)
+{
+	struct nvmet_tcp_cmd *cmd;
+	int c = 0;
+
+	nvmet_tcp_process_resp_list(queue);
+
+	list_for_each_entry(cmd, &queue->resp_send_list, entry) {
+		kfree(cmd->iov);
+		sgl_free(cmd->req.sg);
+		c++;
+	}
+
+	WARN_ON(c != queue->send_list_len);
+
+	if (queue->snd_cmd) {
+		kfree(queue->snd_cmd->iov);
+		sgl_free(queue->snd_cmd->req.sg);
+		c++;
+	}
+
+	pr_debug("qid %u send_list_len %d, free %d unprocessed commands\n",
+			queue->nvme_sq.qid, queue->send_list_len, c);
+}
+
 static void nvmet_tcp_release_queue_work(struct work_struct *w)
 {
 	struct nvmet_tcp_queue *queue =
@@ -1441,6 +1466,7 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
 	nvmet_tcp_uninit_data_in_cmds(queue);
 	nvmet_sq_destroy(&queue->nvme_sq);
 	cancel_work_sync(&queue->io_work);
+	nvmet_tcp_free_resp_list(queue);
 	sock_release(queue->sock);
 	nvmet_tcp_free_cmds(queue);
 	if (queue->hdr_digest || queue->data_digest)
-- 
2.18.2


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

[PATCH] nvmet-tcp: fix memory leak when having inflight commands on disconnect

[elad.grupi]

From: Elad Grupi <elad.grupi@dell.com>

Some nvme commands might get completed during the execution of
nvmet_tcp_release_queue_work. Those commands are being added to
resp_list on the queue and never processed after cancel_work_sync
is done, causing memory leak of the resources allocated for those commands.

Signed-off-by: Elad Grupi <elad.grupi@dell.com>
---
 drivers/nvme/target/tcp.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index f9f34f6caf5e..04d53b532ae2 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1426,6 +1426,31 @@ static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue)
 	}
 }
 
+static void nvmet_tcp_free_resp_list(struct nvmet_tcp_queue *queue)
+{
+	struct nvmet_tcp_cmd *cmd;
+	int c = 0;
+
+	nvmet_tcp_process_resp_list(queue);
+
+	list_for_each_entry(cmd, &queue->resp_send_list, entry) {
+		kfree(cmd->iov);
+		sgl_free(cmd->req.sg);
+		c++;
+	}
+
+	WARN_ON(c != queue->send_list_len);
+
+	if (queue->snd_cmd) {
+		kfree(queue->snd_cmd->iov);
+		sgl_free(queue->snd_cmd->req.sg);
+		c++;
+	}
+
+	pr_debug("qid %u send_list_len %d, free %d unprocessed commands\n",
+			queue->nvme_sq.qid, queue->send_list_len, c);
+}
+
 static void nvmet_tcp_release_queue_work(struct work_struct *w)
 {
 	struct nvmet_tcp_queue *queue =
@@ -1441,6 +1466,7 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
 	nvmet_tcp_uninit_data_in_cmds(queue);
 	nvmet_sq_destroy(&queue->nvme_sq);
 	cancel_work_sync(&queue->io_work);
+	nvmet_tcp_free_resp_list(queue);
 	sock_release(queue->sock);
 	nvmet_tcp_free_cmds(queue);
 	if (queue->hdr_digest || queue->data_digest)
-- 
2.18.2


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