[PATCH 00/11] nvmet-fcloop: track resources via reference counting

[PATCH 00/11] nvmet-fcloop: track resources via reference counting

[Daniel Wagner]

The fcloop module is mainly used for testing, that is with blktests.
Unfortunatly, there are shortcomings when it comes to releases resources.
E.g. unloading the module can result in UAFs.

I've written two new blktests which are triggering reconnects. The first
one is doing this by removing the target while the host is still up and
running. The second one is trigger a reset via the new debugfs interface.

Both of these two tests cases work fine for TCP and RDMA but not FC.

By introducing reference counting on various objects the UAFs go away.
Also KASAN is happy. There are also a bunch of fixes for nvmet-fc which
got uncovered by the fcloop fixes.

I still see one UAF sometimes happening. When the association attempt
fails (looks like yet another bug), the test case removes all resources:

 (NULL device *): Create Association LS failed: Association Allocation Failed
 (NULL device *): queue 0 connect admin queue failed (-6).
 nvme nvme1: NVME-FC{0}: reset: Reconnect attempt failed (-6)
 nvme nvme1: NVME-FC{0}: Reconnect attempt in 1 seconds
 nvme nvme1: NVME-FC{0}: create association : host wwpn 0x20001100aa000001  rport wwpn 0x20001100ab000001: NQN "blktests-subsystem-1"
 (NULL device *): Create Association LS failed: Association Allocation Failed
 (NULL device *): queue 0 connect admin queue failed (-6).
 nvme nvme1: NVME-FC{0}: reset: Reconnect attempt failed (-6)
 nvme nvme1: NVME-FC{0}: Reconnect attempt in 1 seconds
 nvme nvme1: Removing ctrl: NQN "blktests-subsystem-1"
 nvme_ns_head_submit_bio: 29 callbacks suppressed
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 block nvme1n1: no available path - failing I/O
 nvme nvme2: Removing ctrl: NQN "nqn.2014-08.org.nvmexpress.discovery"
 ==================================================================
 BUG: KASAN: slab-use-after-free in nvme_fc_rescan_remoteport+0x56/0x1d0 [nvme_fc]
 Read of size 8 at addr ffff88810b662890 by task kworker/u36:10/1876

 CPU: 3 UID: 0 PID: 1876 Comm: kworker/u36:10 Tainted: G        W          6.14.0-rc2+ #48 d5f3bf6340950de08bebd912d815fcf6b60c18ab
 Tainted: [W]=WARN
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-3.fc41 04/01/2014
 Workqueue: nvmet-wq fcloop_tgt_rscn_work [nvme_fcloop]

static void nvmet_port_subsys_drop_link(struct config_item *parent,
		struct config_item *target)
{
	[...]
found:
	list_del(&p->entry);
	nvmet_port_del_ctrls(port, subsys);
	nvmet_port_disc_changed(port, subsys);   /* XXX triggers the above UAF */

	if (list_empty(&port->subsystems))
		nvmet_disable_port(port);
	up_write(&nvmet_config_sem);
	kfree(p);
}

The nvmet_port_disc_changed is a bit useless, because these event will
never be seen by the host. Anyway, more debugging is necessary.

I'll send the new tests cases for blktests soon.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
Daniel Wagner (11):
      nvmet-fcloop: remove nport from list on last user
      nvmet-fcloop: add ref counting to lport
      nvmet-fcloop: refactor fcloop_nport_alloc
      nvmet-fcloop: track ref counts for nports
      nvmet-fcloop: track tport with ref counting
      nvmet-fcloop: track rport with ref counting
      nvmet-fc: update tgtport ref per assoc
      nvmet-fc: take tgtport reference only once
      nvmet-fc: free pending reqs on tgtport unregister
      nvmet-fc: inline nvmet_fc_delete_assoc
      nvmet-fc: inline nvmet_fc_free_hostport

 drivers/nvme/target/fc.c     |  82 +++++------
 drivers/nvme/target/fcloop.c | 326 ++++++++++++++++++++++++++++---------------
 2 files changed, 255 insertions(+), 153 deletions(-)
---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250214-nvmet-fcloop-a649738b7e6e

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

[PATCH 01/11] nvmet-fcloop: remove nport from list on last user

[Daniel Wagner]

The nport object has an association with the rport and lport object,
that means we can only remove an nport object from the global nport_list
after the last user of an rport or lport is gone.

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

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index e1abb27927ff74c9c55ddefd9581aab18bf3b00f..5493677a948d34391c7c08055dfefd91cc3ff33f 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -1005,6 +1005,11 @@ fcloop_nport_free(struct kref *ref)
 {
 	struct fcloop_nport *nport =
 		container_of(ref, struct fcloop_nport, ref);
+	unsigned long flags;
+
+	spin_lock_irqsave(&fcloop_lock, flags);
+	list_del(&nport->nport_list);
+	spin_unlock_irqrestore(&fcloop_lock, flags);
 
 	kfree(nport);
 }
@@ -1363,8 +1368,6 @@ __unlink_remote_port(struct fcloop_nport *nport)
 		nport->tport->remoteport = NULL;
 	nport->rport = NULL;
 
-	list_del(&nport->nport_list);
-
 	return rport;
 }
 

-- 
2.48.1

Re: [PATCH 01/11] nvmet-fcloop: remove nport from list on last user

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> The nport object has an association with the rport and lport object,
> that means we can only remove an nport object from the global nport_list
> after the last user of an rport or lport is gone.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 7 +++++--
>   1 file changed, 5 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

Re: [PATCH 01/11] nvmet-fcloop: remove nport from list on last user

[Christoph Hellwig]

Looks good:

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

[PATCH 02/11] nvmet-fcloop: add ref counting to lport

[Daniel Wagner]

The fcloop_lport objects live time is controlled by the user interface
add_local_port and del_local_port. nport, rport and tport objects are
pointing to the lport objects but here is no clear tracking. Let's
introduce an explicit ref counter for the lport objects and prepare the
stage for restructuring how lports are used.

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

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 5493677a948d34391c7c08055dfefd91cc3ff33f..ca46830d46ecbaae21f3ee3e69aa7d52905abcae 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -208,6 +208,7 @@ struct fcloop_lport {
 	struct nvme_fc_local_port *localport;
 	struct list_head lport_list;
 	struct completion unreg_done;
+	struct kref ref;
 };
 
 struct fcloop_lport_priv {
@@ -1000,6 +1001,32 @@ fcloop_fcp_abort(struct nvme_fc_local_port *localport,
 	}
 }
 
+static void
+fcloop_lport_free(struct kref *ref)
+{
+	struct fcloop_lport *lport =
+		container_of(ref, struct fcloop_lport, ref);
+	unsigned long flags;
+
+	spin_lock_irqsave(&fcloop_lock, flags);
+	list_del(&lport->lport_list);
+	spin_unlock_irqrestore(&fcloop_lock, flags);
+
+	kfree(lport);
+}
+
+static void
+fcloop_lport_put(struct fcloop_lport *lport)
+{
+	kref_put(&lport->ref, fcloop_lport_free);
+}
+
+static int
+fcloop_lport_get(struct fcloop_lport *lport)
+{
+	return kref_get_unless_zero(&lport->ref);
+}
+
 static void
 fcloop_nport_free(struct kref *ref)
 {
@@ -1145,6 +1172,7 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr,
 
 		lport->localport = localport;
 		INIT_LIST_HEAD(&lport->lport_list);
+		kref_init(&lport->ref);
 
 		spin_lock_irqsave(&fcloop_lock, flags);
 		list_add_tail(&lport->lport_list, &fcloop_lports);
@@ -1161,13 +1189,6 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr,
 	return ret ? ret : count;
 }
 
-
-static void
-__unlink_local_port(struct fcloop_lport *lport)
-{
-	list_del(&lport->lport_list);
-}
-
 static int
 __wait_localport_unreg(struct fcloop_lport *lport)
 {
@@ -1180,7 +1201,7 @@ __wait_localport_unreg(struct fcloop_lport *lport)
 	if (!ret)
 		wait_for_completion(&lport->unreg_done);
 
-	kfree(lport);
+	fcloop_lport_put(lport);
 
 	return ret;
 }
@@ -1204,8 +1225,9 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
 	list_for_each_entry(tlport, &fcloop_lports, lport_list) {
 		if (tlport->localport->node_name == nodename &&
 		    tlport->localport->port_name == portname) {
+			if (!fcloop_lport_get(tlport))
+				break;
 			lport = tlport;
-			__unlink_local_port(lport);
 			break;
 		}
 	}
@@ -1215,6 +1237,7 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
 		return -ENOENT;
 
 	ret = __wait_localport_unreg(lport);
+	fcloop_lport_put(lport);
 
 	return ret ? ret : count;
 }
@@ -1640,17 +1663,17 @@ static void __exit fcloop_exit(void)
 	for (;;) {
 		lport = list_first_entry_or_null(&fcloop_lports,
 						typeof(*lport), lport_list);
-		if (!lport)
+		if (!lport || !fcloop_lport_get(lport))
 			break;
 
-		__unlink_local_port(lport);
-
 		spin_unlock_irqrestore(&fcloop_lock, flags);
 
 		ret = __wait_localport_unreg(lport);
 		if (ret)
 			pr_warn("%s: Failed deleting local port\n", __func__);
 
+		fcloop_lport_put(lport);
+
 		spin_lock_irqsave(&fcloop_lock, flags);
 	}
 

-- 
2.48.1

Re: [PATCH 02/11] nvmet-fcloop: add ref counting to lport

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> The fcloop_lport objects live time is controlled by the user interface
> add_local_port and del_local_port. nport, rport and tport objects are
> pointing to the lport objects but here is no clear tracking. Let's
> introduce an explicit ref counter for the lport objects and prepare the
> stage for restructuring how lports are used.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 47 +++++++++++++++++++++++++++++++++-----------
>   1 file changed, 35 insertions(+), 12 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 02/11] nvmet-fcloop: add ref counting to lport

[Christoph Hellwig]

