[PATCH 0/5] nvme-fc: misc small improvents

[PATCH 0/5] nvme-fc: misc small improvents

[Daniel Wagner]

I've collected a bunch of patches from the last debugging session. I think it's
worth documenting which conditions are expected in the cleanup path by adding a
bunch of WARNs.

Also found a deadlock in the nvme-fc code, so this one should definitly go in.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
Daniel Wagner (5):
      nvme-fc: don't hold rport lock when putting ctrl
      nvme-fc: check all request and response have been processed
      nvmet-fcloop: check all request and response have been processed
      nvmet-fcloop: remove unused lsdir member.
      nvmet-fc: use pr_* print macros instead of dev_*

 drivers/nvme/host/fc.c       |  4 ++++
 drivers/nvme/target/fc.c     | 48 +++++++++++++++++++-------------------------
 drivers/nvme/target/fcloop.c |  9 ++++++---
 3 files changed, 31 insertions(+), 30 deletions(-)
---
base-commit: 77a4fe6a06e265bd94d2b3cdc87fb3cde877a05b
change-id: 20251028-nvmet-fcloop-fixes-4e57e82f1f15

Best regards,
-- 
Daniel Wagner <wagi@kernel.org>

[PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Daniel Wagner]

nvme_fc_ctrl_put can acquire the rport lock when freeing the
ctrl object:

nvme_fc_ctrl_put
  nvme_fc_ctrl_free
    spin_lock_irqsave(rport->lock)

Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/host/fc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 03987f497a5b..2c0ea843ae57 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1488,7 +1488,9 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
 		if (ret)
 			/* leave the ctrl get reference */
 			break;
+		spin_unlock_irqrestore(&rport->lock, flags);
 		nvme_fc_ctrl_put(ctrl);
+		spin_lock_irqsave(&rport->lock, flags);
 	}
 
 	spin_unlock_irqrestore(&rport->lock, flags);

-- 
2.51.0

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Christoph Hellwig]

On Tue, Oct 28, 2025 at 04:26:20PM +0100, Daniel Wagner wrote:
> nvme_fc_ctrl_put can acquire the rport lock when freeing the
> ctrl object:
> 
> nvme_fc_ctrl_put
>   nvme_fc_ctrl_free
>     spin_lock_irqsave(rport->lock)
> 
> Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Hannes Reinecke]

On 10/28/25 16:26, Daniel Wagner wrote:
> nvme_fc_ctrl_put can acquire the rport lock when freeing the
> ctrl object:
> 
> nvme_fc_ctrl_put
>    nvme_fc_ctrl_free
>      spin_lock_irqsave(rport->lock)
> 
> Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/host/fc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 03987f497a5b..2c0ea843ae57 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1488,7 +1488,9 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>   		if (ret)
>   			/* leave the ctrl get reference */
>   			break;
> +		spin_unlock_irqrestore(&rport->lock, flags);
>   		nvme_fc_ctrl_put(ctrl);
> +		spin_lock_irqsave(&rport->lock, flags);
>   	}
>   
>   	spin_unlock_irqrestore(&rport->lock, flags);
> 
In theory, yes.
In practice we're getting a reference to the controller at the start
of the loop, so nvme_fc_ctrl_put() should just decrement the refcount.
But _if_ someone manages to sneak in an drop the reference while we are
within that loop then the entire logic falls apart as the controller
will be deleted and the next loop iteration will trip over an
uninitialized pointer.
So you would need to modify the loop to use
loop_for_each_entry_safe() to avoid this, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Daniel Wagner]

