Re: [PATCH v2 2/2] nvmet: avoid memleak by freeing any remaining aens in nvmet_async_events_free

[Christoph Hellwig <hch@infradead.org>]

On Wed, May 20, 2020 at 05:39:18AM -0500, David Milburn wrote:
> Yi was able to reproduce the memleak using the v2 of the patch series
> since nvmet_async_events_free() ran before nvmet_add_async_event().
> 
> http://lists.infradead.org/pipermail/linux-nvme/2020-May/030512.html
> 
> With Sagi's patch below, I do see after nvmet_add_async_event(),
> nvmet_async_events_process pulls the request, decrements
> ctrl->nr_async_event_cmds to 0, and frees the aen,
> and no memory leak.
> 
> http://lists.infradead.org/pipermail/linux-nvme/2020-May/030548.html

Ok, let's try a version of Sagis latest suggestion then.  What about
this (replacement for this patch only, I applied the first one already):

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 54679260e6648..1525426d0a31f 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -129,8 +129,10 @@ static u32 nvmet_async_event_result(struct nvmet_async_event *aen)
 	return aen->event_type | (aen->event_info << 8) | (aen->log_page << 16);
 }
 
-static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
+static void nvmet_async_event_work(struct work_struct *work)
 {
+	struct nvmet_ctrl *ctrl =
+		container_of(work, struct nvmet_ctrl, async_event_work);
 	struct nvmet_async_event *aen;
 	struct nvmet_req *req;
 
@@ -139,21 +141,20 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
 		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));
+		nvmet_set_result(req, nvmet_async_event_result(aen));
 
 		list_del(&aen->entry);
 		kfree(aen);
 
 		mutex_unlock(&ctrl->lock);
 		trace_nvmet_async_event(ctrl, req->cqe->result.u32);
-		nvmet_req_complete(req, status);
+		nvmet_req_complete(req, 0);
 		mutex_lock(&ctrl->lock);
 	}
 	mutex_unlock(&ctrl->lock);
 }
 
-static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
+static void nvmet_async_events_fail_all(struct nvmet_ctrl *ctrl)
 {
 	struct nvmet_req *req;
 
@@ -167,12 +168,14 @@ static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
 	mutex_unlock(&ctrl->lock);
 }
 
-static void nvmet_async_event_work(struct work_struct *work)
+static void nvmet_async_events_free(struct nvmet_ctrl *ctrl)
 {
-	struct nvmet_ctrl *ctrl =
-		container_of(work, struct nvmet_ctrl, async_event_work);
+	struct nvmet_async_event *aen, *n;
 
-	nvmet_async_events_process(ctrl, 0);
+	list_for_each_entry_safe(aen, n, &ctrl->async_events, entry) {
+		list_del(&aen->entry);
+		kfree(aen);
+	}
 }
 
 void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
@@ -768,17 +771,14 @@ static void nvmet_confirm_sq(struct percpu_ref *ref)
 
 void nvmet_sq_destroy(struct nvmet_sq *sq)
 {
-	u16 status = NVME_SC_INTERNAL | NVME_SC_DNR;
 	struct nvmet_ctrl *ctrl = sq->ctrl;
 
 	/*
 	 * If this is the admin queue, complete all AERs so that our
 	 * queue doesn't have outstanding requests on it.
 	 */
-	if (ctrl && ctrl->sqs && ctrl->sqs[0] == sq) {
-		nvmet_async_events_process(ctrl, status);
-		nvmet_async_events_free(ctrl);
-	}
+	if (ctrl && ctrl->sqs && ctrl->sqs[0] == sq)
+		nvmet_async_events_fail_all(ctrl);
 	percpu_ref_kill_and_confirm(&sq->ref, nvmet_confirm_sq);
 	wait_for_completion(&sq->confirm_done);
 	wait_for_completion(&sq->free_done);
@@ -1368,6 +1368,7 @@ static void nvmet_ctrl_free(struct kref *ref)
 
 	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 
+	nvmet_async_events_free(ctrl);
 	kfree(ctrl->sqs);
 	kfree(ctrl->cqs);
 	kfree(ctrl->changed_ns_list);

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