On Wed, Feb 26, 2025 at 07:45:54PM +0100, Daniel Wagner wrote:
> +static void
> +fcloop_lport_free(struct kref *ref)
> +{
> +	struct fcloop_lport *lport =
> +		container_of(ref, struct fcloop_lport, ref);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	list_del(&lport->lport_list);
> +	spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> +	kfree(lport);

Maybe it's just me, but I find the kref a really horrible pattern over
usig a simple refcount_t.

Otherwise adding proper refcounting looks fine.

Re: [PATCH 02/11] nvmet-fcloop: add ref counting to lport

[Daniel Wagner]

On Wed, Mar 05, 2025 at 03:17:40PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 26, 2025 at 07:45:54PM +0100, Daniel Wagner wrote:
> > +static void
> > +fcloop_lport_free(struct kref *ref)
> > +{
> > +	struct fcloop_lport *lport =
> > +		container_of(ref, struct fcloop_lport, ref);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fcloop_lock, flags);
> > +	list_del(&lport->lport_list);
> > +	spin_unlock_irqrestore(&fcloop_lock, flags);
> > +
> > +	kfree(lport);
> 
> Maybe it's just me, but I find the kref a really horrible pattern over
> usig a simple refcount_t.

There was already some kref usage in the fc code, that's why I started
to use it. But I agree there is not much to be gained from the kref
wrappers. I'll replace them.

> Otherwise adding proper refcounting looks fine.

BTW, I found a bunch more places which need to do proper ref counting. I
have now a version which works pretty good. Though there is one
case which gives me an UAF:

  setup target
  setup host
  connect
  loop
    remove target
    wait for host connecting state
    add target
    wait for host live state

When fcloop has no in flight commands and the target is removed, fcloop
will unregister the localport now. But the nvme-fc driver just assumes
that the port is always there and just sends down new commands
independend of the port status:

nvme_fc_start_fcp_op()
{
[...]

	ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport,
					&ctrl->rport->remoteport,
					queue->lldd_handle, &op->fcp_req);
[...]
}

There is nothing which updates the ctrl in nvme_fc_unregister_localport.
Not really sure what to do here. fcloop obviously is now behaving
differently to the hw drivers. But still, this looks very fragile that
there is no sort of synchronization between port unregistration and ctrl
state.

Re: [PATCH 02/11] nvmet-fcloop: add ref counting to lport

[Daniel Wagner]

On Thu, Mar 06, 2025 at 10:26:40AM +0100, Daniel Wagner wrote:
> There is nothing which updates the ctrl in nvme_fc_unregister_localport.
> Not really sure what to do here. fcloop obviously is now behaving
> differently to the hw drivers. But still, this looks very fragile that
> there is no sort of synchronization between port unregistration and ctrl
> state.

Ah I see what's supposed to happen in this scenario.
nvme_fc_unregister_localport will return -EINVAL when the port is still
in use. That means fcloop is not allowed to pull the rug yet.

[PATCH 03/11] nvmet-fcloop: refactor fcloop_nport_alloc

[Daniel Wagner]

There are many different operations done under the spin lock. Since the
lport and nport are ref counted it's possible to only use for the list
insert and lookup the spin lock.

This allows us to untangle the setup steps into a more linear form which
reduces the complexity of the functions.

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

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index ca46830d46ecbaae21f3ee3e69aa7d52905abcae..de1963c34bd88d0335f70de569565740fd395a0a 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -1038,6 +1038,8 @@ fcloop_nport_free(struct kref *ref)
 	list_del(&nport->nport_list);
 	spin_unlock_irqrestore(&fcloop_lock, flags);
 
+	if (nport->lport)
+		fcloop_lport_put(nport->lport);
 	kfree(nport);
 }
 
@@ -1206,33 +1208,63 @@ __wait_localport_unreg(struct fcloop_lport *lport)
 	return ret;
 }
 
