Re: [PATCH v5 01/14] nvmet-fcloop: track ref counts for nports

[Hannes Reinecke <hare@suse.de>]

On 4/23/25 15:21, Daniel Wagner wrote:
> A nport object is always used in association with targerport,
> remoteport, tport and rport objects. Add explicit references for any of
> the associated object. This ensures that nport is not removed too early
> on shutdown sequences.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 133 +++++++++++++++++++++++++++++--------------
>   1 file changed, 90 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 641201e62c1bafa13986642c6c4067b35f784edd..2b23e43ef4403fa4d70c66263f7750165d2ddc72 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1047,8 +1047,14 @@ static void
>   fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
>   {
>   	struct fcloop_rport *rport = remoteport->private;
> +	unsigned long flags;
>   
>   	flush_work(&rport->ls_work);
> +
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	rport->nport->rport = NULL;
> +	spin_unlock_irqrestore(&fcloop_lock, flags);
> +
>   	fcloop_nport_put(rport->nport);
>   }
>   
> @@ -1056,8 +1062,14 @@ static void
>   fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
>   {
>   	struct fcloop_tport *tport = targetport->private;
> +	unsigned long flags;
>   
>   	flush_work(&tport->ls_work);
> +
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	tport->nport->tport = NULL;
> +	spin_unlock_irqrestore(&fcloop_lock, flags);
> +
>   	fcloop_nport_put(tport->nport);
>   }
>   
> @@ -1184,6 +1196,37 @@ __wait_localport_unreg(struct fcloop_lport *lport)
>   	return ret;
>   }
>   
> +static struct fcloop_nport *
> +__fcloop_nport_lookup(u64 node_name, u64 port_name)
> +{
> +	struct fcloop_nport *nport;
> +
> +	list_for_each_entry(nport, &fcloop_nports, nport_list) {
> +		if (nport->node_name != node_name ||
> +		    nport->port_name != port_name)
> +			continue;
> +
> +		if (fcloop_nport_get(nport))
> +			return nport;
> +
> +		break;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fcloop_nport *
> +fcloop_nport_lookup(u64 node_name, u64 port_name)
> +{
> +	struct fcloop_nport *nport;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	nport = __fcloop_nport_lookup(node_name, port_name);
> +	spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> +	return nport;
> +}
>   
>   static ssize_t
>   fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
> @@ -1365,6 +1408,8 @@ __unlink_remote_port(struct fcloop_nport *nport)
>   {
>   	struct fcloop_rport *rport = nport->rport;
>   
> +	lockdep_assert_held(&fcloop_lock);
> +
>   	if (rport && nport->tport)
>   		nport->tport->remoteport = NULL;
>   	nport->rport = NULL;
> @@ -1377,9 +1422,6 @@ __unlink_remote_port(struct fcloop_nport *nport)
>   static int
>   __remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
>   {
> -	if (!rport)
> -		return -EALREADY;
> -
>   	return nvme_fc_unregister_remoteport(rport->remoteport);
>   }
>   
> @@ -1387,8 +1429,8 @@ static ssize_t
>   fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>   		const char *buf, size_t count)
>   {
> -	struct fcloop_nport *nport = NULL, *tmpport;
> -	static struct fcloop_rport *rport;
> +	struct fcloop_nport *nport;
> +	struct fcloop_rport *rport;
>   	u64 nodename, portname;
>   	unsigned long flags;
>   	int ret;
> @@ -1397,24 +1439,24 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>   	if (ret)
>   		return ret;
>   
> -	spin_lock_irqsave(&fcloop_lock, flags);
> -
> -	list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> -		if (tmpport->node_name == nodename &&
> -		    tmpport->port_name == portname && tmpport->rport) {
> -			nport = tmpport;
> -			rport = __unlink_remote_port(nport);
> -			break;
> -		}
> -	}
> +	nport = fcloop_nport_lookup(nodename, portname);
> +	if (!nport)
> +		return -ENOENT;
>   
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	rport = __unlink_remote_port(nport);
>   	spin_unlock_irqrestore(&fcloop_lock, flags);
>   
> -	if (!nport)
> -		return -ENOENT;
> +	if (!rport) {
> +		ret = -ENOENT;
> +		goto out_nport_put;
> +	}
>   
>   	ret = __remoteport_unreg(nport, rport);
>   
> +out_nport_put:
> +	fcloop_nport_put(nport);
> +
>   	return ret ? ret : count;
>   }
>   
> @@ -1465,6 +1507,8 @@ __unlink_target_port(struct fcloop_nport *nport)
>   {
>   	struct fcloop_tport *tport = nport->tport;
>   
> +	lockdep_assert_held(&fcloop_lock);
> +
>   	if (tport && nport->rport)
>   		nport->rport->targetport = NULL;
>   	nport->tport = NULL;
> @@ -1475,9 +1519,6 @@ __unlink_target_port(struct fcloop_nport *nport)
>   static int
>   __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
>   {
> -	if (!tport)
> -		return -EALREADY;
> -
>   	return nvmet_fc_unregister_targetport(tport->targetport);
>   }
>   
> @@ -1485,8 +1526,8 @@ static ssize_t
>   fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>   		const char *buf, size_t count)
>   {
> -	struct fcloop_nport *nport = NULL, *tmpport;
> -	struct fcloop_tport *tport = NULL;
> +	struct fcloop_nport *nport;
> +	struct fcloop_tport *tport;
>   	u64 nodename, portname;
>   	unsigned long flags;
>   	int ret;
> @@ -1495,24 +1536,24 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>   	if (ret)
>   		return ret;
>   
> -	spin_lock_irqsave(&fcloop_lock, flags);
> -
> -	list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> -		if (tmpport->node_name == nodename &&
> -		    tmpport->port_name == portname && tmpport->tport) {
> -			nport = tmpport;
> -			tport = __unlink_target_port(nport);
> -			break;
> -		}
> -	}
> +	nport = fcloop_nport_lookup(nodename, portname);
> +	if (!nport)
> +		return -ENOENT;
>   
> +	spin_lock_irqsave(&fcloop_lock, flags);
> +	tport = __unlink_target_port(nport);
>   	spin_unlock_irqrestore(&fcloop_lock, flags);
>   
Hmm. This now has a race condition; we're taking the lock
during lokup, drop the lock, take the lock again, and unlink
the port.
Please do a __fcloop_nport_lookup() function which doesn't
take a lock and avoid this race.

> -	if (!nport)
> -		return -ENOENT;
> +	if (!tport) {
> +		ret = -ENOENT;
> +		goto out_nport_put;
> +	}
>   
>   	ret = __targetport_unreg(nport, tport);
>   
> +out_nport_put:
> +	fcloop_nport_put(nport);
> +
>   	return ret ? ret : count;
>   }
>   
> @@ -1609,8 +1650,8 @@ static int __init fcloop_init(void)
>   
>   static void __exit fcloop_exit(void)
>   {
> -	struct fcloop_lport *lport = NULL;
> -	struct fcloop_nport *nport = NULL;
> +	struct fcloop_lport *lport;
> +	struct fcloop_nport *nport;
>   	struct fcloop_tport *tport;
>   	struct fcloop_rport *rport;
>   	unsigned long flags;
> @@ -1621,7 +1662,7 @@ static void __exit fcloop_exit(void)
>   	for (;;) {
>   		nport = list_first_entry_or_null(&fcloop_nports,
>   						typeof(*nport), nport_list);
> -		if (!nport)
> +		if (!nport || !fcloop_nport_get(nport))
>   			break;
>   
>   		tport = __unlink_target_port(nport);
> @@ -1629,13 +1670,19 @@ static void __exit fcloop_exit(void)
>   
>   		spin_unlock_irqrestore(&fcloop_lock, flags);
>   
> -		ret = __targetport_unreg(nport, tport);
> -		if (ret)
> -			pr_warn("%s: Failed deleting target port\n", __func__);
> +		if (tport) {
> +			ret = __targetport_unreg(nport, tport);
> +			if (ret)
> +				pr_warn("%s: Failed deleting target port\n", __func__);
> +		}
>   
And same here; don't drop the lock after lookup.

> -		ret = __remoteport_unreg(nport, rport);
> -		if (ret)
> -			pr_warn("%s: Failed deleting remote port\n", __func__);
> +		if (rport) {
> +			ret = __remoteport_unreg(nport, rport);
> +			if (ret)
> +				pr_warn("%s: Failed deleting remote port\n", __func__);
> +		}
> +
> +		fcloop_nport_put(nport);
>   
>   		spin_lock_irqsave(&fcloop_lock, flags);
>   	}
> 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich