[PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

nvmf_create_ctrl() owns the fabrics options and frees them whenever
->create_ctrl() returns an error, so a transport must not free them on
its own error paths.  nvme-fc tracks this by testing ctrl->ctrl.opts in
nvme_fc_ctrl_free(), which requires nvme_fc_init_ctrl() to clear that
pointer on every error exit.

The coupling is implicit, and commit 1a9e218195a5 ("nvme: split device
add from initialization") broke it by adding a second error exit.  When
nvme_add_ctrl() fails, nvme_fc_init_ctrl() jumps to out_put_ctrl:, past
the "ctrl->ctrl.opts = NULL" that only sits on the fail_ctrl: path, so
nvme_fc_ctrl_free() frees the options and nvmf_create_ctrl() frees them
a second time:

  BUG: KASAN: slab-use-after-free in nvmf_free_options+0x30/0x190
   nvmf_free_options+0x30/0x190 drivers/nvme/host/fabrics.c:1284
   nvmf_create_ctrl drivers/nvme/host/fabrics.c:1374 [inline]
  Freed by task 5534:
   nvme_fc_ctrl_free drivers/nvme/host/fc.c:2374 [inline]
   nvme_fc_init_ctrl+0xe17/0x1450 drivers/nvme/host/fc.c:3605

nvme_add_ctrl() fails when dev_set_name() cannot allocate, so this is
reachable under memory pressure or fault injection.  Without KASAN the
options are freed twice.

Rather than clear the pointer on the second exit as well, derive
ownership the way nvme-tcp, nvme-rdma and nvme-loop do, from list
membership: their free_ctrl leaves the options alone unless the
controller made it onto the transport list.

The list cannot simply be populated on the success path as it is there.
nvme-fc runs the initial connect synchronously via flush_delayed_work(),
and the controller has to be reachable on rport->ctrl_list for the whole
of it: nvme_fc_unregister_remoteport() needs to find it to signal
connectivity loss, nvme_fc_match_disconn_ls() matches an incoming
Disconnect Association LS against ctrl->association_id, which is only
assigned during that window, nvme_fc_resume_controller() needs it on
remoteport re-registration, and nvme_fc_existing_controller() uses it to
reject a duplicate connect racing the one in flight.

Keep the insertion where it is and add a fail_unlist: label, falling
into fail_ctrl:, for the error paths that run after it.  The earlier
error paths never reach the insertion and keep using fail_ctrl:
directly, so the list is only touched where the controller is actually
on it.

nvme_fc_ctrl_free() cannot use the plain "goto free_ctrl" the other
transports use, because it still has to put_device(), release the rport
reference and free the ida entry for resources taken before the
insertion.  Sample list_empty() under rport->lock instead.

ctrl->ctrl.opts also stays valid for the whole teardown now.  That is
not the bug being fixed, but it removes some fragility around the old
idiom: nvme_free_ctrl() calls nvme_auth_free() before ->free_ctrl(), and
ctrl_max_dhchaps() dereferences ctrl->opts without a NULL check when
ctrl->dhchap_ctxs is set, which nvme-fc permits since NVMF_ALLOWED_OPTS
allows the dhchap options.  The nvme sysfs attributes that dereference
ctrl->opts, such as hostnqn and address, evaluate their is_visible()
test once at device_add() time and stay readable until
cdev_device_del().

Fixes: 1a9e218195a5 ("nvme: split device add from initialization")
Cc: stable@vger.kernel.org
Reported-by: syzbot+f58e57380a6083c4041d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f58e57380a6083c4041d
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/nvme/host/fc.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..7e1794f054d6 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2358,9 +2358,15 @@ nvme_fc_ctrl_free(struct kref *ref)
 	struct nvme_fc_ctrl *ctrl =
 		container_of(ref, struct nvme_fc_ctrl, ref);
 	unsigned long flags;
+	bool owns_opts;
 
-	/* remove from rport list */
+	/*
+	 * Presence on the rport list means nvme_fc_init_ctrl() completed,
+	 * and with it ownership of the fabrics options passed to it. If it
+	 * failed instead, the options still belong to nvmf_create_ctrl().
+	 */
 	spin_lock_irqsave(&ctrl->rport->lock, flags);
+	owns_opts = !list_empty(&ctrl->ctrl_list);
 	list_del(&ctrl->ctrl_list);
 	spin_unlock_irqrestore(&ctrl->rport->lock, flags);
 
@@ -2370,7 +2376,7 @@ nvme_fc_ctrl_free(struct kref *ref)
 	nvme_fc_rport_put(ctrl->rport);
 
 	ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
-	if (ctrl->ctrl.opts)
+	if (owns_opts)
 		nvmf_free_options(ctrl->ctrl.opts);
 	kfree(ctrl);
 }
@@ -3569,14 +3575,14 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: failed to init ctrl state\n", ctrl->cnum);
-		goto fail_ctrl;
+		goto fail_unlist;
 	}
 
 	if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: failed to schedule initial connect\n",
 			ctrl->cnum);
