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

[Niklas Cassel <cassel@kernel.org>]

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