[PATCH 0/2] scsi: fnic: fix NVMe/FC local port setup and teardown

[PATCH 0/2] scsi: fnic: fix NVMe/FC local port setup and teardown

[Linmao Li]

Two small fixes to the NVMe/FC transport path added in commit 5efdd5cf9281
("scsi: fnic: Add the NVMe/FC transport path"), both found by reading the
code rather than by hitting them on hardware.  They apply to
scsi/for-next, since that code is not in mainline yet.

Patch 1 initializes struct nvme_fc_port_info in nvfnic_add_lport().
dev_loss_tmo is currently left uninitialized on the stack.  The transport
does not read it for a local port, so this is hygiene rather than a
behaviour fix, and it carries no Fixes tag; nvfnic_add_tport() right below
already memsets its own copy.

Patch 2 moves the kfree() of iport->nv_tmpl out of nvfnic_nvme_unload()
and into nvfnic_local_port_delete().  The template is what the transport
stores in lport->ops, and nvme_fc_unregister_localport() only calls
->localport_delete() inline when no active remote ports are left;
otherwise the call is deferred to nvme_fc_rport_inactive_on_lport(),
which dereferences lport->ops after the unregister has returned.  Today
the driver frees the template even when the removal wait times out, which
leaves the transport with a dangling ->ops.

Note that patch 2 only narrows that one use-after-free.  The rest of the
timeout path still tears the fnic down while the transport holds
lport->private == iport, so that path may deserve a wider look; I did not
want to fold that into a fix.

Patch 2's reasoning rests on the NVMe/FC transport's lifetime rules, so
linux-nvme and the FC transport maintainers are copied for a second
opinion on that argument.

Compile-tested only (allmodconfig, W=1, drivers/scsi/fnic/ clean).  I do
not have Cisco VIC hardware, so neither patch has been tested on a live
adapter.

Linmao Li (2):
  scsi: fnic: initialize the NVMe local port info before registering
  scsi: fnic: free the NVMe port template from the delete callback

 drivers/scsi/fnic/fnic_nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


base-commit: 30733f28c0347d237ffb5333fdfab7a9a2d4ed34
-- 
2.25.1

[PATCH 1/2] scsi: fnic: initialize the NVMe local port info before registering

[Linmao Li]

nvfnic_add_lport() declares struct nvme_fc_port_info on the stack and
fills in four of its five members, leaving dev_loss_tmo holding whatever
the stack happened to contain before the call.  The structure is then
handed to nvme_fc_register_localport().

nvfnic_add_tport(), which registers the remote port a few lines further
down, memsets its own struct nvme_fc_port_info first, so only the local
port path passes uninitialized data across the transport interface.