-		goto fail_ctrl;
+		goto fail_unlist;
 	}
 
 	flush_delayed_work(&ctrl->connect_work);
@@ -3587,14 +3593,22 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	return &ctrl->ctrl;
 
+fail_unlist:
+	/*
+	 * Leaving the list hands the options back to nvmf_create_ctrl();
+	 * see nvme_fc_ctrl_free().  Re-init so that list_empty() there
+	 * reports the controller as unlisted.
+	 */
+	spin_lock_irqsave(&rport->lock, flags);
+	list_del_init(&ctrl->ctrl_list);
+	spin_unlock_irqrestore(&rport->lock, flags);
+
 fail_ctrl:
 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
 	cancel_work_sync(&ctrl->ioerr_work);
 	cancel_work_sync(&ctrl->ctrl.reset_work);
 	cancel_delayed_work_sync(&ctrl->connect_work);
 
-	ctrl->ctrl.opts = NULL;
-
 	if (ctrl->ctrl.admin_tagset)
 		nvme_remove_admin_tag_set(&ctrl->ctrl);
 	/* initiate nvme ctrl ref counting teardown */
-- 
2.55.0

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Fri, Aug 14, 2026 at 04:38:34PM +0200, Niklas Cassel wrote:
> ctrl->ctrl.opts also stays valid for the whole teardown now.  That is
> not the bug being fixed, but it removes some fragility around the old
> idiom: nvme_free_ctrl() calls nvme_auth_free() before ->free_ctrl(), and
> ctrl_max_dhchaps() dereferences ctrl->opts without a NULL check when
> ctrl->dhchap_ctxs is set, which nvme-fc permits since NVMF_ALLOWED_OPTS
> allows the dhchap options.  The nvme sysfs attributes that dereference
> ctrl->opts, such as hostnqn and address, evaluate their is_visible()
> test once at device_add() time and stay readable until
> cdev_device_del().

The allocation that ctrl.opts points is freed in this error path, so any
access after that is a use-after-free that needs to be fixed. I think
setting opts to NULL is easier than the proxy locked list check.

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

Hello,

Thanks for the explanation, and for picking this up.

My testing of v1 and v2 never reached this case.  The syzbot config has
"# CONFIG_NVME_HOST_AUTH is not set", and I was not aware that the
dhchap option tokens are compiled out when it is disabled.  A connect
string containing dhchap_secret= is simply rejected, so every run I did
tore down with ctrl->dhchap_ctxs NULL, and there was no path that
dereferenced ctrl->opts.

I rebuilt with CONFIG_NVME_HOST_AUTH=y and swept fail-nth 1..200 over a
connect write containing dhchap_secret=.  You are right:

  clearing opts at out_put_ctrl:   dies at attempt 20
  this patch                       200/200, with 35 of the injections
                                   landing in nvme_add_ctrl()

  Oops: general protection fault, probably for non-canonical address
        0xdffffc0000000008
  KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
  RIP: 0010:nvme_auth_free+0x99/0x450
   nvme_free_ctrl+0x231/0x6c0

The injection for that attempt landed in kobj_map() under cdev_add(), so
this is the nvme_add_ctrl() failure path.  ctrl_max_dhchaps() then
dereferences ctrl->opts at offset 0x40.  Clearing the pointer really does
just trade the double free for this NULL pointer dereference.  Good catch.

Your version also addresses the two things I was unsure about in my
earlier mail: fail_unlist: covers the two exit paths after
list_add_tail(), and checking list_empty() while holding the lock keeps
ida_free(), put_device(), and nvme_fc_rport_put() on the unlisted path.

Dropping my v2 in favour of this one.

Tested-by: Rihyeon Kim <rihyeon8648@gmail.com>