On Tue, Nov 04, 2025 at 11:51:09AM +0100, Hannes Reinecke wrote:
> On 10/28/25 16:26, Daniel Wagner wrote:
> > nvme_fc_ctrl_put can acquire the rport lock when freeing the
> > ctrl object:
> > 
> > nvme_fc_ctrl_put
> >    nvme_fc_ctrl_free
> >      spin_lock_irqsave(rport->lock)
> > 
> > Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.
> > 
> > Signed-off-by: Daniel Wagner <wagi@kernel.org>
> > ---
> >   drivers/nvme/host/fc.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> > index 03987f497a5b..2c0ea843ae57 100644
> > --- a/drivers/nvme/host/fc.c
> > +++ b/drivers/nvme/host/fc.c
> > @@ -1488,7 +1488,9 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
> >   		if (ret)
> >   			/* leave the ctrl get reference */
> >   			break;
> > +		spin_unlock_irqrestore(&rport->lock, flags);
> >   		nvme_fc_ctrl_put(ctrl);
> > +		spin_lock_irqsave(&rport->lock, flags);
> >   	}
> >   	spin_unlock_irqrestore(&rport->lock, flags);
> > 
> In theory, yes.

I hit this in practice ;)

[PATCH 2/5] nvme-fc: check all request and response have been processed

[Daniel Wagner]

When the rport is removed there shouldn't be any in flight request or
responses.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/host/fc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 2c0ea843ae57..dcb7fc2ca0b7 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -520,6 +520,8 @@ nvme_fc_free_rport(struct kref *ref)
 
 	WARN_ON(rport->remoteport.port_state != FC_OBJSTATE_DELETED);
 	WARN_ON(!list_empty(&rport->ctrl_list));
+	WARN_ON(!list_empty(&rport->ls_req_list));
+	WARN_ON(!list_empty(&rport->ls_rcv_list));
 
 	/* remove from lport list */
 	spin_lock_irqsave(&nvme_fc_lock, flags);

-- 
2.51.0

Re: [PATCH 2/5] nvme-fc: check all request and response have been processed

[Christoph Hellwig]

On Tue, Oct 28, 2025 at 04:26:21PM +0100, Daniel Wagner wrote:
> When the rport is removed there shouldn't be any in flight request or
> responses.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/5] nvme-fc: check all request and response have been processed

[Hannes Reinecke]

On 10/28/25 16:26, Daniel Wagner wrote:
> When the rport is removed there shouldn't be any in flight request or
> responses.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/host/fc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH 3/5] nvmet-fcloop: check all request and response have been processed

[Daniel Wagner]

When the remoteport or the targetport are removed check that there are
no inflight requests or responses.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/target/fcloop.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 5dffcc5becae..4e429a1ea2bd 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -1111,8 +1111,10 @@ fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
 	rport->nport->rport = NULL;
 	spin_unlock_irqrestore(&fcloop_lock, flags);
 
-	if (put_port)
+	if (put_port) {
+		WARN_ON(!list_empty(&rport->ls_list));
 		fcloop_nport_put(rport->nport);
+	}
 }
 
 static void
@@ -1130,8 +1132,10 @@ fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
 	tport->nport->tport = NULL;
 	spin_unlock_irqrestore(&fcloop_lock, flags);
 
-	if (put_port)
+	if (put_port) {
+		WARN_ON(!list_empty(&tport->ls_list));
 		fcloop_nport_put(tport->nport);
+	}
 }
 
 #define	FCLOOP_HW_QUEUES		4

-- 
2.51.0

Re: [PATCH 3/5] nvmet-fcloop: check all request and response have been processed

[Christoph Hellwig]

On Tue, Oct 28, 2025 at 04:26:22PM +0100, Daniel Wagner wrote:
> When the remoteport or the targetport are removed check that there are
> no inflight requests or responses.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/5] nvmet-fcloop: check all request and response have been processed

[Hannes Reinecke]

On 10/28/25 16:26, Daniel Wagner wrote:
> When the remoteport or the targetport are removed check that there are
> no inflight requests or responses.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH 4/5] nvmet-fcloop: remove unused lsdir member.

[Daniel Wagner]

