[PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[David Milburn]

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>
Based-on-a-patch-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: David Milburn <dmilburn@redhatcom>
---
Changes from v1:
 - Check async event list before freeing aen.

 drivers/nvme/target/core.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b685f99d56a1..c9d74ebeaa7d 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,
-				struct nvmet_async_event, entry);
-		if (!aen || !ctrl->nr_async_event_cmds) {
-			mutex_unlock(&ctrl->lock);
-			break;
-		}
-
+	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);
 		req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
 		if (status == 0)
 			nvmet_set_result(req, nvmet_async_event_result(aen));
@@ -152,15 +147,23 @@ 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)
 {
+	struct nvmet_async_event *aen;
 	struct nvmet_req *req;
 
 	mutex_lock(&ctrl->lock);
-	while (ctrl->nr_async_event_cmds) {
+	while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
+		aen = list_first_entry(&ctrl->async_events,
+				       struct nvmet_async_event, entry);
+		list_del(&aen->entry);
+		kfree(aen);
+	  
 		req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
 		mutex_unlock(&ctrl->lock);
 		nvmet_req_complete(req, NVME_SC_INTERNAL | NVME_SC_DNR);
-- 
2.18.1


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

Re: [PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[Daniel Wagner]

Hi David,

On Mon, May 18, 2020 at 08:01:58AM -0500, David Milburn wrote:
>  static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
>  {
> +	struct nvmet_async_event *aen;
>  	struct nvmet_req *req;
>  
>  	mutex_lock(&ctrl->lock);
> -	while (ctrl->nr_async_event_cmds) {
> +	while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
> +		aen = list_first_entry(&ctrl->async_events,
> +				       struct nvmet_async_event, entry);
> +		list_del(&aen->entry);
> +		kfree(aen);
> +

nvmet_async_event_free() is called from nvmet_sq_destroy(). I thought
this function should free all resources. Wouldn't this leak some memory
if the condition gets false?

Thanks,
Daniel

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

Re: [PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[David Milburn]

Hi Daniel,

On 05/18/2020 08:07 AM, Daniel Wagner wrote:
> Hi David,
> 
> On Mon, May 18, 2020 at 08:01:58AM -0500, David Milburn wrote:
>>   static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
>>   {
>> +	struct nvmet_async_event *aen;
>>   	struct nvmet_req *req;
>>   
>>   	mutex_lock(&ctrl->lock);
>> -	while (ctrl->nr_async_event_cmds) {
>> +	while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
>> +		aen = list_first_entry(&ctrl->async_events,
>> +				       struct nvmet_async_event, entry);
>> +		list_del(&aen->entry);
>> +		kfree(aen);
>> +
> 
> nvmet_async_event_free() is called from nvmet_sq_destroy(). I thought
> this function should free all resources. Wouldn't this leak some memory
> if the condition gets false?
> 

It doesn't look like you want to process the nvmet_req if
ctrl->nr_async_event_cmds is 0, so making the test a ||
may end up bad. It looks like nvmet_add_async_event grabs the
ctrl->lock when adding an aen to the list, and it looks like
this maybe happening when ctrl->lock is let go before a
nvmet_req_complete().

Thanks,
David


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

Re: [PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[Daniel Wagner]

Hi David,

> > > -	while (ctrl->nr_async_event_cmds) {
> > > +	while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
> > > +		aen = list_first_entry(&ctrl->async_events,
> > > +				       struct nvmet_async_event, entry);
> > > +		list_del(&aen->entry);
> > > +		kfree(aen);
> > > +
> > 
> > nvmet_async_event_free() is called from nvmet_sq_destroy(). I thought
> > this function should free all resources. Wouldn't this leak some memory
> > if the condition gets false?
> > 
> 
> It doesn't look like you want to process the nvmet_req if
> ctrl->nr_async_event_cmds is 0, so making the test a ||
> may end up bad. It looks like nvmet_add_async_event grabs the
> ctrl->lock when adding an aen to the list, and it looks like
> this maybe happening when ctrl->lock is let go before a
> nvmet_req_complete().

I admit, I don't really grasp all the details. My concern is that either
nr_async_event_cmds == 0 and the list is empty or
nr_async_event_cmds != 0 and the list is not empty. If it is guaranteed
that those variables are always in sync your change is correct. A more
defensive way would be do have free the list and the array separately.

Thanks,
Daniel

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

Re: [PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[David Milburn]

Hi Daniel,

On 05/18/2020 09:21 AM, Daniel Wagner wrote:
> Hi David,
> 
>>>> -	while (ctrl->nr_async_event_cmds) {
>>>> +	while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
>>>> +		aen = list_first_entry(&ctrl->async_events,
>>>> +				       struct nvmet_async_event, entry);
>>>> +		list_del(&aen->entry);
>>>> +		kfree(aen);
>>>> +
>>>
>>> nvmet_async_event_free() is called from nvmet_sq_destroy(). I thought
>>> this function should free all resources. Wouldn't this leak some memory
>>> if the condition gets false?
>>>
>>
>> It doesn't look like you want to process the nvmet_req if
>> ctrl->nr_async_event_cmds is 0, so making the test a ||
>> may end up bad. It looks like nvmet_add_async_event grabs the
>> ctrl->lock when adding an aen to the list, and it looks like
>> this maybe happening when ctrl->lock is let go before a
>> nvmet_req_complete().
> 
> I admit, I don't really grasp all the details. My concern is that either
> nr_async_event_cmds == 0 and the list is empty or
> nr_async_event_cmds != 0 and the list is not empty. If it is guaranteed
> that those variables are always in sync your change is correct. A more
> defensive way would be do have free the list and the array separately.
> 

It does look like there is still is a chance for the variables to be out
of sync, we will retest with the two separated out in nvmet_async_event_
free().

Thanks,
David


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

Re: [PATCH v2] nvmet: nvmet_async_events_process always free async event to avoid memleak

[Christoph Hellwig]

On Mon, May 18, 2020 at 03:07:33PM +0200, Daniel Wagner wrote:
> Hi David,
> 
> On Mon, May 18, 2020 at 08:01:58AM -0500, David Milburn wrote:
> >  static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
> >  {
> > +	struct nvmet_async_event *aen;
> >  	struct nvmet_req *req;
> >  
> >  	mutex_lock(&ctrl->lock);
> > -	while (ctrl->nr_async_event_cmds) {
> > +	while (ctrl->nr_async_event_cmds && !list_empty(&ctrl->async_events)) {
> > +		aen = list_first_entry(&ctrl->async_events,
> > +				       struct nvmet_async_event, entry);
> > +		list_del(&aen->entry);
> > +		kfree(aen);
> > +
> 
> nvmet_async_event_free() is called from nvmet_sq_destroy(). I thought
> this function should free all resources. Wouldn't this leak some memory
> if the condition gets false?

Yes, freeing resources is the job of nvmet_async_event_free.  But it
seems like it fails to look at the aens still on the ctrl->async_events
list.  So in addition to this patch, which is correct despite not
fixing the problem David ran into initially, we'll also need another
one to free all theaens left on ->async_events.

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