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

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