+static struct fcloop_lport *
+fcloop_lport_lookup(u64 node_name, u64 port_name)
+{
+	struct fcloop_lport *lp, *lport = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fcloop_lock, flags);
+	list_for_each_entry(lp, &fcloop_lports, lport_list) {
+		if (lp->localport->node_name != node_name ||
+		    lp->localport->port_name != port_name)
+			continue;
+
+		if (fcloop_lport_get(lp))
+			lport = lp;
+
+		break;
+	}
+	spin_unlock_irqrestore(&fcloop_lock, flags);
+
+	return lport;
+}
+
+static struct fcloop_nport *
+fcloop_nport_lookup(u64 node_name, u64 port_name)
+{
+	struct fcloop_nport *np, *nport = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fcloop_lock, flags);
+	list_for_each_entry(np, &fcloop_nports, nport_list) {
+		if (np->node_name != node_name ||
+		    np->port_name != port_name)
+			continue;
+
+		if (fcloop_nport_get(np))
+			nport = np;
+
+		break;
+	}
+	spin_unlock_irqrestore(&fcloop_lock, flags);
+
+	return nport;
+}
 
 static ssize_t
 fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {
-	struct fcloop_lport *tlport, *lport = NULL;
+	struct fcloop_lport *lport;
 	u64 nodename, portname;
-	unsigned long flags;
 	int ret;
 
 	ret = fcloop_parse_nm_options(dev, &nodename, &portname, buf);
 	if (ret)
 		return ret;
 
-	spin_lock_irqsave(&fcloop_lock, flags);
-
-	list_for_each_entry(tlport, &fcloop_lports, lport_list) {
-		if (tlport->localport->node_name == nodename &&
-		    tlport->localport->port_name == portname) {
-			if (!fcloop_lport_get(tlport))
-				break;
-			lport = tlport;
-			break;
-		}
-	}
-	spin_unlock_irqrestore(&fcloop_lock, flags);
-
+	lport = fcloop_lport_lookup(nodename, portname);
 	if (!lport)
 		return -ENOENT;
 
@@ -1245,9 +1277,9 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
 static struct fcloop_nport *
 fcloop_alloc_nport(const char *buf, size_t count, bool remoteport)
 {
-	struct fcloop_nport *newnport, *nport = NULL;
-	struct fcloop_lport *tmplport, *lport = NULL;
 	struct fcloop_ctrl_options *opts;
+	struct fcloop_nport *nport;
+	struct fcloop_lport *lport;
 	unsigned long flags;
 	u32 opts_mask = (remoteport) ? RPORT_OPTS : TGTPORT_OPTS;
 	int ret;
@@ -1261,79 +1293,59 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport)
 		goto out_free_opts;
 
 	/* everything there ? */
-	if ((opts->mask & opts_mask) != opts_mask) {
-		ret = -EINVAL;
+	if ((opts->mask & opts_mask) != opts_mask)
 		goto out_free_opts;
-	}
 
-	newnport = kzalloc(sizeof(*newnport), GFP_KERNEL);
-	if (!newnport)
+	lport = fcloop_lport_lookup(opts->wwnn, opts->wwpn);
+	if (lport) {
+		/* invalid configuration */
+		fcloop_lport_put(lport);
 		goto out_free_opts;
+	}
 
-	INIT_LIST_HEAD(&newnport->nport_list);
-	newnport->node_name = opts->wwnn;
-	newnport->port_name = opts->wwpn;
-	if (opts->mask & NVMF_OPT_ROLES)
-		newnport->port_role = opts->roles;
-	if (opts->mask & NVMF_OPT_FCADDR)
-		newnport->port_id = opts->fcaddr;
-	kref_init(&newnport->ref);
+	nport = fcloop_nport_lookup(opts->wwnn, opts->wwpn);
+	if (nport && ((remoteport && nport->rport) ||
+		      (!remoteport && nport->tport))) {
+		/* invalid configuration */
+		goto out_put_nport;
+	}
 
-	spin_lock_irqsave(&fcloop_lock, flags);
+	if (!nport) {
+		nport = kzalloc(sizeof(*nport), GFP_KERNEL);
+		if (!nport)
+			goto out_free_opts;
 
-	list_for_each_entry(tmplport, &fcloop_lports, lport_list) {
-		if (tmplport->localport->node_name == opts->wwnn &&
-		    tmplport->localport->port_name == opts->wwpn)
-			goto out_invalid_opts;
+		INIT_LIST_HEAD(&nport->nport_list);
+		nport->node_name = opts->wwnn;
+		nport->port_name = opts->wwpn;
+		kref_init(&nport->ref);
 
-		if (tmplport->localport->node_name == opts->lpwwnn &&
-		    tmplport->localport->port_name == opts->lpwwpn)
-			lport = tmplport;
+		spin_lock_irqsave(&fcloop_lock, flags);
+		list_add_tail(&nport->nport_list, &fcloop_nports);
+		spin_unlock_irqrestore(&fcloop_lock, flags);
 	}
 
+	if (opts->mask & NVMF_OPT_ROLES)
+		nport->port_role = opts->roles;
+	if (opts->mask & NVMF_OPT_FCADDR)
+		nport->port_id = opts->fcaddr;
+
 	if (remoteport) {
+		lport = fcloop_lport_lookup(opts->lpwwnn, opts->lpwwpn);
 		if (!lport)
-			goto out_invalid_opts;
-		newnport->lport = lport;
-	}
-
-	list_for_each_entry(nport, &fcloop_nports, nport_list) {
-		if (nport->node_name == opts->wwnn &&
-		    nport->port_name == opts->wwpn) {
-			if ((remoteport && nport->rport) ||
-			    (!remoteport && nport->tport)) {
-				nport = NULL;
-				goto out_invalid_opts;
-			}
-
-			fcloop_nport_get(nport);
+			goto out_put_nport;
 
-			spin_unlock_irqrestore(&fcloop_lock, flags);
-
-			if (remoteport)
-				nport->lport = lport;
-			if (opts->mask & NVMF_OPT_ROLES)
-				nport->port_role = opts->roles;
-			if (opts->mask & NVMF_OPT_FCADDR)
-				nport->port_id = opts->fcaddr;
-			goto out_free_newnport;
-		}
+		nport->lport = lport;
 	}
 
-	list_add_tail(&newnport->nport_list, &fcloop_nports);
-
-	spin_unlock_irqrestore(&fcloop_lock, flags);
-
 	kfree(opts);
-	return newnport;
+	return nport;
 
-out_invalid_opts:
-	spin_unlock_irqrestore(&fcloop_lock, flags);
-out_free_newnport:
-	kfree(newnport);
+out_put_nport:
+	fcloop_nport_put(nport);
 out_free_opts:
 	kfree(opts);
-	return nport;
+	return NULL;
 }
 
 static ssize_t

-- 
2.48.1

Re: [PATCH 03/11] nvmet-fcloop: refactor fcloop_nport_alloc

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> There are many different operations done under the spin lock. Since the
> lport and nport are ref counted it's possible to only use for the list
> insert and lookup the spin lock.
> 
... it's possible to use the spin lock only for the list instert and lookup.

> This allows us to untangle the setup steps into a more linear form which
> reduces the complexity of the functions.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 156 +++++++++++++++++++++++--------------------
>   1 file changed, 84 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index ca46830d46ecbaae21f3ee3e69aa7d52905abcae..de1963c34bd88d0335f70de569565740fd395a0a 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1038,6 +1038,8 @@ fcloop_nport_free(struct kref *ref)
>   	list_del(&nport->nport_list);
>   	spin_unlock_irqrestore(&fcloop_lock, flags);
>   
> +	if (nport->lport)
> +		fcloop_lport_put(nport->lport);
>   	kfree(nport);
>   }
>   
> @@ -1206,33 +1208,63 @@ __wait_localport_unreg(struct fcloop_lport *lport)
>   	return ret;
>   }
>   
> +static struct fcloop_lport *
> +fcloop_lport_lookup(u64 node_name, u64 port_name)
> +{
> +	struct fcloop_lport *lp, *lport = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	list_for_each_entry(lp, &fcloop_lports, lport_list) {
> +		if (lp->localport->node_name != node_name ||
> +		    lp->localport->port_name != port_name)
> +			continue;
> +
> +		if (fcloop_lport_get(lp))
> +			lport = lp;
> +
> +		break;
> +	}
> +	spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> +	return lport;
> +}
> +
> +static struct fcloop_nport *
> +fcloop_nport_lookup(u64 node_name, u64 port_name)
> +{
> +	struct fcloop_nport *np, *nport = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	list_for_each_entry(np, &fcloop_nports, nport_list) {
> +		if (np->node_name != node_name ||
> +		    np->port_name != port_name)
> +			continue;
> +
> +		if (fcloop_nport_get(np))
> +			nport = np;
> +
> +		break;
> +	}
> +	spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> +	return nport;
> +}
>   
>   static ssize_t
>   fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
>   		const char *buf, size_t count)
>   {
> -	struct fcloop_lport *tlport, *lport = NULL;
> +	struct fcloop_lport *lport;
>   	u64 nodename, portname;
> -	unsigned long flags;
>   	int ret;
>   
>   	ret = fcloop_parse_nm_options(dev, &nodename, &portname, buf);
>   	if (ret)
>   		return ret;
>   
> -	spin_lock_irqsave(&fcloop_lock, flags);
> -
> -	list_for_each_entry(tlport, &fcloop_lports, lport_list) {
> -		if (tlport->localport->node_name == nodename &&
> -		    tlport->localport->port_name == portname) {
> -			if (!fcloop_lport_get(tlport))
> -				break;
> -			lport = tlport;
> -			break;
> -		}
> -	}
> -	spin_unlock_irqrestore(&fcloop_lock, flags);
> -
> +	lport = fcloop_lport_lookup(nodename, portname);
>   	if (!lport)
>   		return -ENOENT;
>   
> @@ -1245,9 +1277,9 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
>   static struct fcloop_nport *
>   fcloop_alloc_nport(const char *buf, size_t count, bool remoteport)
>   {
> -	struct fcloop_nport *newnport, *nport = NULL;
> -	struct fcloop_lport *tmplport, *lport = NULL;
>   	struct fcloop_ctrl_options *opts;
> +	struct fcloop_nport *nport;
> +	struct fcloop_lport *lport;
>   	unsigned long flags;
>   	u32 opts_mask = (remoteport) ? RPORT_OPTS : TGTPORT_OPTS;
>   	int ret;
> @@ -1261,79 +1293,59 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport)
>   		goto out_free_opts;
>   
>   	/* everything there ? */
> -	if ((opts->mask & opts_mask) != opts_mask) {
> -		ret = -EINVAL;
> +	if ((opts->mask & opts_mask) != opts_mask)
>   		goto out_free_opts;
> -	}
>   
> -	newnport = kzalloc(sizeof(*newnport), GFP_KERNEL);
> -	if (!newnport)
> +	lport = fcloop_lport_lookup(opts->wwnn, opts->wwpn);
> +	if (lport) {
> +		/* invalid configuration */
> +		fcloop_lport_put(lport);
>   		goto out_free_opts;
> +	}
>   
> -	INIT_LIST_HEAD(&newnport->nport_list);
> -	newnport->node_name = opts->wwnn;
> -	newnport->port_name = opts->wwpn;
> -	if (opts->mask & NVMF_OPT_ROLES)
> -		newnport->port_role = opts->roles;
> -	if (opts->mask & NVMF_OPT_FCADDR)
> -		newnport->port_id = opts->fcaddr;
> -	kref_init(&newnport->ref);
> +	nport = fcloop_nport_lookup(opts->wwnn, opts->wwpn);
> +	if (nport && ((remoteport && nport->rport) ||
> +		      (!remoteport && nport->tport))) {
> +		/* invalid configuration */
> +		goto out_put_nport;
> +	}
>   
> -	spin_lock_irqsave(&fcloop_lock, flags);
> +	if (!nport) {
> +		nport = kzalloc(sizeof(*nport), GFP_KERNEL);
> +		if (!nport)
> +			goto out_free_opts;
>   
> -	list_for_each_entry(tmplport, &fcloop_lports, lport_list) {
> -		if (tmplport->localport->node_name == opts->wwnn &&
> -		    tmplport->localport->port_name == opts->wwpn)
> -			goto out_invalid_opts;
> +		INIT_LIST_HEAD(&nport->nport_list);
> +		nport->node_name = opts->wwnn;
> +		nport->port_name = opts->wwpn;
> +		kref_init(&nport->ref);
>   
> -		if (tmplport->localport->node_name == opts->lpwwnn &&
> -		    tmplport->localport->port_name == opts->lpwwpn)
> -			lport = tmplport;
> +		spin_lock_irqsave(&fcloop_lock, flags);
> +		list_add_tail(&nport->nport_list, &fcloop_nports);
> +		spin_unlock_irqrestore(&fcloop_lock, flags);

Don't you need to check here if an 'nport' with the same node_name and
port_name is already present?

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 03/11] nvmet-fcloop: refactor fcloop_nport_alloc

[Daniel Wagner]

On Fri, Feb 28, 2025 at 08:11:11AM +0100, Hannes Reinecke wrote:
> > +	nport = fcloop_nport_lookup(opts->wwnn, opts->wwpn);
> > +	if (nport && ((remoteport && nport->rport) ||
> > +		      (!remoteport && nport->tport))) {
> > +		/* invalid configuration */
> > +		goto out_put_nport;
> > +	}
> > -	spin_lock_irqsave(&fcloop_lock, flags);
> > +	if (!nport) {
> > +		nport = kzalloc(sizeof(*nport), GFP_KERNEL);
> > +		if (!nport)
> > +			goto out_free_opts;
> > -	list_for_each_entry(tmplport, &fcloop_lports, lport_list) {
> > -		if (tmplport->localport->node_name == opts->wwnn &&
> > -		    tmplport->localport->port_name == opts->wwpn)
> > -			goto out_invalid_opts;
> > +		INIT_LIST_HEAD(&nport->nport_list);
> > +		nport->node_name = opts->wwnn;
> > +		nport->port_name = opts->wwpn;
> > +		kref_init(&nport->ref);
> > -		if (tmplport->localport->node_name == opts->lpwwnn &&
> > -		    tmplport->localport->port_name == opts->lpwwpn)
> > -			lport = tmplport;
> > +		spin_lock_irqsave(&fcloop_lock, flags);
> > +		list_add_tail(&nport->nport_list, &fcloop_nports);
> > +		spin_unlock_irqrestore(&fcloop_lock, flags);
> 
> Don't you need to check here if an 'nport' with the same node_name and
> port_name is already present?

There is the existing check which filters out some of the duplicates
(the check is there to allow setting up the target or the remote port
first, so the order doesn't matter), though I am not sure if it would
catch all duplicates. I don't mind adding this, but I'd say it would be
better in a separate patch. I tried to refactor this code without
changing anything else.

Re: [PATCH 03/11] nvmet-fcloop: refactor fcloop_nport_alloc

[Christoph Hellwig]

Looks good:

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

[PATCH 04/11] nvmet-fcloop: track ref counts for nports

[Daniel Wagner]

A nport object is always used in association with targerport,
remoteport, tport and rport objects. Add explicit references for any of
the associated object. This makes the unreliable reference updates in
the two callback fcloop_targetport_delete and fcloop_remoteport_delete
obsolete.

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

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index de1963c34bd88d0335f70de569565740fd395a0a..80693705c069dd114b2d4f15d0482dd2d713a273 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -1071,16 +1071,11 @@ fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
 	struct fcloop_rport *rport = remoteport->private;
 
 	flush_work(&rport->ls_work);
-	fcloop_nport_put(rport->nport);
 }
 
 static void
 fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
 {
-	struct fcloop_tport *tport = targetport->private;
-
-	flush_work(&tport->ls_work);
-	fcloop_nport_put(tport->nport);
 }
 
 #define	FCLOOP_HW_QUEUES		4
@@ -1358,6 +1353,7 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
 	struct nvme_fc_port_info pinfo;
 	int ret;
 
+	/* nport ref get: rport */
 	nport = fcloop_alloc_nport(buf, count, true);
 	if (!nport)
 		return -EIO;
@@ -1375,6 +1371,9 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
 		return ret;
 	}
 
+	/* nport ref get: remoteport */
+	fcloop_nport_get(nport);
+
 	/* success */
 	rport = remoteport->private;
 	rport->remoteport = remoteport;
@@ -1403,16 +1402,27 @@ __unlink_remote_port(struct fcloop_nport *nport)
 		nport->tport->remoteport = NULL;
 	nport->rport = NULL;
 
+	/* nport ref put: rport */
+	fcloop_nport_put(nport);
+
 	return rport;
 }
 
 static int
 __remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
 {
-	if (!rport)
-		return -EALREADY;
+	int ret;
 
-	return nvme_fc_unregister_remoteport(rport->remoteport);
+	if (!rport) {
+		ret = -EALREADY;
+		goto out;
+	}
+
+	ret = nvme_fc_unregister_remoteport(rport->remoteport);
+out:
+	/* nport ref put: remoteport */
+	fcloop_nport_put(nport);
+	return ret;
 }
 
 static ssize_t
@@ -1434,6 +1444,9 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
 	list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
 		if (tmpport->node_name == nodename &&
 		    tmpport->port_name == portname && tmpport->rport) {
+
+			if (!fcloop_nport_get(tmpport))
+				break;
 			nport = tmpport;
 			rport = __unlink_remote_port(nport);
 			break;
@@ -1447,6 +1460,8 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
 
 	ret = __remoteport_unreg(nport, rport);
 
+	fcloop_nport_put(nport);
+
 	return ret ? ret : count;
 }
 
@@ -1460,6 +1475,7 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
 	struct nvmet_fc_port_info tinfo;
 	int ret;
 
+	/* nport ref get: tport */
 	nport = fcloop_alloc_nport(buf, count, false);
 	if (!nport)
 		return -EIO;
@@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
 		return ret;
 	}
 
+	/* nport ref get: targetport */
+	fcloop_nport_get(nport);
+
 	/* success */
 	tport = targetport->private;
 	tport->targetport = targetport;
@@ -1501,16 +1520,27 @@ __unlink_target_port(struct fcloop_nport *nport)
 		nport->rport->targetport = NULL;
 	nport->tport = NULL;
 
+	/* nport ref put: tport */
+	fcloop_nport_put(nport);
+
 	return tport;
 }
 
 static int
 __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
 {
-	if (!tport)
-		return -EALREADY;
+	int ret;
 
-	return nvmet_fc_unregister_targetport(tport->targetport);
+	if (!tport) {
+		ret = -EALREADY;
+		goto out;
+	}
+
+	ret = nvmet_fc_unregister_targetport(tport->targetport);
+out:
+	/* nport ref put: targetport */
+	fcloop_nport_put(nport);
+	return ret;
 }
 
 static ssize_t