Nothing is using lsdir member in struct fcloop_lsreq.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/target/fcloop.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 4e429a1ea2bd..c30e9a3e014f 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -254,7 +254,6 @@ struct fcloop_nport {
 struct fcloop_lsreq {
 	struct nvmefc_ls_req		*lsreq;
 	struct nvmefc_ls_rsp		ls_rsp;
-	int				lsdir;	/* H2T or T2H */
 	int				status;
 	struct list_head		ls_list; /* fcloop_rport->ls_list */
 };

-- 
2.51.0

Re: [PATCH 4/5] nvmet-fcloop: remove unused lsdir member.

[Christoph Hellwig]

On Tue, Oct 28, 2025 at 04:26:23PM +0100, Daniel Wagner wrote:
> Nothing is using lsdir member in struct fcloop_lsreq.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/5] nvmet-fcloop: remove unused lsdir member.

[Hannes Reinecke]

On 10/28/25 16:26, Daniel Wagner wrote:
> Nothing is using lsdir member in struct fcloop_lsreq.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 4e429a1ea2bd..c30e9a3e014f 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -254,7 +254,6 @@ struct fcloop_nport {
>   struct fcloop_lsreq {
>   	struct nvmefc_ls_req		*lsreq;
>   	struct nvmefc_ls_rsp		ls_rsp;
> -	int				lsdir;	/* H2T or T2H */
>   	int				status;
>   	struct list_head		ls_list; /* fcloop_rport->ls_list */
>   };
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH 5/5] nvmet-fc: use pr_* print macros instead of dev_*

[Daniel Wagner]

Many of the nvmet-fc log messages cannot print the device used, because
it's not there yet:

  (NULL device *): {0:0} Association deleted

Use the pr_* macros consistently throughout the module and match the
output of the nvme-fc module.

