linux-kernel - Re: [PATCH v5 10/14] nvmet-fcloop: don't wait for lport cleanup

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: <c1f207f3-6657-4803-90df-a059353ba6da@flourine.local>
Date: Thu, 24 Apr 2025 13:48:52 +0200
From: Daniel Wagner <dwagner@...e.de>
To: Hannes Reinecke <hare@...e.de>
Cc: Daniel Wagner <wagi@...nel.org>, 
	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 v5 10/14] nvmet-fcloop: don't wait for lport cleanup

On Thu, Apr 24, 2025 at 12:21:09PM +0200, Hannes Reinecke wrote:
> > @@ -1776,7 +1763,7 @@ static void __exit fcloop_exit(void)
> >   		spin_unlock_irqrestore(&fcloop_lock, flags);
> > -		ret = __wait_localport_unreg(lport);
> > +		ret = __localport_unreg(lport);
> >   		if (ret)
> >   			pr_warn("%s: Failed deleting local port\n", __func__);
> > 
> And who is freeing the allocated fcloop_lsreq structures?

After a fcloop_lsreq is allocated it is put on either rport->ls_list or
tport->ls_list and later freed in fcloop_tport_lsrqst_work, resp.
fcloop_rport_lsrqst_work for the non-error case (or in the callbacks).

That means when the localport is unregistered successfully there are no
fcloop_lsreq around. The issue is what to do when __localport_unreq
returns an error.

This code could loop forever when the port_state is not in
FC_OBJSTATE_ONLINE when calling nvme_fc_unregister_localport(). This
should not happen (it did before this series) but now it shouldn't be
the case anymore. I suppose a pr_warn_once() and a sleep might be a good
idea to avoid a busy loop?