linux-kernel - Re: [PATCH 03/11] nvmet-fcloop: refactor fcloop_nport

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: <0e41c69f-32f0-4c19-8d52-04d767acbaed@flourine.local>
Date: Fri, 28 Feb 2025 08:56:58 +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 03/11] nvmet-fcloop: refactor fcloop_nport_alloc

On Fri, Feb 28, 2025 at 08:11:11AM +0100, Hannes Reinecke wrote:
> > +	nport = fcloop_nport_lookup(opts->wwnn, opts->wwpn);
> > +	if (nport && ((remoteport && nport->rport) ||
> > +		      (!remoteport && nport->tport))) {
> > +		/* invalid configuration */
> > +		goto out_put_nport;
> > +	}
> > -	spin_lock_irqsave(&fcloop_lock, flags);
> > +	if (!nport) {
> > +		nport = kzalloc(sizeof(*nport), GFP_KERNEL);
> > +		if (!nport)
> > +			goto out_free_opts;
> > -	list_for_each_entry(tmplport, &fcloop_lports, lport_list) {
> > -		if (tmplport->localport->node_name == opts->wwnn &&
> > -		    tmplport->localport->port_name == opts->wwpn)
> > -			goto out_invalid_opts;
> > +		INIT_LIST_HEAD(&nport->nport_list);
> > +		nport->node_name = opts->wwnn;
> > +		nport->port_name = opts->wwpn;
> > +		kref_init(&nport->ref);
> > -		if (tmplport->localport->node_name == opts->lpwwnn &&
> > -		    tmplport->localport->port_name == opts->lpwwpn)
> > -			lport = tmplport;
> > +		spin_lock_irqsave(&fcloop_lock, flags);
> > +		list_add_tail(&nport->nport_list, &fcloop_nports);
> > +		spin_unlock_irqrestore(&fcloop_lock, flags);
> 
> Don't you need to check here if an 'nport' with the same node_name and
> port_name is already present?

There is the existing check which filters out some of the duplicates
(the check is there to allow setting up the target or the remote port
first, so the order doesn't matter), though I am not sure if it would
catch all duplicates. I don't mind adding this, but I'd say it would be
better in a separate patch. I tried to refactor this code without
changing anything else.