* [PATCH] nvmet: nvmet_async_events_process always free async event to avoid memleak
@ 2020-05-13 15:08 David Milburn
2020-05-14 15:05 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: David Milburn @ 2020-05-13 15:08 UTC (permalink / raw)
To: linux-nvme; +Cc: hch, chaitanya.kulkarni, dwagner
nvmet_async_event may not be freed yet while processing events.
$ cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff888c1af2c000 (size 32):
comm "nvmetcli", pid 5164, jiffies 4295220864 (age 6829.924s)
hex dump (first 32 bytes):
28 01 82 3b 8b 88 ff ff 28 01 82 3b 8b 88 ff ff (..;....(..;....
02 00 04 65 76 65 6e 74 5f 66 69 6c 65 00 00 00 ...event_file...
backtrace:
[<00000000217ae580>] nvmet_add_async_event+0x57/0x290 [nvmet]
[<0000000012aa2ea9>] nvmet_ns_changed+0x206/0x300 [nvmet]
[<00000000bb3fd52e>] nvmet_ns_disable+0x367/0x4f0 [nvmet]
[<00000000e91ca9ec>] nvmet_ns_free+0x15/0x180 [nvmet]
[<00000000a15deb52>] config_item_release+0xf1/0x1c0
[<000000007e148432>] configfs_rmdir+0x555/0x7c0
[<00000000f4506ea6>] vfs_rmdir+0x142/0x3c0
[<0000000000acaaf0>] do_rmdir+0x2b2/0x340
[<0000000034d1aa52>] do_syscall_64+0xa5/0x4d0
[<00000000211f13bc>] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
Steps to Reproduce:
target:
1. nvmetcli restore rdma.json
client:
2. nvme connect -t rdma -a $IP -s 4420 -n testnqn
target:
3. nvmetcli clear
4. sleep 5 && nvmetcli restore rdma.json
5. cat /sys/kernel/debug/kmemleak after 5 minutes
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: David Milburn <dmilburn@redhat.com>
---
drivers/nvme/target/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b685f99d56a1..8b78f42c1eb6 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -139,6 +139,7 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
aen = list_first_entry_or_null(&ctrl->async_events,
struct nvmet_async_event, entry);
if (!aen || !ctrl->nr_async_event_cmds) {
+ kfree(aen);
mutex_unlock(&ctrl->lock);
break;
}
--
2.18.1
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nvmet: nvmet_async_events_process always free async event to avoid memleak
2020-05-13 15:08 [PATCH] nvmet: nvmet_async_events_process always free async event to avoid memleak David Milburn
@ 2020-05-14 15:05 ` Christoph Hellwig
2020-05-14 15:27 ` David Milburn
2020-05-18 13:03 ` David Milburn
0 siblings, 2 replies; 4+ messages in thread
From: Christoph Hellwig @ 2020-05-14 15:05 UTC (permalink / raw)
To: David Milburn; +Cc: hch, dwagner, chaitanya.kulkarni, linux-nvme
I don't think we should take the aen off the list if there is no
command slot available. So probably something like this instead:
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b685f99d56a1f..0df9300d717d6 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -134,15 +134,10 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
struct nvmet_async_event *aen;
struct nvmet_req *req;
- while (1) {
- mutex_lock(&ctrl->lock);
- aen = list_first_entry_or_null(&ctrl->async_events,
+ mutex_lock(&ctrl->lock);
+ while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
+ aen = list_first_entry(&ctrl->async_events,
struct nvmet_async_event, entry);
- if (!aen || !ctrl->nr_async_event_cmds) {
- mutex_unlock(&ctrl->lock);
- break;
- }
-
req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
if (status == 0)
nvmet_set_result(req, nvmet_async_event_result(aen));
@@ -152,7 +147,9 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
mutex_unlock(&ctrl->lock);
nvmet_req_complete(req, status);
+ mutex_lock(&ctrl->lock);
}
+ mutex_unlock(&ctrl->lock);
}
static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nvmet: nvmet_async_events_process always free async event to avoid memleak
2020-05-14 15:05 ` Christoph Hellwig
@ 2020-05-14 15:27 ` David Milburn
2020-05-18 13:03 ` David Milburn
1 sibling, 0 replies; 4+ messages in thread
From: David Milburn @ 2020-05-14 15:27 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-nvme, chaitanya.kulkarni, dwagner
Hi Christoph,
On 05/14/2020 10:05 AM, Christoph Hellwig wrote:
> I don't think we should take the aen off the list if there is no
> command slot available. So probably something like this instead:
Ok, we will retest.
Thanks,
David
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index b685f99d56a1f..0df9300d717d6 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -134,15 +134,10 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
> struct nvmet_async_event *aen;
> struct nvmet_req *req;
>
> - while (1) {
> - mutex_lock(&ctrl->lock);
> - aen = list_first_entry_or_null(&ctrl->async_events,
> + mutex_lock(&ctrl->lock);
> + while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
> + aen = list_first_entry(&ctrl->async_events,
> struct nvmet_async_event, entry);
> - if (!aen || !ctrl->nr_async_event_cmds) {
> - mutex_unlock(&ctrl->lock);
> - break;
> - }
> -
> req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
> if (status == 0)
> nvmet_set_result(req, nvmet_async_event_result(aen));
> @@ -152,7 +147,9 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
>
> mutex_unlock(&ctrl->lock);
> nvmet_req_complete(req, status);
> + mutex_lock(&ctrl->lock);
> }
> + mutex_unlock(&ctrl->lock);
> }
>
> static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
>
> _______________________________________________
> linux-nvme mailing list
> linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nvmet: nvmet_async_events_process always free async event to avoid memleak
2020-05-14 15:05 ` Christoph Hellwig
2020-05-14 15:27 ` David Milburn
@ 2020-05-18 13:03 ` David Milburn
1 sibling, 0 replies; 4+ messages in thread
From: David Milburn @ 2020-05-18 13:03 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: dwagner, chaitanya.kulkarni, linux-nvme
Hi Christoph,
On 05/14/2020 10:05 AM, Christoph Hellwig wrote:
> I don't think we should take the aen off the list if there is no
> command slot available. So probably something like this instead:
This still leaks:
unreferenced object 0xffff888b9635c4c0 (size 32):
comm "nvmetcli", pid 4962, jiffies 4294954934 (age 658.257s)
hex dump (first 32 bytes):
28 81 f1 81 8b 88 ff ff 28 81 f1 81 8b 88 ff ff (.......(.......
02 00 04 00 00 00 00 00 c0 12 e4 42 8c 55 00 00 ...........B.U..
backtrace:
[<0000000057d91381>] nvmet_add_async_event+0x57/0x290 [nvmet]
[<0000000097269b44>] nvmet_ns_changed+0x206/0x300 [nvmet]
[<00000000ea63c8b2>] nvmet_ns_disable+0x367/0x4f0 [nvmet]
[<00000000a65f5ee4>] nvmet_ns_free+0x15/0x180 [nvmet]
[<0000000071fcc228>] config_item_release+0xf1/0x1c0
[<000000006ba88943>] configfs_rmdir+0x555/0x7c0
[<00000000118db5e1>] vfs_rmdir+0x142/0x3c0
[<00000000f75e2a56>] do_rmdir+0x2b2/0x340
[<00000000b315b784>] do_syscall_64+0xa5/0x4d0
[<000000002dcd7e5e>] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
But, if we do the same nvmet_async_events_free() it will pass Yi's
test, I sent a v2.
Thanks,
David
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index b685f99d56a1f..0df9300d717d6 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -134,15 +134,10 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
> struct nvmet_async_event *aen;
> struct nvmet_req *req;
>
> - while (1) {
> - mutex_lock(&ctrl->lock);
> - aen = list_first_entry_or_null(&ctrl->async_events,
> + mutex_lock(&ctrl->lock);
> + while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
> + aen = list_first_entry(&ctrl->async_events,
> struct nvmet_async_event, entry);
> - if (!aen || !ctrl->nr_async_event_cmds) {
> - mutex_unlock(&ctrl->lock);
> - break;
> - }
> -
> req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
> if (status == 0)
> nvmet_set_result(req, nvmet_async_event_result(aen));
> @@ -152,7 +147,9 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
>
> mutex_unlock(&ctrl->lock);
> nvmet_req_complete(req, status);
> + mutex_lock(&ctrl->lock);
> }
> + mutex_unlock(&ctrl->lock);
> }
>
> static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
>
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-18 13:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 15:08 [PATCH] nvmet: nvmet_async_events_process always free async event to avoid memleak David Milburn
2020-05-14 15:05 ` Christoph Hellwig
2020-05-14 15:27 ` David Milburn
2020-05-18 13:03 ` David Milburn
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.