[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

[syzbot] [nvme?] KASAN: slab-use-after-free Read in nvmf_free_options

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    0d8395707651 Merge tag 'soc-fixes-7.2-2' of git://git.kern..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=130897b9580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=c05be6c9b0d36cb9
dashboard link: https://syzkaller.appspot.com/bug?extid=f58e57380a6083c4041d
compiler:       Debian clang version 22.1.8 (++20260613092233+e80beda6e255-1~exp1~20260613092250.77), Debian LLD 22.1.8

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/d900f083ada3/non_bootable_disk-0d839570.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/f71d0db84455/vmlinux-0d839570.xz
kernel image: https://storage.googleapis.com/syzbot-assets/9e4025aaafe2/bzImage-0d839570.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f58e57380a6083c4041d@syzkaller.appspotmail.com

FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 1
CPU: 0 UID: 0 PID: 5326 Comm: syz.0.0 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 fail_dump lib/fault-inject.c:73 [inline]
 should_fail_ex+0x40c/0x560 lib/fault-inject.c:174
 should_failslab+0xa8/0x100 mm/failslab.c:46
 slab_pre_alloc_hook mm/slub.c:4539 [inline]
 slab_alloc_node mm/slub.c:4897 [inline]
 __do_kmalloc_node mm/slub.c:5333 [inline]
 __kmalloc_node_track_caller_noprof+0x100/0x730 mm/slub.c:5471
 kvasprintf+0xeb/0x1a0 lib/kasprintf.c:25
 kobject_set_name_vargs+0x61/0x110 lib/kobject.c:274
 dev_set_name+0xe2/0x140 drivers/base/core.c:3560
 nvme_add_ctrl+0x6e/0x270 drivers/nvme/host/core.c:5248
 nvme_fc_init_ctrl+0xcb0/0x1450 drivers/nvme/host/fc.c:3554
 nvme_fc_create_ctrl+0x4cd/0x520 drivers/nvme/host/fc.c:3723
 nvmf_create_ctrl drivers/nvme/host/fabrics.c:1359 [inline]
 nvmf_dev_write+0x24ca/0x2d90 drivers/nvme/host/fabrics.c:1406
 vfs_write+0x296/0xba0 fs/read_write.c:685
 ksys_write+0x150/0x270 fs/read_write.c:739
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fea9b39e0d9
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fea9c299fe8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007fea9b625fa0 RCX: 00007fea9b39e0d9
RDX: 000000000000007e RSI: 0000200000000a00 RDI: 0000000000000005
RBP: 00007fea9c29a050 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000002
R13: 00007fea9b626038 R14: 00007fea9b625fa0 R15: 00007ffe156ed1b8
 </TASK>
==================================================================
BUG: KASAN: slab-use-after-free in nvmf_free_options+0x30/0x190 drivers/nvme/host/fabrics.c:1284
Read of size 8 at addr ffff88801f4a2550 by task syz.0.0/5326

CPU: 0 UID: 0 PID: 5326 Comm: syz.0.0 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 print_address_description+0x55/0x1e0 mm/kasan/report.c:378
 print_report+0x58/0x70 mm/kasan/report.c:482
 kasan_report+0x117/0x150 mm/kasan/report.c:595
 nvmf_free_options+0x30/0x190 drivers/nvme/host/fabrics.c:1284
 nvmf_create_ctrl drivers/nvme/host/fabrics.c:1374 [inline]
 nvmf_dev_write+0x267f/0x2d90 drivers/nvme/host/fabrics.c:1406
 vfs_write+0x296/0xba0 fs/read_write.c:685
 ksys_write+0x150/0x270 fs/read_write.c:739
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fea9b39e0d9
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fea9c299fe8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007fea9b625fa0 RCX: 00007fea9b39e0d9
RDX: 000000000000007e RSI: 0000200000000a00 RDI: 0000000000000005
RBP: 00007fea9c29a050 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000002
R13: 00007fea9b626038 R14: 00007fea9b625fa0 R15: 00007ffe156ed1b8
 </TASK>

Allocated by task 5326:
 kasan_save_stack mm/kasan/common.c:57 [inline]
 kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
 poison_kmalloc_redzone mm/kasan/common.c:398 [inline]
 __kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:415
 kasan_kmalloc include/linux/kasan.h:263 [inline]
 __kmalloc_cache_noprof+0x32d/0x660 mm/slub.c:5489
 _kmalloc_noprof include/linux/slab.h:988 [inline]
 _kzalloc_noprof include/linux/slab.h:1309 [inline]
 nvmf_create_ctrl drivers/nvme/host/fabrics.c:1315 [inline]
 nvmf_dev_write+0x243/0x2d90 drivers/nvme/host/fabrics.c:1406
 vfs_write+0x296/0xba0 fs/read_write.c:685
 ksys_write+0x150/0x270 fs/read_write.c:739
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Freed by task 5326:
 kasan_save_stack mm/kasan/common.c:57 [inline]
 kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
 kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:584
 poison_slab_object mm/kasan/common.c:253 [inline]
 __kasan_slab_free+0x5c/0x80 mm/kasan/common.c:285
 kasan_slab_free include/linux/kasan.h:235 [inline]
 slab_free_hook mm/slub.c:2677 [inline]
 slab_free mm/slub.c:6377 [inline]
 kfree+0x1c5/0x640 mm/slub.c:6692
 nvme_fc_ctrl_free drivers/nvme/host/fc.c:2374 [inline]
 kref_put include/linux/kref.h:65 [inline]
 nvme_fc_ctrl_put+0x2cc/0x340 drivers/nvme/host/fc.c:2381
 nvme_free_ctrl+0x38c/0x570 drivers/nvme/host/core.c:5146
 device_release+0xc4/0x1f0 drivers/base/core.c:-1
 kobject_cleanup lib/kobject.c:689 [inline]
 kobject_release lib/kobject.c:720 [inline]
 kref_put include/linux/kref.h:65 [inline]
 kobject_put+0x222/0x550 lib/kobject.c:737
 nvme_put_ctrl drivers/nvme/host/nvme.h:833 [inline]
 nvme_fc_init_ctrl+0xe17/0x1450 drivers/nvme/host/fc.c:3605
 nvme_fc_create_ctrl+0x4cd/0x520 drivers/nvme/host/fc.c:3723
 nvmf_create_ctrl drivers/nvme/host/fabrics.c:1359 [inline]
 nvmf_dev_write+0x24ca/0x2d90 drivers/nvme/host/fabrics.c:1406
 vfs_write+0x296/0xba0 fs/read_write.c:685
 ksys_write+0x150/0x270 fs/read_write.c:739
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The buggy address belongs to the object at ffff88801f4a2500
 which belongs to the cache kmalloc-192 of size 192
The buggy address is located 80 bytes inside of
 freed 192-byte region [ffff88801f4a2500, ffff88801f4a25c0)

The buggy address belongs to the physical page:
page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1f4a2
flags: 0xfff00000000000(node=0|zone=1|lastcpupid=0x7ff)
page_type: f5(slab)
raw: 00fff00000000000 ffff88801b0413c0 dead000000000122 0000000000000000
raw: 0000000000000000 0000000800100010 00000000f5000000 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0xd2cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 5289, tgid 5289 (syz-executor), ts 83703979346, free_ts 83087386547
 set_page_owner include/linux/page_owner.h:32 [inline]
 post_alloc_hook+0x1f9/0x250 mm/page_alloc.c:1859
 prep_new_page mm/page_alloc.c:1867 [inline]
 get_page_from_freelist+0x21fa/0x2270 mm/page_alloc.c:3946
 __alloc_frozen_pages_noprof+0x18d/0x380 mm/page_alloc.c:5304
 alloc_slab_page mm/slub.c:3266 [inline]
 allocate_slab+0x79/0x5e0 mm/slub.c:3380
 new_slab mm/slub.c:3426 [inline]
 refill_objects+0x2d5/0x350 mm/slub.c:7310
 refill_sheaf mm/slub.c:2804 [inline]
 __pcs_replace_empty_main+0x2bf/0x6b0 mm/slub.c:4675
 alloc_from_pcs mm/slub.c:4773 [inline]
 slab_alloc_node mm/slub.c:4905 [inline]
 __do_kmalloc_node mm/slub.c:5333 [inline]
 __kmalloc_node_track_caller_noprof+0x557/0x730 mm/slub.c:5471
 kmemdup_noprof+0x2b/0x70 mm/util.c:138
 kmemdup_noprof include/linux/fortify-string.h:715 [inline]
 ebt_register_table+0x18e/0x1190 net/bridge/netfilter/ebtables.c:1197
 find_inlist_lock_noload+0x171/0x260 net/bridge/netfilter/ebtables.c:344
 find_inlist_lock net/bridge/netfilter/ebtables.c:372 [inline]
 find_table_lock net/bridge/netfilter/ebtables.c:380 [inline]
 do_ebt_get_ctl+0x367/0x1f30 net/bridge/netfilter/ebtables.c:2512
 nf_getsockopt+0x26e/0x290 net/netfilter/nf_sockopt.c:116
 ip_getsockopt+0x19e/0x230 net/ipv4/ip_sockglue.c:1777
 do_sock_getsockopt+0x569/0xa70 net/socket.c:2474
 __sys_getsockopt net/socket.c:2505 [inline]
 __do_sys_getsockopt net/socket.c:2512 [inline]
 __se_sys_getsockopt net/socket.c:2509 [inline]
 __x64_sys_getsockopt+0x1a4/0x240 net/socket.c:2509
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
page last free pid 75 tgid 75 stack trace:
 reset_page_owner include/linux/page_owner.h:25 [inline]
 __free_pages_prepare mm/page_alloc.c:1406 [inline]
 free_unref_folios+0xd84/0x14a0 mm/page_alloc.c:3011
 shrink_folio_list+0x4b16/0x5330 mm/vmscan.c:1582
 evict_folios+0x3821/0x49e0 mm/vmscan.c:4834
 try_to_shrink_lruvec+0xb4f/0xed0 mm/vmscan.c:4983
 shrink_one+0x233/0x730 mm/vmscan.c:5024
 shrink_many mm/vmscan.c:5087 [inline]
 lru_gen_shrink_node mm/vmscan.c:5165 [inline]
 shrink_node+0x3303/0x3b60 mm/vmscan.c:6154
 kswapd_shrink_node mm/vmscan.c:7008 [inline]
 balance_pgdat mm/vmscan.c:7186 [inline]
 kswapd+0x17b6/0x31c0 mm/vmscan.c:7462
 kthread+0x388/0x470 kernel/kthread.c:436
 ret_from_fork+0x514/0xb70 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245

Memory state around the buggy address:
 ffff88801f4a2400: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88801f4a2480: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>ffff88801f4a2500: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                                 ^
 ffff88801f4a2580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
 ffff88801f4a2600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

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

[Rihyeon Kim]

nvmf_create_ctrl() frees opts when ->create_ctrl() returns an error, so
a transport must not free it on its own error paths.  nvme_fc_ctrl_free()
therefore only calls nvmf_free_options() while ctrl->ctrl.opts is still
set, and nvme_fc_init_ctrl() clears that pointer before its last put.

It only does so on the fail_ctrl: path, though.  When nvme_add_ctrl()
fails, nvme_fc_init_ctrl() jumps to out_put_ctrl: instead, so
nvme_fc_ctrl_free() still sees ctrl->ctrl.opts set and frees opts, and
nvmf_create_ctrl() frees it again.

Reproduced with nvme-fcloop and failslab by failing the kvasprintf() in
dev_set_name(), called from nvme_add_ctrl():

  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

Without KASAN opts is simply freed twice.

nvme-tcp and nvme-rdma reach the same error path, but their free_ctrl
only frees opts once the controller is on the global list, so they are
not affected.

Clear ctrl->ctrl.opts on the out_put_ctrl: path as well.  The same
injection then returns -EIO without a report.

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
Assisted-by: Claude:claude-opus-5
Signed-off-by: Rihyeon Kim <rihyeon8648@gmail.com>
---
 drivers/nvme/host/fc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..e4d0eeccd846 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3601,6 +3601,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	nvme_uninit_ctrl(&ctrl->ctrl);
 
 out_put_ctrl:
+	/* nvme_add_ctrl() failures skip the clear in fail_ctrl: above */
+	ctrl->ctrl.opts = NULL;
+
 	/* Remove core ctrl ref. */
 	nvme_put_ctrl(&ctrl->ctrl);
 

base-commit: 0ce37745d4bfbc493f718169c3974898ffec8ee7
-- 
2.43.0

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

[Niklas Cassel]

On Tue, Aug 11, 2026 at 09:53:10PM +0900, Rihyeon Kim wrote:

(snip)

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 04363b9c4489..e4d0eeccd846 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3601,6 +3601,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
>  	nvme_uninit_ctrl(&ctrl->ctrl);
>  
>  out_put_ctrl:
> +	/* nvme_add_ctrl() failures skip the clear in fail_ctrl: above */
> +	ctrl->ctrl.opts = NULL;
> +
>  	/* Remove core ctrl ref. */
>  	nvme_put_ctrl(&ctrl->ctrl);
>  
> 
> base-commit: 0ce37745d4bfbc493f718169c3974898ffec8ee7
> -- 
> 2.43.0
> 
> 

Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
nvme_tcp_ctrl_free(), i.e. something like:

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..2b4c222995ea 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2359,19 +2359,22 @@ nvme_fc_ctrl_free(struct kref *ref)
                container_of(ref, struct nvme_fc_ctrl, ref);
        unsigned long flags;

+       if (list_empty(&ctrl->ctrl_list))
+               goto free_ctrl;
+
        /* remove from rport list */
        spin_lock_irqsave(&ctrl->rport->lock, flags);
        list_del(&ctrl->ctrl_list);
        spin_unlock_irqrestore(&ctrl->rport->lock, flags);

-       kfree(ctrl->queues);
-
        put_device(ctrl->dev);
        nvme_fc_rport_put(ctrl->rport);

        ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
-       if (ctrl->ctrl.opts)
-               nvmf_free_options(ctrl->ctrl.opts);
+
+       nvmf_free_options(ctrl->ctrl.opts);
+free_ctrl:
+       kfree(ctrl->queues);
        kfree(ctrl);
 }

