Thread (80 messages) 80 messages, 5 authors, 2020-03-31

Re: [PATCH 06/29] nvme-fcloop: Fix deallocation of working context

From: Sagi Grimberg <sagi@grimberg.me>
Date: 2020-02-28 20:43:33

Is this directly related to the series? Or should
this be a dedicated bug fix (that in turn will go to
stable etc)?

On 2/5/20 10:37 AM, James Smart wrote:
quoted hunk ↗ jump to hunk
There's been a longstanding bug of LS completions which freed ls
op's, particularly the disconnect LS, while executing on a work
context that is in the memory being free. Not a good thing to do.

Rework LS handling to make callbacks in the rport context
rather than the ls_request context.

Signed-off-by: James Smart <redacted>
---
  drivers/nvme/target/fcloop.c | 76 ++++++++++++++++++++++++++++++--------------
  1 file changed, 52 insertions(+), 24 deletions(-)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 130932a5db0c..6533f4196005 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -198,10 +198,13 @@ struct fcloop_lport_priv {
  };
  
  struct fcloop_rport {
-	struct nvme_fc_remote_port *remoteport;
-	struct nvmet_fc_target_port *targetport;
-	struct fcloop_nport *nport;
-	struct fcloop_lport *lport;
+	struct nvme_fc_remote_port	*remoteport;
+	struct nvmet_fc_target_port	*targetport;
+	struct fcloop_nport		*nport;
+	struct fcloop_lport		*lport;
+	spinlock_t			lock;
+	struct list_head		ls_list;
+	struct work_struct		ls_work;
  };
  
  struct fcloop_tport {
@@ -224,11 +227,10 @@ struct fcloop_nport {
  };
  
  struct fcloop_lsreq {
-	struct fcloop_tport		*tport;
  	struct nvmefc_ls_req		*lsreq;
-	struct work_struct		work;
  	struct nvmefc_ls_rsp		ls_rsp;
  	int				status;
+	struct list_head		ls_list; /* fcloop_rport->ls_list */
  };
  
  struct fcloop_rscn {
@@ -292,21 +294,32 @@ fcloop_delete_queue(struct nvme_fc_local_port *localport,
  {
  }
  
-
-/*
- * Transmit of LS RSP done (e.g. buffers all set). call back up
- * initiator "done" flows.
- */
  static void
-fcloop_tgt_lsrqst_done_work(struct work_struct *work)
+fcloop_rport_lsrqst_work(struct work_struct *work)
  {
-	struct fcloop_lsreq *tls_req =
-		container_of(work, struct fcloop_lsreq, work);
-	struct fcloop_tport *tport = tls_req->tport;
-	struct nvmefc_ls_req *lsreq = tls_req->lsreq;
+	struct fcloop_rport *rport =
+		container_of(work, struct fcloop_rport, ls_work);
+	struct fcloop_lsreq *tls_req;
  
-	if (!tport || tport->remoteport)
-		lsreq->done(lsreq, tls_req->status);
+	spin_lock(&rport->lock);
+	for (;;) {
+		tls_req = list_first_entry_or_null(&rport->ls_list,
+				struct fcloop_lsreq, ls_list);
+		if (!tls_req)
+			break;
+
+		list_del(&tls_req->ls_list);
+		spin_unlock(&rport->lock);
+
+		tls_req->lsreq->done(tls_req->lsreq, tls_req->status);
+		/*
+		 * callee may free memory containing tls_req.
+		 * do not reference lsreq after this.
+		 */
+
+		spin_lock(&rport->lock);
+	}
+	spin_unlock(&rport->lock);
Won't it be easier to splice to a local list instead?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help