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

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