linux-kernel - Re: [PATCH 05/11] nvmet-fcloop: track tport with ref counting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <adb0bdc1-f4be-4004-8651-7f8393276f46@flourine.local>
Date: Fri, 28 Feb 2025 09:30:38 +0100
From: Daniel Wagner <dwagner@...e.de>
To: Hannes Reinecke <hare@...e.de>
Cc: James Smart <james.smart@...adcom.com>, Christoph Hellwig <hch@....de>, 
	Sagi Grimberg <sagi@...mberg.me>, Chaitanya Kulkarni <kch@...dia.com>, 
	Keith Busch <kbusch@...nel.org>, linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/11] nvmet-fcloop: track tport with ref counting

On Fri, Feb 28, 2025 at 08:27:40AM +0100, Hannes Reinecke wrote:
> On 2/26/25 19:45, Daniel Wagner wrote:
> > The tport object is created via nvmet_fc_register_targetport and freed
> > via nvmet_fc_unregister_targetport. That means after the port is
> > unregistered nothing should use it. Thus ensure with refcounting
> > that there is no user left before the unregister step.
> > 
> > Signed-off-by: Daniel Wagner <wagi@...nel.org>
> > ---
> >   drivers/nvme/target/fcloop.c | 52 +++++++++++++++++++++++++++++---------------
> >   1 file changed, 35 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> > index 80693705c069dd114b2d4f15d0482dd2d713a273..2269b4d20af2ef9bb423617b94a5f5326ea124bd 100644
> > --- a/drivers/nvme/target/fcloop.c
> > +++ b/drivers/nvme/target/fcloop.c
> > @@ -233,8 +233,12 @@ struct fcloop_tport {
> >   	spinlock_t			lock;
> >   	struct list_head		ls_list;
> >   	struct work_struct		ls_work;
> > +	struct kref			ref;
> >   };
> > +static int fcloop_tport_get(struct fcloop_tport *tport);
> > +static void fcloop_tport_put(struct fcloop_tport *tport);
> > +
> >   struct fcloop_nport {
> >   	struct fcloop_rport *rport;
> >   	struct fcloop_tport *tport;
> > @@ -426,6 +430,7 @@ fcloop_tport_lsrqst_work(struct work_struct *work)
> >   		spin_lock(&tport->lock);
> >   	}
> >   	spin_unlock(&tport->lock);
> > +	fcloop_tport_put(tport);
> >   }
> >   static int
> > @@ -444,12 +449,16 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
> >   	tls_req->lsreq = lsreq;
> >   	INIT_LIST_HEAD(&tls_req->ls_list);
> > +	if (!tport)
> > +		return -ECONNABORTED;
> > +
> >   	if (!tport->remoteport) {
> >   		tls_req->status = -ECONNREFUSED;
> >   		spin_lock(&tport->lock);
> >   		list_add_tail(&tls_req->ls_list, &tport->ls_list);
> >   		spin_unlock(&tport->lock);
> > -		queue_work(nvmet_wq, &tport->ls_work);
> > +		if (queue_work(nvmet_wq, &tport->ls_work))
> > +			fcloop_tport_get(tport);
> 
> Don't you need to remove the 'tls_req' from the list, too, seeing that
> it'll never be transferred?

Good point. I'll add the error handling.

> > @@ -1576,8 +1597,7 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
> >   	if (!nport)
> >   		return -ENOENT;
> > -	ret = __targetport_unreg(nport, tport);
> > -
> > +	fcloop_tport_put(tport);
> >   	fcloop_nport_put(nport);
> Hmm. Are we sure that the 'tport' reference is always > 1 here? Otherwise
> we'll end up with a funny business when the tport is deleted
> yet the nport still has a reference ..

Yes, I am very sure about this. This doesn't mean it needs to stay like
this. The goal here is to avoid changing the existing structure of the
code and only annotate the life time of the different objects with ref
counters, so we get rid of the implicit 'rules' when it's safe to
access a pointer and when not.

Anyway, I really don't mind getting this sorted out eventually, but
please not in this series.