Re: [PATCH 08/11] nvmet-fc: take tgtport reference only once

[Hannes Reinecke <hare@suse.de>]

On 2/26/25 19:46, Daniel Wagner wrote:
> The reference counting code can be simplified. Instead taking a tgtport
> refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
> if not a new hostport object is allocated, only take it when a new
> hostport object is allocated.
> 
Can it really?
Main point of this operation is that 'tgtport' isn't going away during 
while we're figuring out whether we need it.

With this patch it means that

> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index b807b4c05cac7fe4764df3df76f8fa50f4bab6ba..391917b4ce0115dbc0ad99d1fb363b1af6ee0685 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1046,29 +1046,16 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   	if (!hosthandle)
>   		return NULL;
>   
> -	/*
> -	 * take reference for what will be the newly allocated hostport if
> -	 * we end up using a new allocation
> -	 */
> -	if (!nvmet_fc_tgtport_get(tgtport))
> -		return ERR_PTR(-EINVAL);
> -
 >   	spin_lock_irqsave(&tgtport->lock, flags);>   	match = 
nvmet_fc_match_hostport(tgtport, hosthandle);
>   	spin_unlock_irqrestore(&tgtport->lock, flags);
>   

'tgtport' might be invalid here, causing a crash when taking the lock.

> -	if (match) {
> -		/* no new allocation - release reference */
> -		nvmet_fc_tgtport_put(tgtport);
> +	if (match)
>   		return match;
> -	}
>   
>   	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
> -	if (!newhost) {
> -		/* no new allocation - release reference */
> -		nvmet_fc_tgtport_put(tgtport);
> +	if (!newhost)
>   		return ERR_PTR(-ENOMEM);
> -	}
>   
>   	spin_lock_irqsave(&tgtport->lock, flags);
>   	match = nvmet_fc_match_hostport(tgtport, hosthandle);
> @@ -1077,6 +1064,7 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   		kfree(newhost);
>   		newhost = match;
>   	} else {
> +		nvmet_fc_tgtport_get(tgtport);
>   		newhost->tgtport = tgtport;
>   		newhost->hosthandle = hosthandle;
>   		INIT_LIST_HEAD(&newhost->host_list);
> 

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