@@ -1532,6 +1562,9 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
 	list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
 		if (tmpport->node_name == nodename &&
 		    tmpport->port_name == portname && tmpport->tport) {
+
+			if (!fcloop_nport_get(tmpport))
+				break;
 			nport = tmpport;
 			tport = __unlink_target_port(nport);
 			break;
@@ -1545,6 +1578,8 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
 
 	ret = __targetport_unreg(nport, tport);
 
+	fcloop_nport_put(nport);
+
 	return ret ? ret : count;
 }
 

-- 
2.48.1

Re: [PATCH 04/11] nvmet-fcloop: track ref counts for nports

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> A nport object is always used in association with targerport,
> remoteport, tport and rport objects. Add explicit references for any of
> the associated object. This makes the unreliable reference updates in
> the two callback fcloop_targetport_delete and fcloop_remoteport_delete
> obsolete.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 57 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index de1963c34bd88d0335f70de569565740fd395a0a..80693705c069dd114b2d4f15d0482dd2d713a273 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1071,16 +1071,11 @@ fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
>   	struct fcloop_rport *rport = remoteport->private;
>   
>   	flush_work(&rport->ls_work);
> -	fcloop_nport_put(rport->nport);
>   }
>   
>   static void
>   fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
>   {
> -	struct fcloop_tport *tport = targetport->private;
> -
> -	flush_work(&tport->ls_work);
> -	fcloop_nport_put(tport->nport);
>   }
>   
Errm. Isn't this function empty now? Can't it be remove altogether?

>   #define	FCLOOP_HW_QUEUES		4
> @@ -1358,6 +1353,7 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
>   	struct nvme_fc_port_info pinfo;
>   	int ret;
>   
> +	/* nport ref get: rport */
>   	nport = fcloop_alloc_nport(buf, count, true);
>   	if (!nport)
>   		return -EIO;
> @@ -1375,6 +1371,9 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
>   		return ret;
>   	}
>   
> +	/* nport ref get: remoteport */
> +	fcloop_nport_get(nport);
> +
>   	/* success */
>   	rport = remoteport->private;
>   	rport->remoteport = remoteport;
> @@ -1403,16 +1402,27 @@ __unlink_remote_port(struct fcloop_nport *nport)
>   		nport->tport->remoteport = NULL;
>   	nport->rport = NULL;
>   
> +	/* nport ref put: rport */
> +	fcloop_nport_put(nport);
> +
>   	return rport;
>   }
>   
>   static int
>   __remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
>   {
> -	if (!rport)
> -		return -EALREADY;
> +	int ret;
>   
> -	return nvme_fc_unregister_remoteport(rport->remoteport);
> +	if (!rport) {
> +		ret = -EALREADY;
> +		goto out;
> +	}
> +
> +	ret = nvme_fc_unregister_remoteport(rport->remoteport);
> +out:
> +	/* nport ref put: remoteport */
> +	fcloop_nport_put(nport);
> +	return ret;
>   }
>   
>   static ssize_t
> @@ -1434,6 +1444,9 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>   	list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
>   		if (tmpport->node_name == nodename &&
>   		    tmpport->port_name == portname && tmpport->rport) {
> +
> +			if (!fcloop_nport_get(tmpport))
> +				break;
>   			nport = tmpport;
>   			rport = __unlink_remote_port(nport);
>   			break;
> @@ -1447,6 +1460,8 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>   
>   	ret = __remoteport_unreg(nport, rport);
>   
> +	fcloop_nport_put(nport);
> +
>   	return ret ? ret : count;
>   }
>   
> @@ -1460,6 +1475,7 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
>   	struct nvmet_fc_port_info tinfo;
>   	int ret;
>   
> +	/* nport ref get: tport */
>   	nport = fcloop_alloc_nport(buf, count, false);
>   	if (!nport)
>   		return -EIO;
> @@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
>   		return ret;
>   	}
>   
> +	/* nport ref get: targetport */
> +	fcloop_nport_get(nport);
> +

I would rather move it to the end of the function, after we set all the
references. But that's probably personal style...

>   	/* success */
>   	tport = targetport->private;
>   	tport->targetport = targetport;
> @@ -1501,16 +1520,27 @@ __unlink_target_port(struct fcloop_nport *nport)
>   		nport->rport->targetport = NULL;
>   	nport->tport = NULL;
>   
> +	/* nport ref put: tport */
> +	fcloop_nport_put(nport);
> +
>   	return tport;
>   }
>   
>   static int
>   __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
>   {
> -	if (!tport)
> -		return -EALREADY;
> +	int ret;
That's iffy.
Q1: How can you end up with a NULL tport?
Q2: Why do we have _two_ arguments? Can't we use 'nport->tport'?
Q3: What do you do when tport is valid and tport != nport->tport?

>   
> -	return nvmet_fc_unregister_targetport(tport->targetport);
> +	if (!tport) {
> +		ret = -EALREADY;
> +		goto out;
> +	}
> +
> +	ret = nvmet_fc_unregister_targetport(tport->targetport);
> +out:
> +	/* nport ref put: targetport */
> +	fcloop_nport_put(nport);
> +	return ret;
>   }
>   
>   static ssize_t
> @@ -1532,6 +1562,9 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>   	list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
>   		if (tmpport->node_name == nodename &&
>   		    tmpport->port_name == portname && tmpport->tport) {
> +
> +			if (!fcloop_nport_get(tmpport))
> +				break;
>   			nport = tmpport;
>   			tport = __unlink_target_port(nport);
>   			break;
> @@ -1545,6 +1578,8 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>   
>   	ret = __targetport_unreg(nport, tport);
>   
> +	fcloop_nport_put(nport);
> +
>   	return ret ? ret : count;
>   }
>   
> 
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 04/11] nvmet-fcloop: track ref counts for nports

[Daniel Wagner]

On Fri, Feb 28, 2025 at 08:19:19AM +0100, Hannes Reinecke wrote:
> >   static void
> >   fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
> >   {
> > -	struct fcloop_tport *tport = targetport->private;
> > -
> > -	flush_work(&tport->ls_work);
> > -	fcloop_nport_put(tport->nport);
> >   }
> Errm. Isn't this function empty now? Can't it be remove altogether?

Sure, I'll get rid of it.

> >   	nport = fcloop_alloc_nport(buf, count, false);
> >   	if (!nport)
> >   		return -EIO;
> > @@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> >   		return ret;
> >   	}
> > +	/* nport ref get: targetport */
> > +	fcloop_nport_get(nport);
> > +
> 
> I would rather move it to the end of the function, after we set all the
> references. But that's probably personal style...

I don't mind, During debugging I started to move the get/put function
where the pointers are updated, to highlight why we do the get/put. But
that was still pretty hard to find the leaks. Eventually I added some
debugging patches which annotated the call. The left over of these debug
patches are the 'nport ref get: transport' comments (*). Now that I am
confident that all is in balance, there is no reason not to make the
code more streamlined.

(*) Would something like this here be a no go? It really helps when
trying to debug the inbalance:

+static void __nvmet_fc_tgtport_put(struct nvmet_fc_tgtport *tgtport);
+static int __nvmet_fc_tgtport_get(struct nvmet_fc_tgtport *tgtport);
+
+#if 1
+#define nvmet_fc_tgtport_put(p)						\
+({									\
+	pr_info("nvmet_fc_tgtport_put: %px %d %s:%d\n",			\
+		p, atomic_read(&p->ref.refcount.refs),			\
+		__func__, __LINE__);					\
+	__nvmet_fc_tgtport_put(p);					\
+})
+
+#define nvmet_fc_tgtport_get(p)						\
+({									\
+	int ___r = __nvmet_fc_tgtport_get(p);				\
+									\
+	pr_info("nvmet_fc_tgtport_get: %px %d %s:%d\n",			\
+		p, atomic_read(&p->ref.refcount.refs),			\
+		__func__, __LINE__);					\
+	___r;								\
+})
+#else
+#define nvmet_fc_tgtport_put(p) __nvmet_fc_tgtport_put(p)
+#define nvmet_fc_tgtport_get(p) __nvmet_fc_tgtport_get(p)
+#endif