@@ -3593,8 +3596,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
        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 */



However, I'm not familar with the fc driver, so this suggestion is untested.


Kind regards,
Niklas

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

[Rihyeon Kim]

Hello,

Thanks for the suggestion.

> Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
> nvme_tcp_ctrl_free(), i.e. something like:
>
> +       if (list_empty(&ctrl->ctrl_list))
> +               goto free_ctrl;

I am not sure whether that would work, and I may well be missing
something.  From what I could tell, nvme_tcp_create_ctrl() does its
list_add_tail() last, while nvme_fc_init_ctrl() does it before the
nvme_change_ctrl_state() and queue_delayed_work() checks, so on those two
failure paths the controller is already on the list and opts would end up
freed twice again.  The early goto would also skip the ida_free(),
put_device() and nvme_fc_rport_put() for what nvme_fc_alloc_ctrl() takes
before the list_add.

I am not familiar with this driver either, so please correct me if I have
misread it.

Keith suggested moving the existing clear from fail_ctrl: down to
out_put_ctrl: instead, so I will send that as v2.

Thanks,
Rihyeon

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

[Rihyeon Kim]

Hello,

Thanks for the suggestion.

> Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
> nvme_tcp_ctrl_free(), i.e. something like:
>
> +       if (list_empty(&ctrl->ctrl_list))
> +               goto free_ctrl;

