Re: [PATCH 1/3] nvmet/fc: Sanitize tgtport reference counting for LS requests

[James Smart <james.smart@broadcom.com>]

On 3/6/2020 5:04 AM, Hannes Reinecke wrote:
> We need to validate the targetport upon first access, otherwise
> there's a risk of accessing an invalid targetport.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/target/fc.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 0a0f03b2faf3..aff959300ef3 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -358,6 +358,7 @@ __nvmet_fc_finish_ls_req(struct nvmet_fc_ls_req_op *lsop)
>   
>   	if (!lsop->req_queued) {
>   		spin_unlock_irqrestore(&tgtport->lock, flags);
> +		nvmet_fc_tgtport_put(tgtport);
>   		return;
>   	}
>   
> @@ -386,9 +387,6 @@ __nvmet_fc_send_ls_req(struct nvmet_fc_tgtport *tgtport,
>   	if (!tgtport->ops->ls_req)
>   		return -EOPNOTSUPP;
>   
> -	if (!nvmet_fc_tgtport_get(tgtport))
> -		return -ESHUTDOWN;
> -
>   	lsreq->done = done;
>   	lsop->req_queued = false;
>   	INIT_LIST_HEAD(&lsop->lsreq_list);
> @@ -396,10 +394,9 @@ __nvmet_fc_send_ls_req(struct nvmet_fc_tgtport *tgtport,
>   	lsreq->rqstdma = fc_dma_map_single(tgtport->dev, lsreq->rqstaddr,
>   				  lsreq->rqstlen + lsreq->rsplen,
>   				  DMA_BIDIRECTIONAL);
> -	if (fc_dma_mapping_error(tgtport->dev, lsreq->rqstdma)) {
> -		ret = -EFAULT;
> -		goto out_puttgtport;
> -	}
> +	if (fc_dma_mapping_error(tgtport->dev, lsreq->rqstdma))
> +		return -EFAULT;
> +
>   	lsreq->rspdma = lsreq->rqstdma + lsreq->rqstlen;
>   
>   	spin_lock_irqsave(&tgtport->lock, flags);
> @@ -426,9 +423,6 @@ __nvmet_fc_send_ls_req(struct nvmet_fc_tgtport *tgtport,
>   	fc_dma_unmap_single(tgtport->dev, lsreq->rqstdma,
>   				  (lsreq->rqstlen + lsreq->rsplen),
>   				  DMA_BIDIRECTIONAL);
> -out_puttgtport:
> -	nvmet_fc_tgtport_put(tgtport);
> -
>   	return ret;
>   }
>   
> @@ -482,14 +476,19 @@ nvmet_fc_xmt_disconnect_assoc(struct nvmet_fc_tgt_assoc *assoc)
>   	struct nvmefc_ls_req *lsreq;
>   	int ret;
>   
> +	if (!nvmet_fc_tgtport_get(tgtport))
> +		return;
> +
>   	/*
>   	 * If ls_req is NULL or no hosthandle, it's an older lldd and no
>   	 * message is normal. Otherwise, send unless the hostport has
>   	 * already been invalidated by the lldd.
>   	 */
>   	if (!tgtport->ops->ls_req || !assoc->hostport ||
> -	    assoc->hostport->invalid)
> +	    assoc->hostport->invalid) {
> +		nvmet_fc_tgtport_put(tgtport);
>   		return;
> +	}
>   
>   	lsop = kzalloc((sizeof(*lsop) +
>   			sizeof(*discon_rqst) + sizeof(*discon_acc) +
> @@ -498,6 +497,7 @@ nvmet_fc_xmt_disconnect_assoc(struct nvmet_fc_tgt_assoc *assoc)
>   		dev_info(tgtport->dev,
>   			"{%d:%d} send Disconnect Association failed: ENOMEM\n",
>   			tgtport->fc_target_port.port_num, assoc->a_id);
> +		nvmet_fc_tgtport_put(tgtport);
>   		return;
>   	}
>   
> @@ -522,6 +522,7 @@ nvmet_fc_xmt_disconnect_assoc(struct nvmet_fc_tgt_assoc *assoc)
>   			"{%d:%d} XMT Disconnect Association failed: %d\n",
>   			tgtport->fc_target_port.port_num, assoc->a_id, ret);
>   		kfree(lsop);
> +		nvmet_fc_tgtport_put(tgtport);
>   	}
>   }
>   

I guess I don't see anything wrong with this, but it seems messier to 
handle the ref counting.  I prefer it to be on the send/completion 
localized to the common op processing.  Also creates a different 
implementation than what was done on the host side, even though the rest 
is very similar.

Reviewed-by: James Smart <james.smart@broadcom.com>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme