Re: [PATCH 02/11] nvmet-fcloop: add ref counting to lport

[Daniel Wagner <dwagner@suse.de>]

On Wed, Mar 05, 2025 at 03:17:40PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 26, 2025 at 07:45:54PM +0100, Daniel Wagner wrote:
> > +static void
> > +fcloop_lport_free(struct kref *ref)
> > +{
> > +	struct fcloop_lport *lport =
> > +		container_of(ref, struct fcloop_lport, ref);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fcloop_lock, flags);
> > +	list_del(&lport->lport_list);
> > +	spin_unlock_irqrestore(&fcloop_lock, flags);
> > +
> > +	kfree(lport);
> 
> Maybe it's just me, but I find the kref a really horrible pattern over
> usig a simple refcount_t.

There was already some kref usage in the fc code, that's why I started
to use it. But I agree there is not much to be gained from the kref
wrappers. I'll replace them.

> Otherwise adding proper refcounting looks fine.

BTW, I found a bunch more places which need to do proper ref counting. I
have now a version which works pretty good. Though there is one
case which gives me an UAF:

  setup target
  setup host
  connect
  loop
    remove target
    wait for host connecting state
    add target
    wait for host live state

When fcloop has no in flight commands and the target is removed, fcloop
will unregister the localport now. But the nvme-fc driver just assumes
that the port is always there and just sends down new commands
independend of the port status:

nvme_fc_start_fcp_op()
{
[...]

	ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport,
					&ctrl->rport->remoteport,
					queue->lldd_handle, &op->fcp_req);
[...]
}

There is nothing which updates the ctrl in nvme_fc_unregister_localport.
Not really sure what to do here. fcloop obviously is now behaving
differently to the hw drivers. But still, this looks very fragile that
there is no sort of synchronization between port unregistration and ctrl
state.