I am not sure whether that would work, and I may well be missing
something.  From what I could tell, nvme_tcp_create_ctrl() does its
list_add_tail() last, while nvme_fc_init_ctrl() does it before the
nvme_change_ctrl_state() and queue_delayed_work() checks, so on those two
failure paths the controller is already on the list and opts would end up
freed twice again.  The early goto would also skip the ida_free(),
put_device() and nvme_fc_rport_put() for what nvme_fc_alloc_ctrl() takes
before the list_add.

I am not familiar with this driver either, so please correct me if I have
misread it.

Keith suggested moving the existing clear from fail_ctrl: down to
out_put_ctrl: instead, so I will send that as v2.

Thanks,
Rihyeon

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

[Niklas Cassel]

On Wed, Aug 12, 2026 at 07:59:41PM +0900, Rihyeon Kim wrote:
> Hello,
> 
> Thanks for the suggestion.
> 
> > Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
> > nvme_tcp_ctrl_free(), i.e. something like:
> >
> > +       if (list_empty(&ctrl->ctrl_list))
> > +               goto free_ctrl;
> 
> I am not sure whether that would work, and I may well be missing
> something.  From what I could tell, nvme_tcp_create_ctrl() does its
> list_add_tail() last, while nvme_fc_init_ctrl() does it before the
> nvme_change_ctrl_state() and queue_delayed_work() checks, so on those two
> failure paths the controller is already on the list and opts would end up
> freed twice again.  The early goto would also skip the ida_free(),
> put_device() and nvme_fc_rport_put() for what nvme_fc_alloc_ctrl() takes
> before the list_add.