Thanks,
Rihyeon

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Mon, Aug 17, 2026 at 03:18:15PM +0900, Rihyeon Kim wrote:
> Hello,
> 
> Thanks for the explanation, and for picking this up.
> 
> My testing of v1 and v2 never reached this case.  The syzbot config has
> "# CONFIG_NVME_HOST_AUTH is not set", and I was not aware that the
> dhchap option tokens are compiled out when it is disabled.  A connect
> string containing dhchap_secret= is simply rejected, so every run I did
> tore down with ctrl->dhchap_ctxs NULL, and there was no path that
> dereferenced ctrl->opts.
> 
> I rebuilt with CONFIG_NVME_HOST_AUTH=y and swept fail-nth 1..200 over a
> connect write containing dhchap_secret=.  You are right:
> 
>   clearing opts at out_put_ctrl:   dies at attempt 20
>   this patch                       200/200, with 35 of the injections
>                                    landing in nvme_add_ctrl()
> 
>   Oops: general protection fault, probably for non-canonical address
>         0xdffffc0000000008
>   KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
>   RIP: 0010:nvme_auth_free+0x99/0x450
>    nvme_free_ctrl+0x231/0x6c0
> 
> The injection for that attempt landed in kobj_map() under cdev_add(), so
> this is the nvme_add_ctrl() failure path.  ctrl_max_dhchaps() then
> dereferences ctrl->opts at offset 0x40.  Clearing the pointer really does
> just trade the double free for this NULL pointer dereference.  Good catch.

Thanks a lot for verifying that the Sashiko comment about the NULL pointer
dereference in nvme_auth_free() can actually happen, and was not only
theoretical.


> 
> Your version also addresses the two things I was unsure about in my
> earlier mail: fail_unlist: covers the two exit paths after
> list_add_tail(), and checking list_empty() while holding the lock keeps
> ida_free(), put_device(), and nvme_fc_rport_put() on the unlisted path.
> 
> Dropping my v2 in favour of this one.
> 
> Tested-by: Rihyeon Kim <rihyeon8648@gmail.com>

It seems like Keith did prefer your patch, but as you said, your patch does
not avoid a NULL pointer dereference in nvme_auth_free().


My patch should also avoid a potential sysfs NULL pointer dereference as
reported by Sashiko:
https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com

which is currently also only possible for fc.c, because of the
'ctrl->ctrl.opts = NULL;' hack.


Personally, I prefer this patch, since it makes fc.c more similar to rdma.c,
tcp.c, and loop.c, by checking if the controller is on the linked list or not,
rather than setting 'ctrl->ctrl.opts = NULL;' before the teardown is even done.


I agree that this patch adds more code than what we ideally would have wanted,
but I don't see any other way (other than completely refactoring the ownership
model of the controller options - which would require much bigger changes -
and would most likely require changes to both fabrics.c + all the different
transport drivers).


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Mon, Aug 17, 2026 at 05:10:23PM +0200, Niklas Cassel wrote:
> It seems like Keith did prefer your patch, but as you said, your patch does
> not avoid a NULL pointer dereference in nvme_auth_free().

But you're replacing a NULL pointer dereference to a derefence to freed
memory. That should be fixed too, and I'm just saying checking a pointer
for NULL before dereferencing it is more clear than checking if a list
is empty.

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

Hello Keith,

On Wed, Aug 19, 2026 at 09:28:08AM -0600, Keith Busch wrote:
> On Mon, Aug 17, 2026 at 05:10:23PM +0200, Niklas Cassel wrote:
> > It seems like Keith did prefer your patch, but as you said, your patch does
> > not avoid a NULL pointer dereference in nvme_auth_free().
> 
> But you're replacing a NULL pointer dereference to a derefence to freed
> memory. That should be fixed too, and I'm just saying checking a pointer
> for NULL before dereferencing it is more clear than checking if a list
> is empty.

I am not following.

The options is freed in two different places, depending on if the
ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
successful or not.

If the ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
successful, the teardown path will be:
core.c:nvme_free_ctrl(), which calls nvme_auth_free() (which will use
the options), nvme_free_ctrl() will then call ctrl->ops->free_ctrl(ctrl)
which will free the options (since the ops->create_ctrl() call was successful,
the ctrl will be on the linked-list).

