linux-kernel - Re: [PATCH 04/11] nvmet-fcloop: track ref counts for nports

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <0f8504e1-c890-41b2-9cfb-2b45cd92e6ad@flourine.local>
Date: Fri, 28 Feb 2025 09:09:04 +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 04/11] nvmet-fcloop: track ref counts for nports

On Fri, Feb 28, 2025 at 08:19:19AM +0100, Hannes Reinecke wrote:
> >   static void
> >   fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
> >   {
> > -	struct fcloop_tport *tport = targetport->private;
> > -
> > -	flush_work(&tport->ls_work);
> > -	fcloop_nport_put(tport->nport);
> >   }
> Errm. Isn't this function empty now? Can't it be remove altogether?

Sure, I'll get rid of it.

> >   	nport = fcloop_alloc_nport(buf, count, false);
> >   	if (!nport)
> >   		return -EIO;
> > @@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> >   		return ret;
> >   	}
> > +	/* nport ref get: targetport */
> > +	fcloop_nport_get(nport);
> > +
> 
> I would rather move it to the end of the function, after we set all the
> references. But that's probably personal style...

I don't mind, During debugging I started to move the get/put function
where the pointers are updated, to highlight why we do the get/put. But
that was still pretty hard to find the leaks. Eventually I added some
debugging patches which annotated the call. The left over of these debug
patches are the 'nport ref get: transport' comments (*). Now that I am
confident that all is in balance, there is no reason not to make the
code more streamlined.

(*) Would something like this here be a no go? It really helps when
trying to debug the inbalance:

+static void __nvmet_fc_tgtport_put(struct nvmet_fc_tgtport *tgtport);
+static int __nvmet_fc_tgtport_get(struct nvmet_fc_tgtport *tgtport);
+
+#if 1
+#define nvmet_fc_tgtport_put(p)						\
+({									\
+	pr_info("nvmet_fc_tgtport_put: %px %d %s:%d\n",			\
+		p, atomic_read(&p->ref.refcount.refs),			\
+		__func__, __LINE__);					\
+	__nvmet_fc_tgtport_put(p);					\
+})
+
+#define nvmet_fc_tgtport_get(p)						\
+({									\
+	int ___r = __nvmet_fc_tgtport_get(p);				\
+									\
+	pr_info("nvmet_fc_tgtport_get: %px %d %s:%d\n",			\
+		p, atomic_read(&p->ref.refcount.refs),			\
+		__func__, __LINE__);					\
+	___r;								\
+})
+#else
+#define nvmet_fc_tgtport_put(p) __nvmet_fc_tgtport_put(p)
+#define nvmet_fc_tgtport_get(p) __nvmet_fc_tgtport_get(p)
+#endif