The fact that both rdma.c and tcp.c does:
1) if (list_empty(ctrl_list)) goto free_ctrl;
2) call list_add() last (after nvme_change_ctrl_state())
3) not have any ctrl->ctrl.opts = NULL; hacks anywhere in them

suggests to me that a proper design would be for the fc.c driver to look
the same as rdma.c and tcp.c.

I'm not familiar with fc.c, but I can imagine that the goto free_ctrl label
can be placed such that ida_free() (and whatever else needs to be called)
is done so after the free_ctrl label, while nvmf_free_options() is done
before the free_ctrl label.

But sure, I agree that such a change would have to be done by someone
familiar with the fc.c driver.


> 
> Keith suggested moving the existing clear from fail_ctrl: down to
> out_put_ctrl: instead, so I will send that as v2.

Looking at the Sashiko comment:
https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com

"Will setting ctrl->opts to NULL here cause a guaranteed NULL pointer
dereference during teardown if DHCHAP authentication is configured?"

makes me even more certain that it is wrong for fc.c to have a
ctrl->ctrl.opts = NULL; before calling nvme_put_ctrl().

The nvme_put_ctrl() call will lead to a call to nvme_free_ctrl(), which will
call e.g. nvme_auth_free(), before calling the ctrl->ops->free_ctrl(ctrl);
callback. So some existing teardown functions in nvme_free_ctrl() expects opts
to be valid/non-NULL, and that the it should be freed earliest by the
ctrl->ops->free_ctrl(ctrl) callback, and for all other fabrics (rdma, fc),
this appears to be true.