Re: [PATCH 04/11] nvmet-fcloop: track ref counts for nports

[Daniel Wagner]

On Fri, Feb 28, 2025 at 08:19:19AM +0100, Hannes Reinecke wrote:
> >   static int
> >   __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
> >   {
> > -	if (!tport)
> > -		return -EALREADY;
> > +	int ret;
> That's iffy.
> Q1: How can you end up with a NULL tport?

The existing code doesn't get the life time right. I don't think it's
necessary anymore. This gets removed in the next two patches.

> Q2: Why do we have _two_ arguments? Can't we use 'nport->tport'?

This is addressed when the rport and tport get their own ref counting
(next two patches).

> Q3: What do you do when tport is valid and tport != nport->tport?

That is not going to happen after the full series.

I've tried to keep the changes logically separated. This patch here is
only updating the nport ref counters. It took a long time to get these
changes into a readable state. I really would like to avoid doing to
many things in one patch.

[PATCH 05/11] nvmet-fcloop: track tport with ref counting

[Daniel Wagner]

The tport object is created via nvmet_fc_register_targetport and freed
via nvmet_fc_unregister_targetport. That means after the port is
unregistered nothing should use it. Thus ensure with refcounting
that there is no user left before the unregister step.

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

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 80693705c069dd114b2d4f15d0482dd2d713a273..2269b4d20af2ef9bb423617b94a5f5326ea124bd 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -233,8 +233,12 @@ struct fcloop_tport {
 	spinlock_t			lock;
 	struct list_head		ls_list;
 	struct work_struct		ls_work;
+	struct kref			ref;
 };
 
+static int fcloop_tport_get(struct fcloop_tport *tport);
+static void fcloop_tport_put(struct fcloop_tport *tport);
+
 struct fcloop_nport {
 	struct fcloop_rport *rport;
 	struct fcloop_tport *tport;
@@ -426,6 +430,7 @@ fcloop_tport_lsrqst_work(struct work_struct *work)
 		spin_lock(&tport->lock);
 	}
 	spin_unlock(&tport->lock);
+	fcloop_tport_put(tport);
 }
 
 static int
@@ -444,12 +449,16 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
 	tls_req->lsreq = lsreq;
 	INIT_LIST_HEAD(&tls_req->ls_list);
 
+	if (!tport)
+		return -ECONNABORTED;
+
 	if (!tport->remoteport) {
 		tls_req->status = -ECONNREFUSED;
 		spin_lock(&tport->lock);
 		list_add_tail(&tls_req->ls_list, &tport->ls_list);
 		spin_unlock(&tport->lock);
-		queue_work(nvmet_wq, &tport->ls_work);
+		if (queue_work(nvmet_wq, &tport->ls_work))
+			fcloop_tport_get(tport);
 		return ret;
 	}
 
@@ -481,7 +490,8 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
 		spin_lock(&tport->lock);
 		list_add_tail(&tport->ls_list, &tls_req->ls_list);
 		spin_unlock(&tport->lock);
-		queue_work(nvmet_wq, &tport->ls_work);
+		if (queue_work(nvmet_wq, &tport->ls_work))
+			fcloop_tport_get(tport);
 	}
 
 	return 0;
@@ -1496,6 +1506,8 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
 
 	/* success */
 	tport = targetport->private;
+	kref_init(&tport->ref);
+
 	tport->targetport = targetport;
 	tport->remoteport = (nport->rport) ?  nport->rport->remoteport : NULL;
 	if (nport->rport)
@@ -1526,21 +1538,30 @@ __unlink_target_port(struct fcloop_nport *nport)
 	return tport;
 }
 
-static int
-__targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
+static void
+fcloop_targetport_unreg(struct kref *ref)
 {
-	int ret;
+	struct fcloop_tport *tport =
+		container_of(ref, struct fcloop_tport, ref);
+	struct fcloop_nport *nport;
 
-	if (!tport) {
-		ret = -EALREADY;
-		goto out;
-	}
+	nport = tport->nport;
+	nvmet_fc_unregister_targetport(tport->targetport);
 
-	ret = nvmet_fc_unregister_targetport(tport->targetport);
-out:
 	/* nport ref put: targetport */
 	fcloop_nport_put(nport);
-	return ret;
+}
+
+static int
+fcloop_tport_get(struct fcloop_tport *tport)
+{
+	return kref_get_unless_zero(&tport->ref);
+}
+
+static void
+fcloop_tport_put(struct fcloop_tport *tport)
+{
+	kref_put(&tport->ref, fcloop_targetport_unreg);
 }
 
 static ssize_t
@@ -1576,8 +1597,7 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
 	if (!nport)
 		return -ENOENT;
 
-	ret = __targetport_unreg(nport, tport);
-
+	fcloop_tport_put(tport);
 	fcloop_nport_put(nport);
 
 	return ret ? ret : count;
@@ -1696,9 +1716,7 @@ static void __exit fcloop_exit(void)
 
 		spin_unlock_irqrestore(&fcloop_lock, flags);
 
-		ret = __targetport_unreg(nport, tport);
-		if (ret)
-			pr_warn("%s: Failed deleting target port\n", __func__);
+		fcloop_tport_put(tport);
 
 		ret = __remoteport_unreg(nport, rport);
 		if (ret)

-- 
2.48.1

Re: [PATCH 05/11] nvmet-fcloop: track tport with ref counting

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> The tport object is created via nvmet_fc_register_targetport and freed
> via nvmet_fc_unregister_targetport. That means after the port is
> unregistered nothing should use it. Thus ensure with refcounting
> that there is no user left before the unregister step.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 52 +++++++++++++++++++++++++++++---------------
>   1 file changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 80693705c069dd114b2d4f15d0482dd2d713a273..2269b4d20af2ef9bb423617b94a5f5326ea124bd 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -233,8 +233,12 @@ struct fcloop_tport {
>   	spinlock_t			lock;
>   	struct list_head		ls_list;
>   	struct work_struct		ls_work;
> +	struct kref			ref;
>   };
>   
> +static int fcloop_tport_get(struct fcloop_tport *tport);
> +static void fcloop_tport_put(struct fcloop_tport *tport);
> +
>   struct fcloop_nport {
>   	struct fcloop_rport *rport;
>   	struct fcloop_tport *tport;
> @@ -426,6 +430,7 @@ fcloop_tport_lsrqst_work(struct work_struct *work)
>   		spin_lock(&tport->lock);
>   	}
>   	spin_unlock(&tport->lock);
> +	fcloop_tport_put(tport);
>   }
>   
>   static int
> @@ -444,12 +449,16 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
>   	tls_req->lsreq = lsreq;
>   	INIT_LIST_HEAD(&tls_req->ls_list);
>   
> +	if (!tport)
> +		return -ECONNABORTED;
> +
>   	if (!tport->remoteport) {
>   		tls_req->status = -ECONNREFUSED;
>   		spin_lock(&tport->lock);
>   		list_add_tail(&tls_req->ls_list, &tport->ls_list);
>   		spin_unlock(&tport->lock);
> -		queue_work(nvmet_wq, &tport->ls_work);
> +		if (queue_work(nvmet_wq, &tport->ls_work))
> +			fcloop_tport_get(tport);

Don't you need to remove the 'tls_req' from the list, too, seeing that
it'll never be transferred?

>   		return ret;
>   	}
>   
> @@ -481,7 +490,8 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
>   		spin_lock(&tport->lock);
>   		list_add_tail(&tport->ls_list, &tls_req->ls_list);
>   		spin_unlock(&tport->lock);
> -		queue_work(nvmet_wq, &tport->ls_work);
> +		if (queue_work(nvmet_wq, &tport->ls_work))
> +			fcloop_tport_get(tport);

Same argument here.

>   	}
>   
>   	return 0;
> @@ -1496,6 +1506,8 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
>   
>   	/* success */
>   	tport = targetport->private;
> +	kref_init(&tport->ref);
> +
>   	tport->targetport = targetport;
>   	tport->remoteport = (nport->rport) ?  nport->rport->remoteport : NULL;
>   	if (nport->rport)
> @@ -1526,21 +1538,30 @@ __unlink_target_port(struct fcloop_nport *nport)
>   	return tport;
>   }
>   
> -static int
> -__targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
> +static void
> +fcloop_targetport_unreg(struct kref *ref)
>   {
> -	int ret;
> +	struct fcloop_tport *tport =
> +		container_of(ref, struct fcloop_tport, ref);
> +	struct fcloop_nport *nport;
>   
> -	if (!tport) {
> -		ret = -EALREADY;
> -		goto out;
> -	}
> +	nport = tport->nport;
> +	nvmet_fc_unregister_targetport(tport->targetport);
>   
> -	ret = nvmet_fc_unregister_targetport(tport->targetport);
> -out:
>   	/* nport ref put: targetport */
>   	fcloop_nport_put(nport);
> -	return ret;
> +}
> +
> +static int
> +fcloop_tport_get(struct fcloop_tport *tport)
> +{
> +	return kref_get_unless_zero(&tport->ref);
> +}
> +
> +static void
> +fcloop_tport_put(struct fcloop_tport *tport)
> +{
> +	kref_put(&tport->ref, fcloop_targetport_unreg);
>   }
>   
>   static ssize_t
> @@ -1576,8 +1597,7 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>   	if (!nport)
>   		return -ENOENT;
>   
> -	ret = __targetport_unreg(nport, tport);
> -
> +	fcloop_tport_put(tport);
>   	fcloop_nport_put(nport);
>   
Hmm. Are we sure that the 'tport' reference is always > 1 here? 
Otherwise we'll end up with a funny business when the tport is deleted
yet the nport still has a reference ..

>   	return ret ? ret : count;
> @@ -1696,9 +1716,7 @@ static void __exit fcloop_exit(void)
>   
>   		spin_unlock_irqrestore(&fcloop_lock, flags);
>   
> -		ret = __targetport_unreg(nport, tport);
> -		if (ret)
> -			pr_warn("%s: Failed deleting target port\n", __func__);
> +		fcloop_tport_put(tport);
>   
>   		ret = __remoteport_unreg(nport, rport);
>   		if (ret)
> 

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 05/11] nvmet-fcloop: track tport with ref counting