If the ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl()
fails, the teardown path will be:
core.c:nvme_free_ctrl(), which calls nvme_auth_free() (which will use
the options), nvme_free_ctrl() will then call ctrl->ops->free_ctrl(ctrl)
which will not free the options (since the ops->create_ctrl() call failed,
the ctrl will not be on the linked-list), fabrics.c:nvmf_create_ctrl() will
thus goto out_module_put, which then does a goto out_free_opts, which
calls nvmf_free_options().

In both cases, nvme_auth_free() will access memory that has not yet been
freed. So I don't see us dereferencing freed memory.


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Wed, Aug 19, 2026 at 08:32:33PM +0200, Niklas Cassel wrote:
> Hello Keith,
> 
> On Wed, Aug 19, 2026 at 09:28:08AM -0600, Keith Busch wrote:
> > On Mon, Aug 17, 2026 at 05:10:23PM +0200, Niklas Cassel wrote:
> > > It seems like Keith did prefer your patch, but as you said, your patch does
> > > not avoid a NULL pointer dereference in nvme_auth_free().
> > 
> > But you're replacing a NULL pointer dereference to a derefence to freed
> > memory. That should be fixed too, and I'm just saying checking a pointer
> > for NULL before dereferencing it is more clear than checking if a list
> > is empty.
> 
> I am not following.
> 
> The options is freed in two different places, depending on if the
> ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
> successful or not.

Oh, not that path. You mentioned previously that ctrl->ops is
dereferenced in other places. I thought this means we can successfully
add the controller, which means the attributes are visible, but then
fail something else later that causes the opts to free. Since the
attribute was visible, something can open it before the device exporting
is torn down, and access it after the opts were freed.

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Thu, Aug 20, 2026 at 09:23:10AM -0600, Keith Busch wrote:
> On Wed, Aug 19, 2026 at 08:32:33PM +0200, Niklas Cassel wrote:
> > 
> > I am not following.
> > 
> > The options is freed in two different places, depending on if the
> > ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
> > successful or not.
> 
> Oh, not that path. You mentioned previously that ctrl->ops is
> dereferenced in other places. I thought this means we can successfully
> add the controller, which means the attributes are visible, but then
> fail something else later that causes the opts to free. Since the
> attribute was visible, something can open it before the device exporting
> is torn down, and access it after the opts were freed.

Sashiko flagged another NULL pointer defererence from the sysfs attributes:

"While the sysfs attributes are still fully accessible, an unprivileged user
could concurrently read an attribute like hostnqn, causing a NULL pointer
dereference.":
https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com

This comment was on Rihyeon Kim's patch proposal.

I assume that this is because the fc.c driver (both before and after
Rihyeon patch) force set ctrl->ops = NULL before nvme_free_ctrl() was
called, so a user reading the sysfs attributes at the same time as the
controller was tearing down, could most likely crash the kernel.
(Note that neither rdma.c,tcp.c,loop.c does this, so this problem does
not exist for them.)

sysfs_remove_link() is called before ctrl->ops->free_ctrl(ctrl);


My proposal removes the "force set ctrl->ops = NULL", i.e. makes fc.c
look more like rdma.c,tcp.c,loop.c, so AFAICT these NULL pointer
dereferences in the sysfs accessors are no longer possible.

I assume that the sysfs accessors can be accessed until
sysfs_remove_link() has been called, and that they can be accessed
as long as the ctrl object has been created (i.e. the sysfs attributes
do not care if the ctrl object is on the linked list or not).

In both the success case and the failure case, the options are always
freed after sysfs_remove_link().

I hope that explains why we are not accessing freed memory.