The NVMe/FC transport documents dev_loss_tmo as "Used only on a
remoteport" and does not read it in nvme_fc_register_localport(), so
there is no behavioural change today.  Initialize the structure anyway:
the driver must not depend on which members the transport happens to
consume, and any member added to struct nvme_fc_port_info later would
silently start out as stack garbage.

Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/scsi/fnic/fnic_nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
index b237948dcafdc..00d9d5d439a38 100644
--- a/drivers/scsi/fnic/fnic_nvme.c
+++ b/drivers/scsi/fnic/fnic_nvme.c
@@ -2216,7 +2216,7 @@ int nvfnic_add_tport(struct fnic *fnic, struct fnic_tport_s *tport,
 
 int nvfnic_add_lport(struct fnic *fnic)
 {
-	struct nvme_fc_port_info pinfo;
+	struct nvme_fc_port_info pinfo = {};
 	struct fnic_iport_s *iport = &fnic->iport;
 	int ret = 0;
 
-- 
2.25.1

RE: [PATCH 1/2] scsi: fnic: initialize the NVMe local port info before registering

[Karan Tilak Kumar (kartilak)]

On Wednesday, August 19, 2026 5:13 PM, Linmao Li <lilinmao@kylinos.cn> wrote:
>
> nvfnic_add_lport() declares struct nvme_fc_port_info on the stack and
> fills in four of its five members, leaving dev_loss_tmo holding whatever
> the stack happened to contain before the call.  The structure is then
> handed to nvme_fc_register_localport().
>
> nvfnic_add_tport(), which registers the remote port a few lines further
> down, memsets its own struct nvme_fc_port_info first, so only the local
> port path passes uninitialized data across the transport interface.
>
> The NVMe/FC transport documents dev_loss_tmo as "Used only on a
> remoteport" and does not read it in nvme_fc_register_localport(), so
> there is no behavioural change today.  Initialize the structure anyway:
> the driver must not depend on which members the transport happens to
> consume, and any member added to struct nvme_fc_port_info later would
> silently start out as stack garbage.
>
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
> drivers/scsi/fnic/fnic_nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> index b237948dcafdc..00d9d5d439a38 100644
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c
> @@ -2216,7 +2216,7 @@ int nvfnic_add_tport(struct fnic *fnic, struct fnic_tport_s *tport,
>
> int nvfnic_add_lport(struct fnic *fnic)
> {
> -     struct nvme_fc_port_info pinfo;
> +     struct nvme_fc_port_info pinfo = {};
> struct fnic_iport_s *iport = &fnic->iport;
> int ret = 0;
>
> --
> 2.25.1
>
>

Thanks for the change, Linmao.
It seems simple and straightforward. 
I'll bundle it up with the other change in our internal testing and get back to you.

Regards,
Karan

RE: [PATCH 1/2] scsi: fnic: initialize the NVMe local port info before registering

[Karan Tilak Kumar (kartilak)]

On Sunday, August 23, 2026 10:35 AM, Karan Tilak Kumar (kartilak) wrote:
>
> On Wednesday, August 19, 2026 5:13 PM, Linmao Li <lilinmao@kylinos.cn> wrote:
> >
> > nvfnic_add_lport() declares struct nvme_fc_port_info on the stack and
> > fills in four of its five members, leaving dev_loss_tmo holding whatever
> > the stack happened to contain before the call.  The structure is then
> > handed to nvme_fc_register_localport().
> >
> > nvfnic_add_tport(), which registers the remote port a few lines further
> > down, memsets its own struct nvme_fc_port_info first, so only the local
> > port path passes uninitialized data across the transport interface.
> >
> > The NVMe/FC transport documents dev_loss_tmo as "Used only on a
> > remoteport" and does not read it in nvme_fc_register_localport(), so
> > there is no behavioural change today.  Initialize the structure anyway:
> > the driver must not depend on which members the transport happens to
> > consume, and any member added to struct nvme_fc_port_info later would
> > silently start out as stack garbage.
> >
> > Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> > ---
> > drivers/scsi/fnic/fnic_nvme.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> > index b237948dcafdc..00d9d5d439a38 100644
> > --- a/drivers/scsi/fnic/fnic_nvme.c
> > +++ b/drivers/scsi/fnic/fnic_nvme.c
> > @@ -2216,7 +2216,7 @@ int nvfnic_add_tport(struct fnic *fnic, struct fnic_tport_s *tport,
> >
> > int nvfnic_add_lport(struct fnic *fnic)
> > {
> > -     struct nvme_fc_port_info pinfo;
> > +     struct nvme_fc_port_info pinfo = {};
> > struct fnic_iport_s *iport = &fnic->iport;
> > int ret = 0;
> >
> > --
> > 2.25.1
> >
> >
>
> Thanks for the change, Linmao.
> It seems simple and straightforward.
> I'll bundle it up with the other change in our internal testing and get back to you.
>
> Regards,
> Karan
>

Just realized that you've requested to drop patch 2/2. 
I'll do a build and load of this change and check that nothing's broken.
I'll get back to you.

Regards,
Karan

RE: [PATCH 1/2] scsi: fnic: initialize the NVMe local port info before registering

[Karan Tilak Kumar (kartilak)]

On Sunday, August 23, 2026 10:41 AM, Karan Tilak Kumar (kartilak) wrote:
>
> On Sunday, August 23, 2026 10:35 AM, Karan Tilak Kumar (kartilak) wrote:
> >
> > On Wednesday, August 19, 2026 5:13 PM, Linmao Li <lilinmao@kylinos.cn> wrote:
> > >
> > > nvfnic_add_lport() declares struct nvme_fc_port_info on the stack and
> > > fills in four of its five members, leaving dev_loss_tmo holding whatever
> > > the stack happened to contain before the call.  The structure is then
> > > handed to nvme_fc_register_localport().
> > >
> > > nvfnic_add_tport(), which registers the remote port a few lines further
> > > down, memsets its own struct nvme_fc_port_info first, so only the local
> > > port path passes uninitialized data across the transport interface.
> > >
> > > The NVMe/FC transport documents dev_loss_tmo as "Used only on a
> > > remoteport" and does not read it in nvme_fc_register_localport(), so
> > > there is no behavioural change today.  Initialize the structure anyway:
> > > the driver must not depend on which members the transport happens to
> > > consume, and any member added to struct nvme_fc_port_info later would
> > > silently start out as stack garbage.
> > >
> > > Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> > > ---
> > > drivers/scsi/fnic/fnic_nvme.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> > > index b237948dcafdc..00d9d5d439a38 100644
> > > --- a/drivers/scsi/fnic/fnic_nvme.c
> > > +++ b/drivers/scsi/fnic/fnic_nvme.c
> > > @@ -2216,7 +2216,7 @@ int nvfnic_add_tport(struct fnic *fnic, struct fnic_tport_s *tport,
> > >
> > > int nvfnic_add_lport(struct fnic *fnic)
> > > {
> > > -     struct nvme_fc_port_info pinfo;
> > > +     struct nvme_fc_port_info pinfo = {};
> > > struct fnic_iport_s *iport = &fnic->iport;
> > > int ret = 0;
> > >
> > > --
> > > 2.25.1
> > >
> > >
> >
> > Thanks for the change, Linmao.
> > It seems simple and straightforward.
> > I'll bundle it up with the other change in our internal testing and get back to you.
> >
> > Regards,
> > Karan
> >
>
> Just realized that you've requested to drop patch 2/2.
> I'll do a build and load of this change and check that nothing's broken.
> I'll get back to you.
>
> Regards,
> Karan
>

Thanks for the change. It looks good.

Tested-by: Karan Tilak Kumar <kartilak@cisco.com>
Reviewed-by: Karan Tilak Kumar <kartilak@cisco.com>

Regards,
Karan

Re: [PATCH 1/2] scsi: fnic: initialize the NVMe local port info before registering

[Martin K. Petersen (Oracle)]

Karan,

> Thanks for the change. It looks good.
>
> Tested-by: Karan Tilak Kumar <kartilak@cisco.com>
> Reviewed-by: Karan Tilak Kumar <kartilak@cisco.com>

Applied to 7.3/scsi-staging, thanks!

-- 
Martin K. Petersen

[PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback

[Linmao Li]

nvfnic_nvme_unload() calls nvme_fc_unregister_localport(), waits for the
transport to call back into nvfnic_local_port_delete(), and then frees
iport->nv_tmpl.  The wait is bounded, and when it expires the driver
only warns and frees the template anyway.

iport->nv_tmpl is the struct nvme_fc_port_template the transport keeps
in lport->ops.  nvme_fc_unregister_localport() only invokes
->localport_delete() when the local port has no active remote ports
left; otherwise the call is deferred to
nvme_fc_rport_inactive_on_lport(), which dereferences lport->ops long
after the unregister call returned.  Freeing the template on the timeout
path therefore leaves the transport with a dangling ->ops.

Tie the lifetime to the callback that marks the end of the transport's
use of the template instead of to the timeout.  nvfnic_local_port_delete()
runs from ->localport_delete(), after which nvme_fc_free_lport() no
longer touches lport->ops, so freeing there is safe in both the direct
and the deferred case.  The free is done before the completion is
signalled, because the unload path may tear the fnic down, and with it
the embedded iport, as soon as it wakes up.

Fixes: 5efdd5cf9281 ("scsi: fnic: Add the NVMe/FC transport path")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/scsi/fnic/fnic_nvme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
index 00d9d5d439a38..a8b7b7d40b9ab 100644
--- a/drivers/scsi/fnic/fnic_nvme.c
+++ b/drivers/scsi/fnic/fnic_nvme.c
@@ -1243,7 +1243,6 @@ void nvfnic_nvme_unload(struct fnic *fnic)
 				WARN_ON(1);
 			}
 			iport->flags &= ~FNIC_LPORT_NVME_REGISTERED;
-			kfree(iport->nv_tmpl);
 		}
 	}
 
@@ -1669,6 +1668,9 @@ void nvfnic_local_port_delete(struct nvme_fc_local_port *lport)
 	FNIC_NVME_DBG(KERN_INFO, fnic, "lport delete 0x%x\n",
 		      iport->fcid);
 
+	kfree(iport->nv_tmpl);
+	iport->nv_tmpl = NULL;
+
 	spin_lock_irqsave(&fnic->fnic_lock, flags);
 	if (fnic->nvme_lport_unreg_done)
 		complete(fnic->nvme_lport_unreg_done);
-- 
2.25.1

Re: [PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback

[Linmao Li]

Resending: this reply went to the review thread and linux-scsi on Aug 19
but not to any of you directly, so you may not have seen it.  Please drop
2/2.  The bot's two findings are quoted below; the rest of its mail is
trimmed.

On Wed, Aug 19, 2026 at 11:52 AM +0000, sashiko-bot@kernel.org wrote:
> New issues:
> - [Critical] Relocating `kfree(iport->nv_tmpl)` to the asynchronous
> `nvfnic_local_port_delete()` callback introduces a severe arbitrary free
> and memory corruption vulnerability if the unregister process times out.
>
> Pre-existing issues:
> - [Critical] The driver's bounded timeout design for NVMe FC
> unregistration leaves the `fnic` structure vulnerable to a Use-After-Free
> when deferred callbacks execute.

The bot is right.  Please drop this patch; 1/2 is independent and still
stands on its own.

If the wait times out, fnic_remove() continues and eventually frees fnic,
which also frees the embedded iport still referenced by lport->private.
A later ->localport_delete() callback would therefore read nv_tmpl from
freed memory before passing it to kfree().

The underlying problem is that the bounded wait allows fnic to be freed
while the NVMe/FC transport still holds pointers into it.  I will revisit
that teardown separately.

Sorry for the noise on 2/2.


Thanks,
Linmao

RE: [PATCH 2/2] scsi: fnic: free the NVMe port template from the delete callback

[Karan Tilak Kumar (kartilak)]

On Thursday, August 20, 2026 1:00 PM, Linmao Li <lilinmao@kylinos.cn> wrote:
>
> Resending: this reply went to the review thread and linux-scsi on Aug 19
> but not to any of you directly, so you may not have seen it.  Please drop
> 2/2.  The bot's two findings are quoted below; the rest of its mail is
> trimmed.
>
> On Wed, Aug 19, 2026 at 11:52 AM +0000, sashiko-bot@kernel.org wrote:
> > New issues:
> > - [Critical] Relocating `kfree(iport->nv_tmpl)` to the asynchronous
> > `nvfnic_local_port_delete()` callback introduces a severe arbitrary free
> > and memory corruption vulnerability if the unregister process times out.
> >
> > Pre-existing issues:
> > - [Critical] The driver's bounded timeout design for NVMe FC
> > unregistration leaves the `fnic` structure vulnerable to a Use-After-Free
> > when deferred callbacks execute.
>
> The bot is right.  Please drop this patch; 1/2 is independent and still
> stands on its own.
>
> If the wait times out, fnic_remove() continues and eventually frees fnic,
> which also frees the embedded iport still referenced by lport->private.
> A later ->localport_delete() callback would therefore read nv_tmpl from
> freed memory before passing it to kfree().
>
> The underlying problem is that the bounded wait allows fnic to be freed
> while the NVMe/FC transport still holds pointers into it.  I will revisit
> that teardown separately.
>
> Sorry for the noise on 2/2.
>
>
> Thanks,
> Linmao
>

Thanks for the change, Linmao.

I will get back to you about this change after testing it internally.

Regards,
Karan