The nvmet-fc uses references on the targetport to ensure no UAFs
happens. The consequence is that when the targetport is unregistered,
not all resources are freed immediately. Ensure that all activities from
the unregister call have been submitted (deassocication) before
continuing with the shutdown sequence.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/target/fcloop.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 69121a5f0f280936d1b720e9e994d6e5eb9186ff..cddaa424bb3ff62156cef14c787fdcb33c15d76e 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -239,6 +239,7 @@ struct fcloop_nport {
 	struct fcloop_rport *rport;
 	struct fcloop_tport *tport;
 	struct fcloop_lport *lport;
+	struct completion tport_unreg_done;
 	struct list_head nport_list;
 	refcount_t ref;
 	u64 node_name;
@@ -1078,6 +1079,8 @@ fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
 	tport->nport->tport = NULL;
 	spin_unlock_irqrestore(&fcloop_lock, flags);
 
+	complete(&tport->nport->tport_unreg_done);
+
 	/* nport ref put: tport */
 	fcloop_nport_put(tport->nport);
 }
@@ -1507,7 +1510,17 @@ __unlink_target_port(struct fcloop_nport *nport)
 static int
 __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
 {
-	return nvmet_fc_unregister_targetport(tport->targetport);
+	int ret;
+
+	init_completion(&nport->tport_unreg_done);
+
+	ret = nvmet_fc_unregister_targetport(tport->targetport);
+	if (ret)
+		return ret;
+
+	wait_for_completion(&nport->tport_unreg_done);
+
+	return 0;
 }
 
 static ssize_t

-- 
2.48.1

On Tue, Mar 18, 2025 at 11:40:00AM +0100, Daniel Wagner wrote:
> The nvmet-fc uses references on the targetport to ensure no UAFs
> happens. The consequence is that when the targetport is unregistered,
> not all resources are freed immediately. Ensure that all activities from
> the unregister call have been submitted (deassocication) before
> continuing with the shutdown sequence.

This needs to explain why that is needed.  In general a completion
waiting for references to go away is a bit of an anti-patter, so
it should come with a good excuse.

On Fri, Mar 21, 2025 at 07:08:32AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 18, 2025 at 11:40:00AM +0100, Daniel Wagner wrote:
> > The nvmet-fc uses references on the targetport to ensure no UAFs
> > happens. The consequence is that when the targetport is unregistered,
> > not all resources are freed immediately. Ensure that all activities from
> > the unregister call have been submitted (deassocication) before
> > continuing with the shutdown sequence.
> 
> This needs to explain why that is needed.  In general a completion
> waiting for references to go away is a bit of an anti-patter, so
> it should come with a good excuse.

FWIW, I was able to drop this patch and also the similar code for the
lport object. The reason is with the 'nvmet-fcloop: allocate/free
fcloop_lsreq directly' patch this is not necessary anymore.

On 3/18/25 11:40, Daniel Wagner wrote:
> The nvmet-fc uses references on the targetport to ensure no UAFs
> happens. The consequence is that when the targetport is unregistered,
> not all resources are freed immediately. Ensure that all activities from
> the unregister call have been submitted (deassocication) before
> continuing with the shutdown sequence.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 69121a5f0f280936d1b720e9e994d6e5eb9186ff..cddaa424bb3ff62156cef14c787fdcb33c15d76e 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -239,6 +239,7 @@ struct fcloop_nport {
>   	struct fcloop_rport *rport;
>   	struct fcloop_tport *tport;
>   	struct fcloop_lport *lport;
> +	struct completion tport_unreg_done;
>   	struct list_head nport_list;
>   	refcount_t ref;
>   	u64 node_name;
> @@ -1078,6 +1079,8 @@ fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
>   	tport->nport->tport = NULL;
>   	spin_unlock_irqrestore(&fcloop_lock, flags);
>   
> +	complete(&tport->nport->tport_unreg_done);
> +
>   	/* nport ref put: tport */
>   	fcloop_nport_put(tport->nport);
>   }
> @@ -1507,7 +1510,17 @@ __unlink_target_port(struct fcloop_nport *nport)
>   static int
>   __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
>   {
> -	return nvmet_fc_unregister_targetport(tport->targetport);
> +	int ret;
> +
> +	init_completion(&nport->tport_unreg_done);
> +
> +	ret = nvmet_fc_unregister_targetport(tport->targetport);
> +	if (ret)
> +		return ret;
> +
> +	wait_for_completion(&nport->tport_unreg_done);
> +
> +	return 0;
>   }
>   
>   static ssize_t
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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