Removing the ugly force setting of "ctrl->ops = NULL" that existed in fc.c
(and fc.c only) was a big motivation why I wrote this patch in the first
place.


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Fri, Aug 21, 2026 at 03:21:56PM +0200, Niklas Cassel wrote:
> On Thu, Aug 20, 2026 at 09:23:10AM -0600, Keith Busch wrote:
> > On Wed, Aug 19, 2026 at 08:32:33PM +0200, Niklas Cassel wrote:
> > > 
> > > I am not following.
> > > 
> > > The options is freed in two different places, depending on if the
> > > ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
> > > successful or not.
> > 
> > Oh, not that path. You mentioned previously that ctrl->ops is
> > dereferenced in other places. I thought this means we can successfully
> > add the controller, which means the attributes are visible, but then
> > fail something else later that causes the opts to free. Since the
> > attribute was visible, something can open it before the device exporting
> > is torn down, and access it after the opts were freed.
> 
> Sashiko flagged another NULL pointer defererence from the sysfs attributes:
> 
> "While the sysfs attributes are still fully accessible, an unprivileged user
> could concurrently read an attribute like hostnqn, causing a NULL pointer
> dereference.":
> https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com
> 
> This comment was on Rihyeon Kim's patch proposal.
> 
> I assume that this is because the fc.c driver (both before and after
> Rihyeon patch) force set ctrl->ops = NULL before nvme_free_ctrl() was
> called, so a user reading the sysfs attributes at the same time as the
> controller was tearing down, could most likely crash the kernel.
> (Note that neither rdma.c,tcp.c,loop.c does this, so this problem does
> not exist for them.)
> 
> sysfs_remove_link() is called before ctrl->ops->free_ctrl(ctrl);

When ops->create_ctrl() fails, the opts are freed from
nvmf_create_ctrl(). We can still fail ops->create_ctrl() after a
successful nvme_add_ctrl(), so the handle will be visible to user space
for a moment.

I think you're relying on nvme_fc_init_ctrl's nvme_put_ctrl() error case
to be the final reference, but it might not be if the character device
was created and someone opened it. If so then:

  out_put_ctrl:
     nvme_put_ctrl()   -> refcount 1, NOT zero

So now when nvme_free_ctrl is called sometime after nvmf_create_ctrl()
freed the opts, the ctrl is pointing to freed memory.

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Fri, Aug 21, 2026 at 08:18:31AM -0600, Keith Busch wrote:
> I think you're relying on nvme_fc_init_ctrl's nvme_put_ctrl() error case
> to be the final reference, but it might not be if the character device
> was created and someone opened it. If so then:

And a successful open requires the ctrl state is live, which it is not,
so it is always the last reference as needed. Okay, please let's forget
I've said in this thread!

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Fri, Aug 21, 2026 at 08:18:31AM -0600, Keith Busch wrote:
> On Fri, Aug 21, 2026 at 03:21:56PM +0200, Niklas Cassel wrote:
> > On Thu, Aug 20, 2026 at 09:23:10AM -0600, Keith Busch wrote:
> > > On Wed, Aug 19, 2026 at 08:32:33PM +0200, Niklas Cassel wrote:
> > > > 
> > > > I am not following.
> > > > 
> > > > The options is freed in two different places, depending on if the
> > > > ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
> > > > successful or not.
> > > 
> > > Oh, not that path. You mentioned previously that ctrl->ops is
> > > dereferenced in other places. I thought this means we can successfully
> > > add the controller, which means the attributes are visible, but then
> > > fail something else later that causes the opts to free. Since the
> > > attribute was visible, something can open it before the device exporting
> > > is torn down, and access it after the opts were freed.
> > 
> > Sashiko flagged another NULL pointer defererence from the sysfs attributes:
> > 
> > "While the sysfs attributes are still fully accessible, an unprivileged user
> > could concurrently read an attribute like hostnqn, causing a NULL pointer
> > dereference.":
> > https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com
> > 
> > This comment was on Rihyeon Kim's patch proposal.
> > 
> > I assume that this is because the fc.c driver (both before and after
> > Rihyeon patch) force set ctrl->ops = NULL before nvme_free_ctrl() was
> > called, so a user reading the sysfs attributes at the same time as the
> > controller was tearing down, could most likely crash the kernel.
> > (Note that neither rdma.c,tcp.c,loop.c does this, so this problem does
> > not exist for them.)
> > 
> > sysfs_remove_link() is called before ctrl->ops->free_ctrl(ctrl);
> 
> When ops->create_ctrl() fails, the opts are freed from
> nvmf_create_ctrl(). We can still fail ops->create_ctrl() after a
> successful nvme_add_ctrl(), so the handle will be visible to user space
> for a moment.
> 
> I think you're relying on nvme_fc_init_ctrl's nvme_put_ctrl() error case
> to be the final reference, but it might not be if the character device
> was created and someone opened it. If so then:
> 
>   out_put_ctrl:
>      nvme_put_ctrl()   -> refcount 1, NOT zero
> 
> So now when nvme_free_ctrl is called sometime after nvmf_create_ctrl()
> freed the opts, the ctrl is pointing to freed memory.