[Daniel Wagner]

On Fri, Feb 28, 2025 at 08:27:40AM +0100, Hannes Reinecke wrote:
> On 2/26/25 19:45, Daniel Wagner wrote:
> > The tport object is created via nvmet_fc_register_targetport and freed
> > via nvmet_fc_unregister_targetport. That means after the port is
> > unregistered nothing should use it. Thus ensure with refcounting
> > that there is no user left before the unregister step.
> > 
> > Signed-off-by: Daniel Wagner <wagi@kernel.org>
> > ---
> >   drivers/nvme/target/fcloop.c | 52 +++++++++++++++++++++++++++++---------------
> >   1 file changed, 35 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> > index 80693705c069dd114b2d4f15d0482dd2d713a273..2269b4d20af2ef9bb423617b94a5f5326ea124bd 100644
> > --- a/drivers/nvme/target/fcloop.c
> > +++ b/drivers/nvme/target/fcloop.c
> > @@ -233,8 +233,12 @@ struct fcloop_tport {
> >   	spinlock_t			lock;
> >   	struct list_head		ls_list;
> >   	struct work_struct		ls_work;
> > +	struct kref			ref;
> >   };
> > +static int fcloop_tport_get(struct fcloop_tport *tport);
> > +static void fcloop_tport_put(struct fcloop_tport *tport);
> > +
> >   struct fcloop_nport {
> >   	struct fcloop_rport *rport;
> >   	struct fcloop_tport *tport;
> > @@ -426,6 +430,7 @@ fcloop_tport_lsrqst_work(struct work_struct *work)
> >   		spin_lock(&tport->lock);
> >   	}
> >   	spin_unlock(&tport->lock);
> > +	fcloop_tport_put(tport);
> >   }
> >   static int
> > @@ -444,12 +449,16 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
> >   	tls_req->lsreq = lsreq;
> >   	INIT_LIST_HEAD(&tls_req->ls_list);
> > +	if (!tport)
> > +		return -ECONNABORTED;
> > +
> >   	if (!tport->remoteport) {
> >   		tls_req->status = -ECONNREFUSED;
> >   		spin_lock(&tport->lock);
> >   		list_add_tail(&tls_req->ls_list, &tport->ls_list);
> >   		spin_unlock(&tport->lock);
> > -		queue_work(nvmet_wq, &tport->ls_work);
> > +		if (queue_work(nvmet_wq, &tport->ls_work))
> > +			fcloop_tport_get(tport);
> 
> Don't you need to remove the 'tls_req' from the list, too, seeing that
> it'll never be transferred?

Good point. I'll add the error handling.

> > @@ -1576,8 +1597,7 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
> >   	if (!nport)
> >   		return -ENOENT;
> > -	ret = __targetport_unreg(nport, tport);
> > -
> > +	fcloop_tport_put(tport);
> >   	fcloop_nport_put(nport);
> Hmm. Are we sure that the 'tport' reference is always > 1 here? Otherwise
> we'll end up with a funny business when the tport is deleted
> yet the nport still has a reference ..

Yes, I am very sure about this. This doesn't mean it needs to stay like
this. The goal here is to avoid changing the existing structure of the
code and only annotate the life time of the different objects with ref
counters, so we get rid of the implicit 'rules' when it's safe to
access a pointer and when not.

Anyway, I really don't mind getting this sorted out eventually, but
please not in this series.

Re: [PATCH 05/11] nvmet-fcloop: track tport with ref counting

[Daniel Wagner]

On Fri, Feb 28, 2025 at 09:30:42AM +0100, Daniel Wagner wrote:
> On Fri, Feb 28, 2025 at 08:27:40AM +0100, Hannes Reinecke wrote:
> > >   	if (!tport->remoteport) {
> > >   		tls_req->status = -ECONNREFUSED;
> > >   		spin_lock(&tport->lock);
> > >   		list_add_tail(&tls_req->ls_list, &tport->ls_list);
> > >   		spin_unlock(&tport->lock);
> > > -		queue_work(nvmet_wq, &tport->ls_work);
> > > +		if (queue_work(nvmet_wq, &tport->ls_work))
> > > +			fcloop_tport_get(tport);
> > 
> > Don't you need to remove the 'tls_req' from the list, too, seeing that
> > it'll never be transferred?
> 
> Good point. I'll add the error handling.

In fact, I think a WARN_ONCE is the better choice here, as the element
should not be on the list (an error in the code) and queue_work will only
return false if the work item is already scheduled.

[PATCH 06/11] nvmet-fcloop: track rport with ref counting

[Daniel Wagner]

The rport object is created via nvme_fc_register_remote and freed
via nvme_fc_unregister_remoteport. That means after the port is
unregistered nothing should use it. Thus ensure with refcounting
that there is no user left before the unregister step.

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

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 2269b4d20af2ef9bb423617b94a5f5326ea124bd..d64f5fba136e13c9e4e545acecd905c31542d442 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -223,8 +223,12 @@ struct fcloop_rport {
 	spinlock_t			lock;
 	struct list_head		ls_list;
 	struct work_struct		ls_work;
+	struct kref			ref;
 };
 
+static int fcloop_rport_get(struct fcloop_rport *rport);
+static void fcloop_rport_put(struct fcloop_rport *rport);
+
 struct fcloop_tport {
 	struct nvmet_fc_target_port	*targetport;
 	struct nvme_fc_remote_port	*remoteport;
@@ -346,6 +350,7 @@ fcloop_rport_lsrqst_work(struct work_struct *work)
 		spin_lock(&rport->lock);
 	}
 	spin_unlock(&rport->lock);
+	fcloop_rport_put(rport);
 }
 
 static int
@@ -365,7 +370,8 @@ fcloop_h2t_ls_req(struct nvme_fc_local_port *localport,
 		spin_lock(&rport->lock);
 		list_add_tail(&tls_req->ls_list, &rport->ls_list);
 		spin_unlock(&rport->lock);
-		queue_work(nvmet_wq, &rport->ls_work);
+		if (queue_work(nvmet_wq, &rport->ls_work))
+			fcloop_rport_get(rport);
 		return ret;
 	}
 
@@ -398,7 +404,8 @@ fcloop_h2t_xmt_ls_rsp(struct nvmet_fc_target_port *targetport,
 		spin_lock(&rport->lock);
 		list_add_tail(&tls_req->ls_list, &rport->ls_list);
 		spin_unlock(&rport->lock);
-		queue_work(nvmet_wq, &rport->ls_work);
+		if (queue_work(nvmet_wq, &rport->ls_work))
+			fcloop_rport_get(rport);
 	}
 
 	return 0;
@@ -1078,9 +1085,6 @@ fcloop_localport_delete(struct nvme_fc_local_port *localport)
 static void
 fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
 {
-	struct fcloop_rport *rport = remoteport->private;
-
-	flush_work(&rport->ls_work);
 }
 
 static void
@@ -1386,6 +1390,8 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
 
 	/* success */
 	rport = remoteport->private;
+	kref_init(&rport->ref);
+
 	rport->remoteport = remoteport;
 	rport->targetport = (nport->tport) ?  nport->tport->targetport : NULL;
 	if (nport->tport) {
@@ -1418,21 +1424,30 @@ __unlink_remote_port(struct fcloop_nport *nport)
 	return rport;
 }
 
-static int
-__remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
+static void
+fcloop_remoteport_unreg(struct kref *ref)
 {
-	int ret;
+	struct fcloop_rport *rport =
+		container_of(ref, struct fcloop_rport, ref);
+	struct fcloop_nport *nport;
 
-	if (!rport) {
-		ret = -EALREADY;
-		goto out;
-	}
+	nport = rport->nport;
+	nvme_fc_unregister_remoteport(rport->remoteport);
 
-	ret = nvme_fc_unregister_remoteport(rport->remoteport);
-out:
 	/* nport ref put: remoteport */
 	fcloop_nport_put(nport);
-	return ret;
+}
+
+static int
+fcloop_rport_get(struct fcloop_rport *rport)
+{
+	return kref_get_unless_zero(&rport->ref);
+}
+
+static void
+fcloop_rport_put(struct fcloop_rport *rport)
+{
+	kref_put(&rport->ref, fcloop_remoteport_unreg);
 }
 
 static ssize_t
@@ -1468,8 +1483,7 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
 	if (!nport)
 		return -ENOENT;
 
-	ret = __remoteport_unreg(nport, rport);
-
+	fcloop_rport_put(rport);
 	fcloop_nport_put(nport);
 
 	return ret ? ret : count;
@@ -1717,10 +1731,7 @@ static void __exit fcloop_exit(void)
 		spin_unlock_irqrestore(&fcloop_lock, flags);
 
 		fcloop_tport_put(tport);
-
-		ret = __remoteport_unreg(nport, rport);
-		if (ret)
-			pr_warn("%s: Failed deleting remote port\n", __func__);
+		fcloop_rport_put(rport);
 
 		spin_lock_irqsave(&fcloop_lock, flags);
 	}

-- 
2.48.1