Using port:association ids are more useful when debugging what's going
on, because these match now with the log entries from nvme-fc.
---
 drivers/nvme/target/fc.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 7d84527d5a43..0d9784004c9b 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -490,8 +490,7 @@ nvmet_fc_xmt_disconnect_assoc(struct nvmet_fc_tgt_assoc *assoc)
 			sizeof(*discon_rqst) + sizeof(*discon_acc) +
 			tgtport->ops->lsrqst_priv_sz), GFP_KERNEL);
 	if (!lsop) {
-		dev_info(tgtport->dev,
-			"{%d:%d} send Disconnect Association failed: ENOMEM\n",
+		pr_info("{%d:%d}: send Disconnect Association failed: ENOMEM\n",
 			tgtport->fc_target_port.port_num, assoc->a_id);
 		return;
 	}
@@ -513,8 +512,7 @@ nvmet_fc_xmt_disconnect_assoc(struct nvmet_fc_tgt_assoc *assoc)
 	ret = nvmet_fc_send_ls_req_async(tgtport, lsop,
 				nvmet_fc_disconnect_assoc_done);
 	if (ret) {
-		dev_info(tgtport->dev,
-			"{%d:%d} XMT Disconnect Association failed: %d\n",
+		pr_info("{%d:%d}: XMT Disconnect Association failed: %d\n",
 			tgtport->fc_target_port.port_num, assoc->a_id, ret);
 		kfree(lsop);
 	}
@@ -1187,8 +1185,7 @@ nvmet_fc_target_assoc_free(struct kref *ref)
 	if (oldls)
 		nvmet_fc_xmt_ls_rsp(tgtport, oldls);
 	ida_free(&tgtport->assoc_cnt, assoc->a_id);
-	dev_info(tgtport->dev,
-		"{%d:%d} Association freed\n",
+	pr_info("{%d:%d}: Association freed\n",
 		tgtport->fc_target_port.port_num, assoc->a_id);
 	kfree(assoc);
 }
@@ -1224,8 +1221,7 @@ nvmet_fc_delete_target_assoc(struct nvmet_fc_tgt_assoc *assoc)
 			flush_workqueue(assoc->queues[i]->work_q);
 	}
 
-	dev_info(tgtport->dev,
-		"{%d:%d} Association deleted\n",
+	pr_info("{%d:%d}: Association deleted\n",
 		tgtport->fc_target_port.port_num, assoc->a_id);
 
 	nvmet_fc_tgtport_put(tgtport);
@@ -1716,9 +1712,9 @@ nvmet_fc_ls_create_association(struct nvmet_fc_tgtport *tgtport,
 	}
 
 	if (ret) {
-		dev_err(tgtport->dev,
-			"Create Association LS failed: %s\n",
-			validation_errors[ret]);
+		pr_err("{%d}: Create Association LS failed: %s\n",
+		       tgtport->fc_target_port.port_num,
+		       validation_errors[ret]);
 		iod->lsrsp->rsplen = nvme_fc_format_rjt(acc,
 				sizeof(*acc), rqst->w0.ls_cmd,
 				FCNVME_RJT_RC_LOGIC,
@@ -1730,8 +1726,7 @@ nvmet_fc_ls_create_association(struct nvmet_fc_tgtport *tgtport,
 	atomic_set(&queue->connected, 1);
 	queue->sqhd = 0;	/* best place to init value */
 
-	dev_info(tgtport->dev,
-		"{%d:%d} Association created\n",
+	pr_info("{%d:%d}: Association created\n",
 		tgtport->fc_target_port.port_num, iod->assoc->a_id);
 
 	/* format a response */
@@ -1809,9 +1804,9 @@ nvmet_fc_ls_create_connection(struct nvmet_fc_tgtport *tgtport,
 	}
 
 	if (ret) {
-		dev_err(tgtport->dev,
-			"Create Connection LS failed: %s\n",
-			validation_errors[ret]);
+		pr_err("{%d}: Create Connection LS failed: %s\n",
+		       tgtport->fc_target_port.port_num,
+		       validation_errors[ret]);
 		iod->lsrsp->rsplen = nvme_fc_format_rjt(acc,
 				sizeof(*acc), rqst->w0.ls_cmd,
 				(ret == VERR_NO_ASSOC) ?
@@ -1871,9 +1866,9 @@ nvmet_fc_ls_disconnect(struct nvmet_fc_tgtport *tgtport,
 	}
 
 	if (ret || !assoc) {
-		dev_err(tgtport->dev,
-			"Disconnect LS failed: %s\n",
-			validation_errors[ret]);
+		pr_err("{%d}: Disconnect LS failed: %s\n",
+		       tgtport->fc_target_port.port_num,
+		       validation_errors[ret]);
 		iod->lsrsp->rsplen = nvme_fc_format_rjt(acc,
 				sizeof(*acc), rqst->w0.ls_cmd,
 				(ret == VERR_NO_ASSOC) ?
@@ -1907,8 +1902,7 @@ nvmet_fc_ls_disconnect(struct nvmet_fc_tgtport *tgtport,
 	spin_unlock_irqrestore(&tgtport->lock, flags);
 
 	if (oldls) {
-		dev_info(tgtport->dev,
-			"{%d:%d} Multiple Disconnect Association LS's "
+		pr_info("{%d:%d}: Multiple Disconnect Association LS's "
 			"received\n",
 			tgtport->fc_target_port.port_num, assoc->a_id);
 		/* overwrite good response with bogus failure */
@@ -2051,8 +2045,8 @@ nvmet_fc_rcv_ls_req(struct nvmet_fc_target_port *target_port,
 	struct fcnvme_ls_rqst_w0 *w0 = (struct fcnvme_ls_rqst_w0 *)lsreqbuf;
 
 	if (lsreqbuf_len > sizeof(union nvmefc_ls_requests)) {
-		dev_info(tgtport->dev,
-			"RCV %s LS failed: payload too large (%d)\n",
+		pr_info("{%d}: RCV %s LS failed: payload too large (%d)\n",
+			tgtport->fc_target_port.port_num,
 			(w0->ls_cmd <= NVME_FC_LAST_LS_CMD_VALUE) ?
 				nvmefc_ls_names[w0->ls_cmd] : "",
 			lsreqbuf_len);
@@ -2060,8 +2054,8 @@ nvmet_fc_rcv_ls_req(struct nvmet_fc_target_port *target_port,
 	}
 
 	if (!nvmet_fc_tgtport_get(tgtport)) {
-		dev_info(tgtport->dev,
-			"RCV %s LS failed: target deleting\n",
+		pr_info("{%d}: RCV %s LS failed: target deleting\n",
+			tgtport->fc_target_port.port_num,
 			(w0->ls_cmd <= NVME_FC_LAST_LS_CMD_VALUE) ?
 				nvmefc_ls_names[w0->ls_cmd] : "");
 		return -ESHUTDOWN;
@@ -2069,8 +2063,8 @@ nvmet_fc_rcv_ls_req(struct nvmet_fc_target_port *target_port,
 
 	iod = nvmet_fc_alloc_ls_iod(tgtport);
 	if (!iod) {
-		dev_info(tgtport->dev,
-			"RCV %s LS failed: context allocation failed\n",
+		pr_info("{%d}: RCV %s LS failed: context allocation failed\n",
+			tgtport->fc_target_port.port_num,
 			(w0->ls_cmd <= NVME_FC_LAST_LS_CMD_VALUE) ?
 				nvmefc_ls_names[w0->ls_cmd] : "");
 		nvmet_fc_tgtport_put(tgtport);

-- 
2.51.0

Re: [PATCH 5/5] nvmet-fc: use pr_* print macros instead of dev_*

[Daniel Wagner]

On Tue, Oct 28, 2025 at 04:26:24PM +0100, Daniel Wagner wrote:
> Many of the nvmet-fc log messages cannot print the device used, because
> it's not there yet:
> 
>   (NULL device *): {0:0} Association deleted
> 
> Use the pr_* macros consistently throughout the module and match the
> output of the nvme-fc module.
> 
> Using port:association ids are more useful when debugging what's going
> on, because these match now with the log entries from nvme-fc.

Signed-off-by: Daniel Wagner <wagi@kernel.org>

Re: [PATCH 5/5] nvmet-fc: use pr_* print macros instead of dev_*

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/5] nvmet-fc: use pr_* print macros instead of dev_*

[Hannes Reinecke]

On 10/28/25 16:26, Daniel Wagner wrote:
> Many of the nvmet-fc log messages cannot print the device used, because
> it's not there yet:
> 
>    (NULL device *): {0:0} Association deleted
> 
> Use the pr_* macros consistently throughout the module and match the
> output of the nvme-fc module.
> 
> Using port:association ids are more useful when debugging what's going
> on, because these match now with the log entries from nvme-fc.
> ---
>   drivers/nvme/target/fc.c | 48 +++++++++++++++++++++---------------------------
>   1 file changed, 21 insertions(+), 27 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Justin Tee]

Hi Daniel,

> nvme_fc_ctrl_put can acquire the rport lock when freeing the
> ctrl object:
>
> nvme_fc_ctrl_put
>   nvme_fc_ctrl_free
>     spin_lock_irqsave(rport->lock)
>
> Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.

While I agree that we can’t hold the rport lock when calling
nvme_fc_ctrl_put, nvme_fc_ctrl_free also does a nvme_fc_rport_put,
which could also trigger nvme_fc_free_rport, making rport invalid.
Should we also add kref get on the rport before entering the
list_for_each_entry loop?

Also, because nvme_fc_ctrl_free removes itself from the
rport->ctrl_list, should we also start using list_for_each_entry_safe?

So, something like this?

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 03987f497a5b..fcd30801921b 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1468,14 +1468,16 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
 {
        struct fcnvme_ls_disconnect_assoc_rqst *rqst =
                                        &lsop->rqstbuf->rq_dis_assoc;
-       struct nvme_fc_ctrl *ctrl, *ret = NULL;
+       struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
        struct nvmefc_ls_rcv_op *oldls = NULL;
        u64 association_id = be64_to_cpu(rqst->associd.association_id);
        unsigned long flags;

+       nvme_fc_rport_get(rport);
+
        spin_lock_irqsave(&rport->lock, flags);

-       list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
+       list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
                if (!nvme_fc_ctrl_get(ctrl))
                        continue;
                spin_lock(&ctrl->lock);
@@ -1488,11 +1490,15 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
                if (ret)
                        /* leave the ctrl get reference */
                        break;
+               spin_unlock_irqrestore(&rport->lock, flags);
                nvme_fc_ctrl_put(ctrl);
+               spin_lock_irqsave(&rport->lock, flags);
        }

        spin_unlock_irqrestore(&rport->lock, flags);

+       nvme_fc_rport_put(rport);
+
        /* transmit a response for anything that was pending */
        if (oldls) {
                dev_info(rport->lport->dev,

Regards,
Justin

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Daniel Wagner]

Hi Justin,

On Tue, Oct 28, 2025 at 05:33:17PM -0700, Justin Tee wrote:
> > nvme_fc_ctrl_put can acquire the rport lock when freeing the
> > ctrl object:
> >
> > nvme_fc_ctrl_put
> >   nvme_fc_ctrl_free
> >     spin_lock_irqsave(rport->lock)
> >
> > Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.
> 
> While I agree that we can’t hold the rport lock when calling
> nvme_fc_ctrl_put, nvme_fc_ctrl_free also does a nvme_fc_rport_put,
> which could also trigger nvme_fc_free_rport, making rport invalid.
> Should we also add kref get on the rport before entering the
> list_for_each_entry loop?
> 
> Also, because nvme_fc_ctrl_free removes itself from the
> rport->ctrl_list, should we also start using list_for_each_entry_safe?
> 
> So, something like this?

Yes, this makes sense. Just wondering why I didn't see any KASAN
reports. 

Should I add your change to my patch (obviously mentioning it), or do
you want to send a patch yourself?

In the meantime, I am giving this patch a spin in my test setup.

Thanks,
Daniel

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Justin Tee]

Hi Daniel,

> Should I add your change to my patch (obviously mentioning it)
Sure, this sounds fine to me.

Thanks,
Justin

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Daniel Wagner]

On Tue, Oct 28, 2025 at 05:33:17PM -0700, Justin Tee wrote:
> So, something like this?
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 03987f497a5b..fcd30801921b 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1468,14 +1468,16 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>  {
>         struct fcnvme_ls_disconnect_assoc_rqst *rqst =
>                                         &lsop->rqstbuf->rq_dis_assoc;
> -       struct nvme_fc_ctrl *ctrl, *ret = NULL;
> +       struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
>         struct nvmefc_ls_rcv_op *oldls = NULL;
>         u64 association_id = be64_to_cpu(rqst->associd.association_id);
>         unsigned long flags;
> 
> +       nvme_fc_rport_get(rport);
> +
>         spin_lock_irqsave(&rport->lock, flags);
> 
> -       list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> +       list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
>                 if (!nvme_fc_ctrl_get(ctrl))
>                         continue;
>                 spin_lock(&ctrl->lock);
> @@ -1488,11 +1490,15 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>                 if (ret)
>                         /* leave the ctrl get reference */
>                         break;
> +               spin_unlock_irqrestore(&rport->lock, flags);
>                 nvme_fc_ctrl_put(ctrl);
> +               spin_lock_irqsave(&rport->lock, flags);
>         }
> 
>         spin_unlock_irqrestore(&rport->lock, flags);
> 
> +       nvme_fc_rport_put(rport);
> +
>         /* transmit a response for anything that was pending */
>         if (oldls) {
>                 dev_info(rport->lport->dev,

I've dropped the nvme_fc_rport_get because it's not necessary, a ref is
taken in nvme_fc_rcv_ls_req, so the port is not going away until the
disconnect LS is processed.

All tests seems to work fine except one, nvme/58 (test rapid namespace
remapping). But this one also fails for the other trasnport, e.g TCP:

	block nvme1n1: no available path - failing I/O
	Oops: general protection fault, probably for non-canonical address 0xdffffc00000I
	KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
	CPU: 5 UID: 0 PID: 707 Comm: kworker/5:2H Tainted: G      D W           6.17.0-ra
	Tainted: [D]=DIE, [W]=WARN
	Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-6.fc43 04/01/2014
	Workqueue: kblockd nvme_requeue_work [nvme_core]
	RIP: 0010:__rq_qos_done_bio+0x24/0xa0
	Code: 90 90 90 90 90 90 0f 1f 44 00 00 41 54 55 53 49 89 f4 48 83 ec 08 48 89 fbc
	RSP: 0018:ffff888116e07980 EFLAGS: 00010256
	RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
	RDX: 1ffff11022f2a6eb RSI: ffff88810d2a6140 RDI: 0000000000000000
	nvme nvme4: IDs don't match for shared namespace 3
	RBP: dffffc0000000000 R08: 0000000000000001 R09: ffffffff9f4df3cf
	R10: ffff888116e0760f R11: ffffffffa6b771d0 R12: ffff88810d2a6140
	R13: ffff88810d2a6148 R14: ffff88810d2a6140 R15: ffff888106d66808
	FS:  0000000000000000(0000) GS:ffff8881b2408000(0000) knlGS:0000000000000000
	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	nvme nvme3: rescanning namespaces.
	CR2: 000055f4b1f6d080 CR3: 000000011c082000 CR4: 0000000000750ef0
	PKRU: 55555554
	Call Trace:
	 <TASK>
	 bio_endio+0x1c9/0x610
	 nvme_ns_head_submit_bio+0x79e/0xa20 [nvme_core 8550ebbc328e8e8a801d6cacfcd551a5]
	 ? lock_release+0x244/0x450
	 __submit_bio+0x31a/0x6c0
	 ? __pfx___submit_bio+0x10/0x10
	 ? lock_acquire+0x292/0x2e0
	 ? lock_release+0x244/0x450
	 ? __pfx_blk_cgroup_bio_start+0x10/0x10
	 ? lock_acquire+0x292/0x2e0
	 ? submit_bio_noacct_nocheck+0x577/0xaa0
	 submit_bio_noacct_nocheck+0x577/0xaa0
	 ? trace_irq_enable.constprop.0+0xc0/0x100
	 ? trace_hardirqs_on+0x32/0x40
	 ? __pfx_submit_bio_noacct_nocheck+0x10/0x10
	 ? submit_bio_noacct+0x97d/0x1ce0
	 nvme_requeue_work+0xa2/0xd0 [nvme_core 8550ebbc328e8e8a801d6cacfcd551a5345e7fd9]
	 process_one_work+0x7f3/0x1420
	 ? __pfx_process_one_work+0x10/0x10
	 ? local_clock+0x11/0x30
	 ? assign_work+0x156/0x390
	 worker_thread+0x5cf/0xfe0
	 ? _raw_spin_unlock_irqrestore+0x40/0x60
	 ? __pfx_worker_thread+0x10/0x10
	 ? __kthread_parkme+0xb3/0x1f0
	 ? __pfx_worker_thread+0x10/0x10
	 kthread+0x39d/0x740
	 ? __pfx_do_raw_spin_trylock+0x10/0x10
	 ? __pfx_kthread+0x10/0x10
	 ? kvm_sched_clock_read+0xd/0x20
	 ? local_clock_noinstr+0xb/0xf0
	 ? local_clock+0x11/0x30
	 ? lock_release+0x2e2/0x450
	 ? __pfx_kthread+0x10/0x10
	 ret_from_fork+0x371/0x460
	 ? __pfx_kthread+0x10/0x10
	 ? __pfx_kthread+0x10/0x10
	 ret_from_fork_asm+0x1a/0x30
	 </TASK>
	Modules linked in: nvmet_tcp nvmet nvme_tcp nvme nvme_fabrics nvme_core nvme_autp
	---[ end trace 0000000000000000 ]---
	

Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

[Justin Tee]

> I've dropped the nvme_fc_rport_get because it's not necessary, a ref is
> taken in nvme_fc_rcv_ls_req, so the port is not going away until the
> disconnect LS is processed.
Yes, correct, and the put would be in
nvme_fc_xmt_ls_rsp_done->nvme_fc_xmt_ls_rsp_free.  Thank you for
checking.

> All tests seems to work fine except one, nvme/58 (test rapid namespace
> remapping). But this one also fails for the other trasnport, e.g TCP:
Okay, sure, we can try to address this in a different patch set.

Regards,
Justin