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.1On 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.
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.
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.
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
© 2016 - 2026 Red Hat, Inc.