Re: [PATCH 06/11] nvmet-fcloop: track rport with ref counting

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> The rport object is created via nvme_fc_register_remote and freed
> via nvme_fc_unregister_remoteport. That means after the port is
> unregistered nothing should use it. Thus ensure with refcounting
> that there is no user left before the unregister step.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 53 ++++++++++++++++++++++++++------------------
>   1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 2269b4d20af2ef9bb423617b94a5f5326ea124bd..d64f5fba136e13c9e4e545acecd905c31542d442 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -223,8 +223,12 @@ struct fcloop_rport {
>   	spinlock_t			lock;
>   	struct list_head		ls_list;
>   	struct work_struct		ls_work;
> +	struct kref			ref;
>   };
>   
> +static int fcloop_rport_get(struct fcloop_rport *rport);
> +static void fcloop_rport_put(struct fcloop_rport *rport);
> +
>   struct fcloop_tport {
>   	struct nvmet_fc_target_port	*targetport;
>   	struct nvme_fc_remote_port	*remoteport;
> @@ -346,6 +350,7 @@ fcloop_rport_lsrqst_work(struct work_struct *work)
>   		spin_lock(&rport->lock);
>   	}
>   	spin_unlock(&rport->lock);
> +	fcloop_rport_put(rport);
>   }
>   
>   static int
> @@ -365,7 +370,8 @@ fcloop_h2t_ls_req(struct nvme_fc_local_port *localport,
>   		spin_lock(&rport->lock);
>   		list_add_tail(&tls_req->ls_list, &rport->ls_list);
>   		spin_unlock(&rport->lock);
> -		queue_work(nvmet_wq, &rport->ls_work);
> +		if (queue_work(nvmet_wq, &rport->ls_work))
> +			fcloop_rport_get(rport);

Same argument than previously: don't we need to remove 'tls_req' from 
the list on failure?

>   		return ret;
>   	}
>   
> @@ -398,7 +404,8 @@ fcloop_h2t_xmt_ls_rsp(struct nvmet_fc_target_port *targetport,
>   		spin_lock(&rport->lock);
>   		list_add_tail(&tls_req->ls_list, &rport->ls_list);
>   		spin_unlock(&rport->lock);
> -		queue_work(nvmet_wq, &rport->ls_work);
> +		if (queue_work(nvmet_wq, &rport->ls_work))
> +			fcloop_rport_get(rport);

Same here.

>   	}
>   
>   	return 0;
> @@ -1078,9 +1085,6 @@ fcloop_localport_delete(struct nvme_fc_local_port *localport)
>   static void
>   fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
>   {
> -	struct fcloop_rport *rport = remoteport->private;
> -
> -	flush_work(&rport->ls_work);
>   }

Empty function.

>   
>   static void
> @@ -1386,6 +1390,8 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
>   
>   	/* success */
>   	rport = remoteport->private;
> +	kref_init(&rport->ref);
> +
>   	rport->remoteport = remoteport;
>   	rport->targetport = (nport->tport) ?  nport->tport->targetport : NULL;
>   	if (nport->tport) {
> @@ -1418,21 +1424,30 @@ __unlink_remote_port(struct fcloop_nport *nport)
>   	return rport;
>   }
>   
> -static int
> -__remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
> +static void
> +fcloop_remoteport_unreg(struct kref *ref)
>   {
> -	int ret;
> +	struct fcloop_rport *rport =
> +		container_of(ref, struct fcloop_rport, ref);
> +	struct fcloop_nport *nport;
>   
> -	if (!rport) {
> -		ret = -EALREADY;
> -		goto out;
> -	}
> +	nport = rport->nport;
> +	nvme_fc_unregister_remoteport(rport->remoteport);
>   
> -	ret = nvme_fc_unregister_remoteport(rport->remoteport);
> -out:
>   	/* nport ref put: remoteport */
>   	fcloop_nport_put(nport);
> -	return ret;
> +}
> +
> +static int
> +fcloop_rport_get(struct fcloop_rport *rport)
> +{
> +	return kref_get_unless_zero(&rport->ref);
> +}
> +
> +static void
> +fcloop_rport_put(struct fcloop_rport *rport)
> +{
> +	kref_put(&rport->ref, fcloop_remoteport_unreg);
>   }
>   
>   static ssize_t
> @@ -1468,8 +1483,7 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>   	if (!nport)
>   		return -ENOENT;
>   
> -	ret = __remoteport_unreg(nport, rport);
> -
> +	fcloop_rport_put(rport);
>   	fcloop_nport_put(nport);
>   
>   	return ret ? ret : count;
> @@ -1717,10 +1731,7 @@ static void __exit fcloop_exit(void)
>   		spin_unlock_irqrestore(&fcloop_lock, flags);
>   
>   		fcloop_tport_put(tport);
> -
> -		ret = __remoteport_unreg(nport, rport);
> -		if (ret)
> -			pr_warn("%s: Failed deleting remote port\n", __func__);
> +		fcloop_rport_put(rport);
>   
>   		spin_lock_irqsave(&fcloop_lock, flags);
>   	}
> 

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 07/11] nvmet-fc: update tgtport ref per assoc

[Daniel Wagner]

We need to take for each unique association a reference.
nvmet_fc_alloc_hostport for each newly created association.

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

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 3ef4beacde3257147a59321e8f13451326302de0..b807b4c05cac7fe4764df3df76f8fa50f4bab6ba 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1157,6 +1157,7 @@ nvmet_fc_alloc_target_assoc(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 		goto out_ida;
 
 	assoc->tgtport = tgtport;
+	nvmet_fc_tgtport_get(tgtport);
 	assoc->a_id = idx;
 	INIT_LIST_HEAD(&assoc->a_list);
 	kref_init(&assoc->ref);
@@ -1258,6 +1259,8 @@ nvmet_fc_delete_target_assoc(struct nvmet_fc_tgt_assoc *assoc)
 	dev_info(tgtport->dev,
 		"{%d:%d} Association deleted\n",
 		tgtport->fc_target_port.port_num, assoc->a_id);
+
+	nvmet_fc_tgtport_put(tgtport);
 }
 
 static struct nvmet_fc_tgt_assoc *

-- 
2.48.1

Re: [PATCH 07/11] nvmet-fc: update tgtport ref per assoc

[Hannes Reinecke]

On 2/26/25 19:45, Daniel Wagner wrote:
> We need to take for each unique association a reference.
> nvmet_fc_alloc_hostport for each newly created association.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 3ef4beacde3257147a59321e8f13451326302de0..b807b4c05cac7fe4764df3df76f8fa50f4bab6ba 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1157,6 +1157,7 @@ nvmet_fc_alloc_target_assoc(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   		goto out_ida;
>   
>   	assoc->tgtport = tgtport;
> +	nvmet_fc_tgtport_get(tgtport);
>   	assoc->a_id = idx;
>   	INIT_LIST_HEAD(&assoc->a_list);
>   	kref_init(&assoc->ref);
> @@ -1258,6 +1259,8 @@ nvmet_fc_delete_target_assoc(struct nvmet_fc_tgt_assoc *assoc)
>   	dev_info(tgtport->dev,
>   		"{%d:%d} Association deleted\n",
>   		tgtport->fc_target_port.port_num, assoc->a_id);
> +
> +	nvmet_fc_tgtport_put(tgtport);
>   }
>   
>   static struct nvmet_fc_tgt_assoc *
> 
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 08/11] nvmet-fc: take tgtport reference only once

[Daniel Wagner]

The reference counting code can be simplified. Instead taking a tgtport
refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
if not a new hostport object is allocated, only take it when a new
hostport object is allocated.

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

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index b807b4c05cac7fe4764df3df76f8fa50f4bab6ba..391917b4ce0115dbc0ad99d1fb363b1af6ee0685 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1046,29 +1046,16 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 	if (!hosthandle)
 		return NULL;
 
-	/*
-	 * take reference for what will be the newly allocated hostport if
-	 * we end up using a new allocation
-	 */
-	if (!nvmet_fc_tgtport_get(tgtport))
-		return ERR_PTR(-EINVAL);
-
 	spin_lock_irqsave(&tgtport->lock, flags);
 	match = nvmet_fc_match_hostport(tgtport, hosthandle);
 	spin_unlock_irqrestore(&tgtport->lock, flags);
 
-	if (match) {
-		/* no new allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
+	if (match)
 		return match;
-	}
 
 	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
-	if (!newhost) {
-		/* no new allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
+	if (!newhost)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	spin_lock_irqsave(&tgtport->lock, flags);
 	match = nvmet_fc_match_hostport(tgtport, hosthandle);
@@ -1077,6 +1064,7 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 		kfree(newhost);
 		newhost = match;
 	} else {
+		nvmet_fc_tgtport_get(tgtport);
 		newhost->tgtport = tgtport;
 		newhost->hosthandle = hosthandle;
 		INIT_LIST_HEAD(&newhost->host_list);

-- 
2.48.1

Re: [PATCH 08/11] nvmet-fc: take tgtport reference only once

[Hannes Reinecke]

On 2/26/25 19:46, Daniel Wagner wrote:
> The reference counting code can be simplified. Instead taking a tgtport
> refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
> if not a new hostport object is allocated, only take it when a new
> hostport object is allocated.
> 
Can it really?
Main point of this operation is that 'tgtport' isn't going away during 
while we're figuring out whether we need it.

With this patch it means that

> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index b807b4c05cac7fe4764df3df76f8fa50f4bab6ba..391917b4ce0115dbc0ad99d1fb363b1af6ee0685 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1046,29 +1046,16 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   	if (!hosthandle)
>   		return NULL;
>   
> -	/*
> -	 * take reference for what will be the newly allocated hostport if
> -	 * we end up using a new allocation
> -	 */
> -	if (!nvmet_fc_tgtport_get(tgtport))
> -		return ERR_PTR(-EINVAL);
> -
 >   	spin_lock_irqsave(&tgtport->lock, flags);>   	match = 
nvmet_fc_match_hostport(tgtport, hosthandle);
>   	spin_unlock_irqrestore(&tgtport->lock, flags);
>   

'tgtport' might be invalid here, causing a crash when taking the lock.

> -	if (match) {
> -		/* no new allocation - release reference */
> -		nvmet_fc_tgtport_put(tgtport);
> +	if (match)
>   		return match;
> -	}
>   
>   	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
> -	if (!newhost) {
> -		/* no new allocation - release reference */
> -		nvmet_fc_tgtport_put(tgtport);
> +	if (!newhost)
>   		return ERR_PTR(-ENOMEM);
> -	}
>   
>   	spin_lock_irqsave(&tgtport->lock, flags);
>   	match = nvmet_fc_match_hostport(tgtport, hosthandle);
> @@ -1077,6 +1064,7 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   		kfree(newhost);
>   		newhost = match;
>   	} else {
> +		nvmet_fc_tgtport_get(tgtport);
>   		newhost->tgtport = tgtport;
>   		newhost->hosthandle = hosthandle;
>   		INIT_LIST_HEAD(&newhost->host_list);
> 

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 08/11] nvmet-fc: take tgtport reference only once

[Daniel Wagner]

On Fri, Feb 28, 2025 at 08:34:51AM +0100, Hannes Reinecke wrote:
> On 2/26/25 19:46, Daniel Wagner wrote:
> > The reference counting code can be simplified. Instead taking a tgtport
> > refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
> > if not a new hostport object is allocated, only take it when a new
> > hostport object is allocated.
> > 
> Can it really?
> Main point of this operation is that 'tgtport' isn't going away during while
> we're figuring out whether we need it.
> 
> With this patch it means that