Note that the same Sashiko link also reports a sysfs NULL pointer dereference
that is possible by the ctrl->ctrl.opts = NULL;

To me, it seems like fc.c should simply look more like tcp.c and fc.c.

That way:
1) No ctrl->ctrl.opts = NULL; hack needed in fc.c.
2) The sysfs NULL pointer dereference is no longer possible.
3) Teardown functions called by nvme_free_ctrl() can continue to assume that
   ctrl->opts is valid.

Perhaps some of the fc.c maintainers could have a look?


Kind regards,
Niklas

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

[Niklas Cassel]

On Wed, Aug 12, 2026 at 04:26:24PM +0200, Niklas Cassel wrote:
> 
> To me, it seems like fc.c should simply look more like tcp.c and fc.c.

[...] more like tcp.c and rdma.c.


Kind regards,
Niklas

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

[Keith Busch]

On Tue, Aug 11, 2026 at 09:53:10PM +0900, Rihyeon Kim wrote:
> It only does so on the fail_ctrl: path, though.  When nvme_add_ctrl()
> fails, nvme_fc_init_ctrl() jumps to out_put_ctrl: instead, so
> nvme_fc_ctrl_free() still sees ctrl->ctrl.opts set and frees opts, and
> nvmf_create_ctrl() frees it again.

...

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 04363b9c4489..e4d0eeccd846 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3601,6 +3601,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
>  	nvme_uninit_ctrl(&ctrl->ctrl);
>  
>  out_put_ctrl:
> +	/* nvme_add_ctrl() failures skip the clear in fail_ctrl: above */
> +	ctrl->ctrl.opts = NULL;
> +
>  	/* Remove core ctrl ref. */
>  	nvme_put_ctrl(&ctrl->ctrl);

Can't you move the setting from the "fail_ctrl:" label to the
"out_put_ctrl:" one instead of duplicating it for both?

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

[Rihyeon Kim]

Hello,

Thanks for the review.

> Can't you move the setting from the "fail_ctrl:" label to the
> "out_put_ctrl:" one instead of duplicating it for both?

Yes.  I had kept both because I was not sure the put inside
nvme_uninit_ctrl() could not be the last one, which would run
nvme_fc_ctrl_free() before out_put_ctrl: cleared the pointer.  As far as
I could tell nvme-tcp and nvme-rdma use the same uninit-then-put ladder,
and testing does not show it either.

I swept fail-nth 1..200 over the connect write with fcloop and failslab:
unpatched hits the report at 17, and with the clear moved all 200 pass,
21 of the injections landing in nvme_alloc_admin_tag_set(), so fail_ctrl:
is covered as well.

It also looks like moving it drops the window where opts is already NULL
while the fabrics sysfs attributes, which do not check it, are still
there.

v2 on the way.

Thanks,
Rihyeon

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

[Rihyeon Kim]

Hello,

Thanks for the review.

> Can't you move the setting from the "fail_ctrl:" label to the
> "out_put_ctrl:" one instead of duplicating it for both?

Yes.  I had kept both because I was not sure the put inside
nvme_uninit_ctrl() could not be the last one, which would run
nvme_fc_ctrl_free() before out_put_ctrl: cleared the pointer.  As far as
I could tell nvme-tcp and nvme-rdma use the same uninit-then-put ladder,
and testing does not show it either.

I swept fail-nth 1..200 over the connect write with fcloop and failslab:
unpatched hits the report at 17, and with the clear moved all 200 pass,
21 of the injections landing in nvme_alloc_admin_tag_set(), so fail_ctrl:
is covered as well.

It also looks like moving it drops the window where opts is already NULL
while the fabrics sysfs attributes, which do not check it, are still
there.

v2 on the way.

Thanks,
Rihyeon