Yes, if someone called open() without calling close() for a long time,
then I agree that refcount will be 1 in at this label, and that
nvme_free_ctrl() will run after nvmf_create_ctrl() freed the opts.

I guess that is how whoever designed nvmf_create_ctrl() implemented things...

AFAICT, this problem is there for rdma.c,tcp.c,loop.c, so it is not a fc.c
specific problem.

The unconditional NULL pointer dereference in nvme_auth_free() was specific
to fc.c, and was solved by this patch.

So while the patch does solve an unconditional fc.c specific NULL pointer
dereference in nvme_auth_free() (if DHCAP is used), it does not solve a
potential NULL pointer dereference that can happen to: rdma.c,tcp.c,loop.c
(+fc.c after my patch) if someone is very quick to grab a reference in the
short window between creation and a failure during creation.

fc.c was not affected by this bug, because of the force setting of ctrl->ops
to NULL.

I still think this patch makes sense, since it brings the fc.c driver in
line with the other fabrics drivers, and avoids the dereference in
nvme_auth_free().

But yes, someone should probably do a follow up series that cleans up the
options handling for all fabrics drivers to solve the problem you just
described. At least the options handling will be uniform after this patch,
so hopefully that cleanup will be easier to implement because of this.


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Hannes Reinecke]

On 8/14/26 4:38 PM, Niklas Cassel wrote:
> nvmf_create_ctrl() owns the fabrics options and frees them whenever
> ->create_ctrl() returns an error, so a transport must not free them on
> its own error paths.  nvme-fc tracks this by testing ctrl->ctrl.opts in
> nvme_fc_ctrl_free(), which requires nvme_fc_init_ctrl() to clear that
> pointer on every error exit.
> 
> The coupling is implicit, and commit 1a9e218195a5 ("nvme: split device
> add from initialization") broke it by adding a second error exit.  When
> nvme_add_ctrl() fails, nvme_fc_init_ctrl() jumps to out_put_ctrl:, past
> the "ctrl->ctrl.opts = NULL" that only sits on the fail_ctrl: path, so
> nvme_fc_ctrl_free() frees the options and nvmf_create_ctrl() frees them
> a second time:
> 
>    BUG: KASAN: slab-use-after-free in nvmf_free_options+0x30/0x190
>     nvmf_free_options+0x30/0x190 drivers/nvme/host/fabrics.c:1284
>     nvmf_create_ctrl drivers/nvme/host/fabrics.c:1374 [inline]
>    Freed by task 5534:
>     nvme_fc_ctrl_free drivers/nvme/host/fc.c:2374 [inline]
>     nvme_fc_init_ctrl+0xe17/0x1450 drivers/nvme/host/fc.c:3605
> 
> nvme_add_ctrl() fails when dev_set_name() cannot allocate, so this is
> reachable under memory pressure or fault injection.  Without KASAN the
> options are freed twice.
> 
> Rather than clear the pointer on the second exit as well, derive
> ownership the way nvme-tcp, nvme-rdma and nvme-loop do, from list
> membership: their free_ctrl leaves the options alone unless the
> controller made it onto the transport list.
> 
> The list cannot simply be populated on the success path as it is there.
> nvme-fc runs the initial connect synchronously via flush_delayed_work(),
> and the controller has to be reachable on rport->ctrl_list for the whole
> of it: nvme_fc_unregister_remoteport() needs to find it to signal
> connectivity loss, nvme_fc_match_disconn_ls() matches an incoming
> Disconnect Association LS against ctrl->association_id, which is only
> assigned during that window, nvme_fc_resume_controller() needs it on
> remoteport re-registration, and nvme_fc_existing_controller() uses it to
> reject a duplicate connect racing the one in flight.
> 
> Keep the insertion where it is and add a fail_unlist: label, falling
> into fail_ctrl:, for the error paths that run after it.  The earlier
> error paths never reach the insertion and keep using fail_ctrl:
> directly, so the list is only touched where the controller is actually
> on it.
> 
> nvme_fc_ctrl_free() cannot use the plain "goto free_ctrl" the other
> transports use, because it still has to put_device(), release the rport
> reference and free the ida entry for resources taken before the
> insertion.  Sample list_empty() under rport->lock instead.
> 
> ctrl->ctrl.opts also stays valid for the whole teardown now.  That is
> not the bug being fixed, but it removes some fragility around the old
> idiom: nvme_free_ctrl() calls nvme_auth_free() before ->free_ctrl(), and
> ctrl_max_dhchaps() dereferences ctrl->opts without a NULL check when
> ctrl->dhchap_ctxs is set, which nvme-fc permits since NVMF_ALLOWED_OPTS
> allows the dhchap options.  The nvme sysfs attributes that dereference
> ctrl->opts, such as hostnqn and address, evaluate their is_visible()
> test once at device_add() time and stay readable until
> cdev_device_del().
> 
> Fixes: 1a9e218195a5 ("nvme: split device add from initialization")
> Cc: stable@vger.kernel.org
> Reported-by: syzbot+f58e57380a6083c4041d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f58e57380a6083c4041d
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/nvme/host/fc.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 04363b9c4489..7e1794f054d6 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2358,9 +2358,15 @@ nvme_fc_ctrl_free(struct kref *ref)
>   	struct nvme_fc_ctrl *ctrl =
>   		container_of(ref, struct nvme_fc_ctrl, ref);
>   	unsigned long flags;
> +	bool owns_opts;
>   
> -	/* remove from rport list */
> +	/*
> +	 * Presence on the rport list means nvme_fc_init_ctrl() completed,
> +	 * and with it ownership of the fabrics options passed to it. If it
> +	 * failed instead, the options still belong to nvmf_create_ctrl().
> +	 */
>   	spin_lock_irqsave(&ctrl->rport->lock, flags);
> +	owns_opts = !list_empty(&ctrl->ctrl_list);
>   	list_del(&ctrl->ctrl_list);
>   	spin_unlock_irqrestore(&ctrl->rport->lock, flags);
>   
> @@ -2370,7 +2376,7 @@ nvme_fc_ctrl_free(struct kref *ref)
>   	nvme_fc_rport_put(ctrl->rport);
>   
>   	ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
> -	if (ctrl->ctrl.opts)
> +	if (owns_opts)
>   		nvmf_free_options(ctrl->ctrl.opts);
>   	kfree(ctrl);
>   }
> @@ -3569,14 +3575,14 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
>   	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
>   		dev_err(ctrl->ctrl.device,
>   			"NVME-FC{%d}: failed to init ctrl state\n", ctrl->cnum);
> -		goto fail_ctrl;
> +		goto fail_unlist;
>   	}
>   
>   	if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
>   		dev_err(ctrl->ctrl.device,
>   			"NVME-FC{%d}: failed to schedule initial connect\n",
>   			ctrl->cnum);
> -		goto fail_ctrl;
> +		goto fail_unlist;
>   	}
>   
>   	flush_delayed_work(&ctrl->connect_work);
> @@ -3587,14 +3593,22 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
>   
>   	return &ctrl->ctrl;
>   
> +fail_unlist:
> +	/*
> +	 * Leaving the list hands the options back to nvmf_create_ctrl();
> +	 * see nvme_fc_ctrl_free().  Re-init so that list_empty() there
> +	 * reports the controller as unlisted.
> +	 */
> +	spin_lock_irqsave(&rport->lock, flags);
> +	list_del_init(&ctrl->ctrl_list);
> +	spin_unlock_irqrestore(&rport->lock, flags);
> +< hare
>   fail_ctrl:
>   	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
>   	cancel_work_sync(&ctrl->ioerr_work);
>   	cancel_work_sync(&ctrl->ctrl.reset_work);
>   	cancel_delayed_work_sync(&ctrl->connect_work);
>   
> -	ctrl->ctrl.opts = NULL;
> -
>   	if (ctrl->ctrl.admin_tagset)
>   		nvme_remove_admin_tag_set(&ctrl->ctrl);
>   	/* initiate nvme ctrl ref counting teardown */

Reviewed-by: Hannes Reinecke <hare@kernel.org>

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] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Fri, Aug 14, 2026 at 04:38:34PM +0200, Niklas Cassel wrote:
> nvmf_create_ctrl() owns the fabrics options and frees them whenever
> ->create_ctrl() returns an error, so a transport must not free them on
> its own error paths.  nvme-fc tracks this by testing ctrl->ctrl.opts in
> nvme_fc_ctrl_free(), which requires nvme_fc_init_ctrl() to clear that
> pointer on every error exit.

Since the race condition I was previously concerned about doesn't appear
to be possible, this solution matching the other fabrics drivers looks
good to go.

Thanks, applied to nvme-7.3.