The tgtport is not going away. nvmet_fc_alloc_hostport can only be
called with reference on tgtport being hold:

nvmet_fc_rcv_ls_req
  nvmet_fc_tgtport_get(tgtport)
  nvmet_fc_handle_ls_rqst_work
    nvmet_fc_handle_ls_rqst
      nvmet_fc_ls_create_association
        nvmet_fc_alloc_target_assoc
          nvmet_fc_alloc_hostport

The goal with this patch here is to make it simpler to read where we
take a ref. IMO, there is not really anything gained by the existing
logic, though I agree it's not obvious that the tgtport is not going
away. Would it be okay to add a comment?

[PATCH 09/11] nvmet-fc: free pending reqs on tgtport unregister

[Daniel Wagner]

When nvmet_fc_unregister_targetport is called by the LLDD, it's not
possible to communicate with the host, thus all pending request will not
be process. Thus explicitly free them.

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

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 391917b4ce0115dbc0ad99d1fb363b1af6ee0685..a5f7cb18ac9f49e41626e1c9a031c3cc830af9ba 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1610,6 +1610,37 @@ nvmet_fc_delete_ctrl(struct nvmet_ctrl *ctrl)
 	spin_unlock_irqrestore(&nvmet_fc_tgtlock, flags);
 }
 
+static void
+nvmet_fc_free_pending_reqs(struct nvmet_fc_tgtport *tgtport)
+{
+	struct nvmet_fc_ls_req_op *lsop;
+	struct nvmefc_ls_req *lsreq;
+
+	/*
+	 * After this point the connection is lost and thus any pending
+	 * request can't be processed by the normal completion path. This
+	 * is likely a request from nvmet_fc_send_ls_req_async.
+	 */
+	for (;;) {
+		lsop = list_first_entry_or_null(&tgtport->ls_req_list,
+						struct nvmet_fc_ls_req_op,
+						lsreq_list);
+		if (!lsop)
+			break;
+
+		list_del(&lsop->lsreq_list);
+
+		if (!lsop->req_queued)
+			continue;
+
+		lsreq = &lsop->ls_req;
+		fc_dma_unmap_single(tgtport->dev, lsreq->rqstdma,
+				    (lsreq->rqstlen + lsreq->rsplen),
+				    DMA_BIDIRECTIONAL);
+		nvmet_fc_tgtport_put(tgtport);
+	}
+}
+
 /**
  * nvmet_fc_unregister_targetport - transport entry point called by an
  *                              LLDD to deregister/remove a previously
@@ -1633,13 +1664,7 @@ nvmet_fc_unregister_targetport(struct nvmet_fc_target_port *target_port)
 
 	flush_workqueue(nvmet_wq);
 
-	/*
-	 * should terminate LS's as well. However, LS's will be generated
-	 * at the tail end of association termination, so they likely don't
-	 * exist yet. And even if they did, it's worthwhile to just let
-	 * them finish and targetport ref counting will clean things up.
-	 */
-
+	nvmet_fc_free_pending_reqs(tgtport);
 	nvmet_fc_tgtport_put(tgtport);
 
 	return 0;

-- 
2.48.1

Re: [PATCH 09/11] nvmet-fc: free pending reqs on tgtport unregister

[Hannes Reinecke]

On 2/26/25 19:46, Daniel Wagner wrote:
> When nvmet_fc_unregister_targetport is called by the LLDD, it's not
> possible to communicate with the host, thus all pending request will not
> be process. Thus explicitly free them.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 39 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 391917b4ce0115dbc0ad99d1fb363b1af6ee0685..a5f7cb18ac9f49e41626e1c9a031c3cc830af9ba 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1610,6 +1610,37 @@ nvmet_fc_delete_ctrl(struct nvmet_ctrl *ctrl)
>   	spin_unlock_irqrestore(&nvmet_fc_tgtlock, flags);
>   }
>   
> +static void
> +nvmet_fc_free_pending_reqs(struct nvmet_fc_tgtport *tgtport)
> +{
> +	struct nvmet_fc_ls_req_op *lsop;
> +	struct nvmefc_ls_req *lsreq;
> +
> +	/*
> +	 * After this point the connection is lost and thus any pending
> +	 * request can't be processed by the normal completion path. This
> +	 * is likely a request from nvmet_fc_send_ls_req_async.
> +	 */
> +	for (;;) {
> +		lsop = list_first_entry_or_null(&tgtport->ls_req_list,
> +						struct nvmet_fc_ls_req_op,
> +						lsreq_list);
> +		if (!lsop)
> +			break;
> +
> +		list_del(&lsop->lsreq_list);
> +
> +		if (!lsop->req_queued)
> +			continue;
> +
> +		lsreq = &lsop->ls_req;
> +		fc_dma_unmap_single(tgtport->dev, lsreq->rqstdma,
> +				    (lsreq->rqstlen + lsreq->rsplen),
> +				    DMA_BIDIRECTIONAL);
> +		nvmet_fc_tgtport_put(tgtport);
> +	}
> +}
> +
>   /**
>    * nvmet_fc_unregister_targetport - transport entry point called by an
>    *                              LLDD to deregister/remove a previously
> @@ -1633,13 +1664,7 @@ nvmet_fc_unregister_targetport(struct nvmet_fc_target_port *target_port)
>   
>   	flush_workqueue(nvmet_wq);
>   
> -	/*
> -	 * should terminate LS's as well. However, LS's will be generated
> -	 * at the tail end of association termination, so they likely don't
> -	 * exist yet. And even if they did, it's worthwhile to just let
> -	 * them finish and targetport ref counting will clean things up.
> -	 */
> -
> +	nvmet_fc_free_pending_reqs(tgtport);
>   	nvmet_fc_tgtport_put(tgtport);
>   
>   	return 0;
> 
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 10/11] nvmet-fc: inline nvmet_fc_delete_assoc

[Daniel Wagner]

There is only one user for this helper function, just inline it.

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

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index a5f7cb18ac9f49e41626e1c9a031c3cc830af9ba..fcf191aa1c6bf73e7fb84213bbb7eb76a486734f 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1077,13 +1077,6 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 	return newhost;
 }
 
-static void
-nvmet_fc_delete_assoc(struct nvmet_fc_tgt_assoc *assoc)
-{
-	nvmet_fc_delete_target_assoc(assoc);
-	nvmet_fc_tgt_a_put(assoc);
-}
-
 static void
 nvmet_fc_delete_assoc_work(struct work_struct *work)
 {
@@ -1091,7 +1084,8 @@ nvmet_fc_delete_assoc_work(struct work_struct *work)
 		container_of(work, struct nvmet_fc_tgt_assoc, del_work);
 	struct nvmet_fc_tgtport *tgtport = assoc->tgtport;
 
-	nvmet_fc_delete_assoc(assoc);
+	nvmet_fc_delete_target_assoc(assoc);
+	nvmet_fc_tgt_a_put(assoc);
 	nvmet_fc_tgtport_put(tgtport);
 }
 

-- 
2.48.1

Re: [PATCH 10/11] nvmet-fc: inline nvmet_fc_delete_assoc

[Hannes Reinecke]

On 2/26/25 19:46, Daniel Wagner wrote:
> There is only one user for this helper function, just inline it.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
But maybe a comment is in order.
Other than that:

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 11/11] nvmet-fc: inline nvmet_fc_free_hostport

[Daniel Wagner]

No need for this tiny helper with only user, let's inline it.
And since the hostport ref counter needs to stay in sync, it's not
optional anymore to give back the reference.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/target/fc.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index fcf191aa1c6bf73e7fb84213bbb7eb76a486734f..f42f7c674f69b87a43bf06639a8da60d14a48509 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1009,16 +1009,6 @@ nvmet_fc_hostport_get(struct nvmet_fc_hostport *hostport)
 	return kref_get_unless_zero(&hostport->ref);
 }
 
-static void
-nvmet_fc_free_hostport(struct nvmet_fc_hostport *hostport)
-{
-	/* if LLDD not implemented, leave as NULL */
-	if (!hostport || !hostport->hosthandle)
-		return;
-
-	nvmet_fc_hostport_put(hostport);
-}
-
 static struct nvmet_fc_hostport *
 nvmet_fc_match_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 {
@@ -1187,7 +1177,7 @@ nvmet_fc_target_assoc_free(struct kref *ref)
 	/* Send Disconnect now that all i/o has completed */
 	nvmet_fc_xmt_disconnect_assoc(assoc);
 
-	nvmet_fc_free_hostport(assoc->hostport);
+	nvmet_fc_hostport_put(assoc->hostport);
 	spin_lock_irqsave(&tgtport->lock, flags);
 	oldls = assoc->rcv_disconn;
 	spin_unlock_irqrestore(&tgtport->lock, flags);

-- 
2.48.1

Re: [PATCH 11/11] nvmet-fc: inline nvmet_fc_free_hostport

[Hannes Reinecke]

On 2/26/25 19:46, Daniel Wagner wrote:
> No need for this tiny helper with only user, let's inline it.
> And since the hostport ref counter needs to stay in sync, it's not
> optional anymore to give back the reference.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 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 00/11] nvmet-fcloop: track resources via reference counting

[Daniel Wagner]

On Wed, Feb 26, 2025 at 07:45:52PM +0100, Daniel Wagner wrote:
> static void nvmet_port_subsys_drop_link(struct config_item *parent,
> 		struct config_item *target)
> {
> 	[...]
> found:
> 	list_del(&p->entry);
> 	nvmet_port_del_ctrls(port, subsys);
> 	nvmet_port_disc_changed(port, subsys);   /* XXX triggers the above UAF */
> 
> 	if (list_empty(&port->subsystems))
> 		nvmet_disable_port(port);
> 	up_write(&nvmet_config_sem);
> 	kfree(p);
> }
> 
> The nvmet_port_disc_changed is a bit useless, because these event will
> never be seen by the host. Anyway, more debugging is necessary.

The problem is there is no ref counting for pe->tgtport. And in
nvmet_port_disc_changed needs to take a ref on hostport. I am doing some
more testing and it looks promising. Hopefully this is one of those
famous lost words.