Re: [PATCH v13 2/2] zfcp: Use DMA streaming API to map I/O

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] scsi: zfcp: premature DMA mapping and missing sync of QTCB in zfcp_fsf
- [Critical] scsi: zfcp: premature DMA unmap before abort in zfcp_scsi_eh_abort_handler()
- [High] scsi: zfcp: stale SBAL entries left in req_q on DMA map error paths
- [Medium] scsi: zfcp: stale sg_table nents in zfcp_fc_exec_bsg_job() truncates DMA map
--

commit 55b15d18ad86f261b72e93af06572f7a0dd07799
Author: M Nikhil <nikh1092@linux.ibm.com>

zfcp: Use DMA streaming API to map I/O

This commit converts zFCP I/O buffer handling (QTCB, SRB, CT/ELS, and SCSI
I/O buffers) from direct physical address translation to the DMA streaming
API using sg_table, dma_map_sgtable(), and dma_unmap_sgtable().

> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index 12b3d2d0a2c87..2031055c48f8a 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -14,6 +14,7 @@
>  #include <linux/types.h>
>  #include <linux/slab.h>
>  #include <scsi/fc/fc_els.h>
> +#include <linux/dma-mapping.h>
>  #include "zfcp_ext.h"
>  #include "zfcp_fc.h"
>  #include "zfcp_dbf.h"
> @@ -84,6 +85,86 @@ static void zfcp_fsf_class_not_supp(struct zfcp_fsf_req *req)
>  	req->status |= ZFCP_STATUS_FSFREQ_ERROR;
>  }
>  
> +/**
> + * zfcp_fsf_req_map_sg - map the scattergather list for DMA transfer
> + * @req: pointer to zfcp_fsf_req
> + * @sgt: pointer to scattergather list table
> + * @dma_dir: DMA data direction
> + * Returns: 0 on success, -EIO otherwise
> + */
> +int zfcp_fsf_req_map_sg(struct zfcp_fsf_req *req, struct sg_table *sgt,
> +			enum dma_data_direction dma_dir)
> +{
> +	unsigned int saved_orig_nents;
> +	int ret;
> +
> +	if (!valid_dma_direction(dma_dir))
> +		return sgt->orig_nents ? -EIO : 0;
> +
> +	if (!sgt->orig_nents)
> +		return 0;
> +
> +	/*
> +	 * The SCSI midlayer sets orig_nents to the allocated SG capacity and
> +	 * nents to the actually populated entry count (nents <= orig_nents).
> +	 * dma_map_sgtable() and dma_unmap_sgtable() both use orig_nents, so
> +	 * clamp it to the populated count to avoid mapping trailing zero-length
> +	 * entries. Only clamp when nents has been explicitly set (nents > 0);
> +	 * zFCP-internal sg_tables leave nents = 0 before mapping, relying
> +	 * solely on orig_nents.
> +	 */
> +	saved_orig_nents = sgt->orig_nents;
> +	if (sgt->nents > 0 && sgt->nents < sgt->orig_nents)
> +		sgt->orig_nents = sgt->nents;
> +
> +	ret = dma_map_sgtable(&req->adapter->ccw_device->dev, sgt, dma_dir, 0);
> +
> +	sgt->orig_nents = saved_orig_nents;
> +
> +	return ret;
> +}
> +
> +/**
> + * zfcp_fsf_req_unmap_sg - unmap the scattergather list after DMA transfer
> + * @req: pointer to zfcp_fsf_req
> + * @sgt: pointer to scattergather list table
> + * @dma_dir: DMA direction of the sg
> + */
> +void zfcp_fsf_req_unmap_sg(struct zfcp_fsf_req *req, struct sg_table *sgt,
> +			   enum dma_data_direction dma_dir)
> +{
> +	unsigned int saved_orig_nents;
> +
> +	/*
> +	 * We still need the check for mapped sg count here to not perform
> +	 * unmap for SCSI commands which have zero payload.
> +	 */
> +	if (!sgt->nents)
> +		return;
> +
> +	saved_orig_nents = sgt->orig_nents;
> +	if (sgt->nents < sgt->orig_nents)
> +		sgt->orig_nents = sgt->nents;
> +
> +	dma_unmap_sgtable(&req->adapter->ccw_device->dev, sgt, dma_dir, 0);
> +
> +	sgt->orig_nents = saved_orig_nents;
> +}

[Severity: Medium]
Can a reused BSG request tag leave a stale sgt->nents value in
ct_els->sgt_req or ct_els->sgt_resp and cause zfcp_fsf_req_map_sg() to
truncate the DMA mapping?

In drivers/s390/scsi/zfcp_fsf.c, zfcp_fsf_req_map_sg() assumes that
zFCP-internal sg_table structures always have sgt->nents == 0 prior to
mapping, and clamps sgt->orig_nents to sgt->nents whenever
0 < sgt->nents < sgt->orig_nents. When the request completes,
zfcp_fsf_req_unmap_sg() leaves sgt->nents set to the mapped entry count.

However, in the FC BSG path:

  bsg_transport_sg_io_fn()
    bsg_queue_rq()
      fc_bsg_dispatch()
        zfcp_fc_exec_bsg_job()
          zfcp_fsf_send_ct() / zfcp_fsf_send_els()
            zfcp_fsf_setup_ct_els()
              zfcp_fsf_setup_ct_els_sbals()
                zfcp_qdio_sbals_from_sg()
                  zfcp_fsf_req_map_sg()

bsg_transport_sg_io_fn() only zeroes sizeof(struct bsg_job) and does not
clear job->dd_data (struct zfcp_fsf_ct_els) in the blk-mq request PDU. In
drivers/s390/scsi/zfcp_fc.c, zfcp_fc_exec_bsg_job() initializes sgl and
orig_nents without resetting nents:

	ct_els->sgt_req.sgl = job->request_payload.sg_list;
	ct_els->sgt_resp.sgl = job->reply_payload.sg_list;
	ct_els->sgt_req.orig_nents = sg_nents(ct_els->sgt_req.sgl);
	ct_els->sgt_resp.orig_nents = sg_nents(ct_els->sgt_resp.sgl);

If a BSG request tag is reused for a multi-segment payload after a command
that mapped fewer SG segments, won't zfcp_fsf_req_map_sg() clamp orig_nents
to the stale sgt->nents value and map only part of the scatterlist into
SBALEs while zfcp_qdio_real_bytes() sets req_buf_length and resp_buf_length
to the full byte count, causing the adapter to reject the request with
FSF_SBAL_MISMATCH?

[ ... ]

> @@ -870,10 +954,27 @@ static struct zfcp_fsf_req *zfcp_fsf_req_create(struct zfcp_qdio *qdio,
>  		req->qtcb->prefix.qtcb_version = FSF_QTCB_CURRENT_VERSION;
>  		req->qtcb->header.req_handle = req->req_id;
>  		req->qtcb->header.fsf_command = fsf_cmd;
> -	}
>  
> -	zfcp_qdio_req_init(adapter->qdio, &req->qdio_req, req->req_id, sbtype,
> -			   req->qtcb, sizeof(struct fsf_qtcb));
> +		sg_init_one(&req->metadata_sg, req->qtcb,
> +			    sizeof(struct fsf_qtcb));
> +		req->metadata_sgt.sgl = &req->metadata_sg;
> +		req->metadata_sgt.orig_nents = 1;
> +
> +		if (zfcp_fsf_req_map_sg(req, &req->metadata_sgt,
> +					DMA_BIDIRECTIONAL)) {
> +			zfcp_fsf_req_free(req);
> +			return ERR_PTR(-EIO);
> +		}
> +
> +		zfcp_qdio_req_init(adapter->qdio, &req->qdio_req, req->req_id,
> +				   sbtype, true,
> +				   sg_dma_len(req->metadata_sgt.sgl),
> +				   sg_dma_address(req->metadata_sgt.sgl));
> +	} else {
> +		/* SRB */
> +		zfcp_qdio_req_init(adapter->qdio, &req->qdio_req, req->req_id,
> +				   sbtype, false, 0, 0);
> +	}
>  
>  	return req;
>  }

[ ... ]

> @@ -1130,19 +1247,41 @@ static void zfcp_fsf_send_ct_handler(struct zfcp_fsf_req *req)
>  		ct->handler(ct->handler_data);
>  }
>  
> -static void zfcp_fsf_setup_ct_els_unchained(struct zfcp_qdio *qdio,
> -					    struct zfcp_qdio_req *q_req,
> -					    struct scatterlist *sg_req,
> -					    struct scatterlist *sg_resp)
> +static int zfcp_fsf_setup_ct_els_unchained(struct zfcp_fsf_req *req,
> +					   struct zfcp_qdio *qdio,
> +					   struct sg_table *sg_req_table,
> +					   struct sg_table *sg_resp_table)
>  {
> -	zfcp_qdio_fill_next(qdio, q_req, sg_virt(sg_req), sg_req->length);
> -	zfcp_qdio_fill_next(qdio, q_req, sg_virt(sg_resp), sg_resp->length);
> -	zfcp_qdio_set_sbale_last(qdio, q_req);
> +	int retval = 0;
> +
> +	retval = zfcp_fsf_req_map_sg(req, sg_req_table, DMA_TO_DEVICE);
> +	if (unlikely(retval))
> +		goto err;
> +
> +	retval = zfcp_fsf_req_map_sg(req, sg_resp_table,
> +				     DMA_FROM_DEVICE);
> +	if (unlikely(retval))
> +		goto err_unmap;
> +
> +	zfcp_qdio_fill_next(qdio, &req->qdio_req,
> +			    sg_dma_address(sg_req_table->sgl),
> +			    sg_dma_len(sg_req_table->sgl));
> +	zfcp_qdio_fill_next(qdio, &req->qdio_req,
> +			    sg_dma_address(sg_resp_table->sgl),
> +			    sg_dma_len(sg_resp_table->sgl));
> +	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
> +
> +	return 0;
> +
> +err_unmap:
> +	zfcp_fsf_req_unmap_sg(req, sg_req_table, DMA_TO_DEVICE);
> +err:
> +	return retval;
>  }
>  
>  static int zfcp_fsf_setup_ct_els_sbals(struct zfcp_fsf_req *req,
> -				       struct scatterlist *sg_req,
> -				       struct scatterlist *sg_resp)
> +				       struct sg_table *sgt_req,
> +				       struct sg_table *sgt_resp)
>  {
>  	struct zfcp_adapter *adapter = req->adapter;
>  	struct zfcp_qdio *qdio = adapter->qdio;
> @@ -1150,57 +1289,72 @@ static int zfcp_fsf_setup_ct_els_sbals(struct zfcp_fsf_req *req,
>  	u32 feat = adapter->adapter_features;
>  
>  	if (zfcp_adapter_multi_buffer_active(adapter)) {
> -		if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_req))
> +		if (zfcp_qdio_sbals_from_sg(qdio, sgt_req, req, DMA_TO_DEVICE))
>  			return -EIO;
> +
>  		qtcb->bottom.support.req_buf_length =
> -			zfcp_qdio_real_bytes(sg_req);
> -		if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_resp))
> -			return -EIO;
> +			zfcp_qdio_real_bytes(sgt_req->sgl);
> +
> +		if (zfcp_qdio_sbals_from_sg(qdio, sgt_resp, req, DMA_FROM_DEVICE))
> +			goto err_unmap;
> +
>  		qtcb->bottom.support.resp_buf_length =
> -			zfcp_qdio_real_bytes(sg_resp);
> +			zfcp_qdio_real_bytes(sgt_resp->sgl);
>  
> -		zfcp_qdio_set_data_div(qdio, &req->qdio_req, sg_nents(sg_req));
> +		zfcp_qdio_set_data_div(qdio, &req->qdio_req,
> +				       sgt_req->nents);
>  		zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
>  		zfcp_qdio_set_scount(qdio, &req->qdio_req);
>  		return 0;
>  	}
>  
>  	/* use single, unchained SBAL if it can hold the request */
> -	if (zfcp_qdio_sg_one_sbale(sg_req) && zfcp_qdio_sg_one_sbale(sg_resp)) {
> -		zfcp_fsf_setup_ct_els_unchained(qdio, &req->qdio_req,
> -						sg_req, sg_resp);
> +	if (zfcp_qdio_sg_one_sbale(sgt_req->sgl) &&
> +	    zfcp_qdio_sg_one_sbale(sgt_resp->sgl)) {
> +		if (zfcp_fsf_setup_ct_els_unchained(req, qdio, sgt_req,
> +						    sgt_resp))
> +			return -EIO;
> +
>  		return 0;
>  	}
>  
>  	if (!(feat & FSF_FEATURE_ELS_CT_CHAINED_SBALS))
>  		return -EOPNOTSUPP;
>  
> -	if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_req))
> +	if (zfcp_qdio_sbals_from_sg(qdio, sgt_req, req, DMA_TO_DEVICE))
>  		return -EIO;
>  
> -	qtcb->bottom.support.req_buf_length = zfcp_qdio_real_bytes(sg_req);
> +	qtcb->bottom.support.req_buf_length =
> +		zfcp_qdio_real_bytes(sgt_req->sgl);
>  
>  	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
>  	zfcp_qdio_skip_to_last_sbale(qdio, &req->qdio_req);
>  
> -	if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_resp))
> -		return -EIO;
> +	if (zfcp_qdio_sbals_from_sg(qdio, sgt_resp, req, DMA_FROM_DEVICE))
> +		goto err_unmap;
>  
> -	qtcb->bottom.support.resp_buf_length = zfcp_qdio_real_bytes(sg_resp);
> +	qtcb->bottom.support.resp_buf_length =
> +		zfcp_qdio_real_bytes(sgt_resp->sgl);
>  
>  	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
>  
>  	return 0;
> +
> +err_unmap:
> +	zfcp_fsf_req_unmap_sg(req, sgt_req, DMA_TO_DEVICE);
> +	zfcp_qdio_zero_sbals(qdio->req_q, req->qdio_req.sbal_first,
> +			     req->qdio_req.sbal_number);
> +	return -EIO;
>  }

[ ... ]

> @@ -2606,6 +2793,10 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
>  	io->service_class = FSF_CLASS_3;
>  	io->fcp_cmnd_length = FCP_CMND_LEN;
>  
> +	dma_sync_sgtable_for_device(&req->adapter->ccw_device->dev,
> +				    &req->metadata_sgt,
> +				    DMA_BIDIRECTIONAL);
> +
>  	if (scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) {
>  		io->data_block_length = scsi_prot_interval(scsi_cmnd);
>  		io->ref_tag_value = scsi_prot_ref_tag(scsi_cmnd);

[Severity: Critical]
When SWIOTLB bounce buffering is active (such as under s390 Secure Execution),
won't the device see stale/zeroed QTCB contents for SCSI commands and all
other QTCB-based FSF requests?

In drivers/s390/scsi/zfcp_fsf.c, zfcp_fsf_req_create() maps req->qtcb with
zfcp_fsf_req_map_sg(req, &req->metadata_sgt, DMA_BIDIRECTIONAL) immediately
after setting only the initial prefix and header fields. Under SWIOTLB,
dma_map_sgtable() copies req->qtcb into the bounce buffer at that point.

In zfcp_fsf_fcp_cmnd(), dma_sync_sgtable_for_device() is called before the
remaining fields in req->qtcb->bottom.io are populated:

	dma_sync_sgtable_for_device(&req->adapter->ccw_device->dev,
				    &req->metadata_sgt,
				    DMA_BIDIRECTIONAL);

	if (scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) {
		io->data_block_length = scsi_prot_interval(scsi_cmnd);
		io->ref_tag_value = scsi_prot_ref_tag(scsi_cmnd);
	}

	if (zfcp_fsf_set_data_dir(scsi_cmnd, &io->data_direction))
		goto failed_scsi_cmnd;

	BUILD_BUG_ON(sizeof(struct fcp_cmnd) > FSF_FCP_CMND_SIZE);
	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
	zfcp_fc_scsi_to_fcp(fcp_cmnd, scsi_cmnd);

As a result, io->data_block_length, io->ref_tag_value, io->data_direction,
fcp_cmnd (including the FCP LUN, CDB, and transfer length), and
io->prot_data_length are written after the sync and are never copied to the
bounce buffer.

In addition, none of the other QTCB-based FSF functions in
drivers/s390/scsi/zfcp_fsf.c (zfcp_fsf_abort_fcp_cmnd(), zfcp_fsf_send_ct(),
zfcp_fsf_send_els(), zfcp_fsf_exchange_config_data(),
zfcp_fsf_exchange_config_data_sync(), zfcp_fsf_exchange_port_data(),
zfcp_fsf_exchange_port_data_sync(), zfcp_fsf_open_port(),
zfcp_fsf_close_port(), zfcp_fsf_open_wka_port(), zfcp_fsf_close_wka_port(),
zfcp_fsf_close_physical_port(), zfcp_fsf_open_lun(), zfcp_fsf_close_lun(),
and zfcp_fsf_fcp_task_mgmt()) or zfcp_fsf_req_send() call
dma_sync_sgtable_for_device() after filling in req->qtcb.

Should req->qtcb be synced in zfcp_fsf_req_send() right before
zfcp_qdio_send(), or mapped only after req->qtcb is fully populated?

> @@ -2620,20 +2811,22 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
>  
>  	if ((scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) &&
>  	    scsi_prot_sg_count(scsi_cmnd)) {
> -		zfcp_qdio_set_data_div(qdio, &req->qdio_req,
> -				       scsi_prot_sg_count(scsi_cmnd));
> -		retval = zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req,
> -						 scsi_prot_sglist(scsi_cmnd));
> +		retval = zfcp_qdio_sbals_from_sg(qdio,
> +						 &scsi_cmnd->prot_sdb->table,
> +						 req,
> +						 scsi_cmnd->sc_data_direction);
>  		if (retval)
>  			goto failed_scsi_cmnd;
> +		zfcp_qdio_set_data_div(qdio, &req->qdio_req,
> +				       scsi_cmnd->prot_sdb->table.nents);
>  		io->prot_data_length = zfcp_qdio_real_bytes(
>  						scsi_prot_sglist(scsi_cmnd));
>  	}
>  
> -	retval = zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req,
> -					 scsi_sglist(scsi_cmnd));
> +	retval = zfcp_qdio_sbals_from_sg(qdio, &scsi_cmnd->sdb.table, req,
> +					 scsi_cmnd->sc_data_direction);
>  	if (unlikely(retval))
> -		goto failed_scsi_cmnd;
> +		goto failed_normal_scsi_map;
>  
>  	zfcp_qdio_set_sbale_last(adapter->qdio, &req->qdio_req);
>  	if (zfcp_adapter_multi_buffer_active(adapter))
> @@ -2641,12 +2834,25 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
>  
>  	retval = zfcp_fsf_req_send(req);
>  	if (unlikely(retval))
> -		goto failed_scsi_cmnd;
> +		goto failed_req_send;
> +
>  	/* NOTE: DO NOT TOUCH req PAST THIS POINT! */
>  
>  	goto out;
>  
> +failed_req_send:
> +	zfcp_fsf_req_unmap_sg(req, &scsi_cmnd->sdb.table,
> +			      scsi_cmnd->sc_data_direction);
> +failed_normal_scsi_map:
> +	if ((scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) &&
> +	    scsi_prot_sg_count(scsi_cmnd)) {
> +		zfcp_fsf_req_unmap_sg(req, &scsi_cmnd->prot_sdb->table,
> +				      scsi_cmnd->sc_data_direction);
> +		zfcp_qdio_zero_sbals(qdio->req_q, req->qdio_req.sbal_first,
> +				     req->qdio_req.sbal_number);
> +	}
>  failed_scsi_cmnd:
> +	zfcp_fsf_req_unmap_metadata(req);
>  	zfcp_fsf_req_free(req);
>  	scsi_cmnd->host_scribble = NULL;
>  out:

[ ... ]

> diff --git a/drivers/s390/scsi/zfcp_qdio.c b/drivers/s390/scsi/zfcp_qdio.c
> index bac8385e069fa..50db08388514c 100644
> --- a/drivers/s390/scsi/zfcp_qdio.c
> +++ b/drivers/s390/scsi/zfcp_qdio.c
> @@ -233,31 +233,40 @@ zfcp_qdio_sbale_next(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req)
>  }
>  
>  /**
> - * zfcp_qdio_sbals_from_sg - fill SBALs from scatter-gather list
> + * zfcp_qdio_sbals_from_sg - DMA map scatter-gather list and fill SBALs from it
>   * @qdio: pointer to struct zfcp_qdio
> - * @q_req: pointer to struct zfcp_qdio_req
> - * @sg: scatter-gather list
> - * Returns: zero or -EINVAL on error
> + * @sgt: pointer to scattergather list table
> + * @req: pointer to zfcp_fsf_req
> + * @dma_dir: DMA direction of I/O
> + * Returns: zero on success, -EINVAL or -EIO otherwise
>   */
> -int zfcp_qdio_sbals_from_sg(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req,
> -			    struct scatterlist *sg)
> +int zfcp_qdio_sbals_from_sg(struct zfcp_qdio *qdio, struct sg_table *sgt,
> +			    struct zfcp_fsf_req *req,
> +			    enum dma_data_direction dma_dir)
>  {
>  	struct qdio_buffer_element *sbale;
> +	int i;
> +	struct scatterlist *sg_element;
> +
> +	if (zfcp_fsf_req_map_sg(req, sgt, dma_dir))
> +		return -EIO;
>  
>  	/* set storage-block type for this request */
> -	sbale = zfcp_qdio_sbale_req(qdio, q_req);
> -	sbale->sflags |= q_req->sbtype;
> +	sbale = zfcp_qdio_sbale_req(qdio, &req->qdio_req);
> +	sbale->sflags |= req->qdio_req.sbtype;
>  
> -	for (; sg; sg = sg_next(sg)) {
> -		sbale = zfcp_qdio_sbale_next(qdio, q_req);
> +	for_each_sgtable_dma_sg(sgt, sg_element, i) {
> +		sbale = zfcp_qdio_sbale_next(qdio, &req->qdio_req);
>  		if (!sbale) {
>  			atomic_inc(&qdio->req_q_full);
> -			zfcp_qdio_zero_sbals(qdio->req_q, q_req->sbal_first,
> -					     q_req->sbal_number);
> +			zfcp_qdio_zero_sbals(qdio->req_q,
> +					     req->qdio_req.sbal_first,
> +					     req->qdio_req.sbal_number);
> +			zfcp_fsf_req_unmap_sg(req, sgt, dma_dir);
>  			return -EINVAL;
>  		}
> -		sbale->addr = u64_to_dma64(sg_phys(sg));
> -		sbale->length = sg->length;
> +		sbale->addr = u64_to_dma64(sg_dma_address(sg_element));
> +		sbale->length = sg_dma_len(sg_element);
>  	}
>  	return 0;
>  }
> diff --git a/drivers/s390/scsi/zfcp_qdio.h b/drivers/s390/scsi/zfcp_qdio.h
> index 8f7d2ae94441a..9f89f2975f6ff 100644
> --- a/drivers/s390/scsi/zfcp_qdio.h
> +++ b/drivers/s390/scsi/zfcp_qdio.h
> @@ -106,16 +107,19 @@ zfcp_qdio_sbale_curr(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req)
>   * @q_req: the qdio request to start
>   * @req_id: The request id
>   * @sbtype: type flags to set for all sbals
> - * @data: First data block
> - * @len: Length of first data block
> + * @has_qtcb: FSF request has a QTCB
> + * @qtcb_len: Length of first data block QTCB, if any
> + * @qtcb_dma_handle: DMA handle of the QTCB, if any
>   *
>   * This is the start of putting the request into the queue, the last
>   * step is passing the request to zfcp_qdio_send. The request queue
>   * lock must be held during the whole process from init to send.
>   */
>  static inline
> -void zfcp_qdio_req_init(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req,
> -			u64 req_id, u8 sbtype, void *data, u32 len)
> +void zfcp_qdio_req_init(struct zfcp_qdio *qdio,
> +			struct zfcp_qdio_req *q_req, u64 req_id,
> +			u8 sbtype, bool has_qtcb, u32 qtcb_len,
> +			dma_addr_t qtcb_dma_handle)
>  {
>  	struct qdio_buffer_element *sbale;
>  	int count = min(atomic_read(&qdio->req_q_free),
> @@ -133,33 +137,34 @@ void zfcp_qdio_req_init(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req,
>  	sbale->eflags = 0;
>  	sbale->sflags = SBAL_SFLAGS0_COMMAND | sbtype;
>  
> -	if (unlikely(!data))
> +	if (unlikely(!has_qtcb))
>  		return;
>  	sbale++;
> -	sbale->addr = virt_to_dma64(data);
> -	sbale->length = len;
> +	sbale->addr = u64_to_dma64(qtcb_dma_handle);
> +	sbale->length = qtcb_len;
>  }

[Severity: High]
If a payload DMA mapping fails after zfcp_fsf_req_create() has already
initialized the SBAL in qdio->req_q, doesn't this leave a stale, unmapped
QTCB DMA address in element[1] of qdio->req_q[qdio->req_q_idx]?

In zfcp_fsf_req_create(), zfcp_qdio_req_init() writes req_id and sflags to
element[0] and writes qtcb_dma_handle and qtcb_len to element[1] of the SBAL
at qdio->req_q_idx.

If zfcp_fsf_req_map_sg() then fails in zfcp_qdio_sbals_from_sg() or
zfcp_fsf_setup_ct_els_unchained(), it returns -EIO without zeroing the SBAL:
- In zfcp_fsf_fcp_cmnd() (drivers/s390/scsi/zfcp_fsf.c), if mapping
  scsi_cmnd->sdb.table fails on a command without DIF/DIX protection data (or
  if mapping scsi_cmnd->prot_sdb->table fails, or zfcp_fsf_set_data_dir()
  fails), failed_normal_scsi_map and failed_scsi_cmnd unmap and free
  req->qtcb without calling zfcp_qdio_zero_sbals().
- In zfcp_fsf_setup_ct_els_sbals() (drivers/s390/scsi/zfcp_fsf.c), if
  mapping sgt_req fails or zfcp_fsf_setup_ct_els_unchained() fails, it
  returns -EIO without calling zfcp_qdio_zero_sbals().

Because qdio->req_q_idx is only advanced in zfcp_qdio_send(), the next
request reuses the same SBAL slot. If that next request is
zfcp_fsf_status_read(), zfcp_qdio_req_init() is called with has_qtcb = false,
which returns early while leaving sbale_curr = 1 and leaving the freed QTCB's
DMA address and length in element[1], and zfcp_qdio_fill_next() then writes
sr_buf into element[2].

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index ea4b968352f4d..091be339d309c 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -184,6 +184,20 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  		zfcp_dbf_scsi_abort("abrt_or", scpnt, NULL);
>  		return FAILED; /* completion could be in progress */
>  	}
> +
> +	/*
> +	 * Unmap SCSI payload DMA mappings before clearing req->data.
> +	 * Once req->data is NULL the hardware completion handler cannot
> +	 * reach the sg_tables, causing a permanent IOVA leak.
> +	 */
> +	if ((scsi_get_prot_op(scpnt) != SCSI_PROT_NORMAL) &&
> +	    scsi_prot_sg_count(scpnt))
> +		zfcp_fsf_req_unmap_sg(old_req, &scpnt->prot_sdb->table,
> +				      scpnt->sc_data_direction);
> +
> +	zfcp_fsf_req_unmap_sg(old_req, &scpnt->sdb.table,
> +			      scpnt->sc_data_direction);
> +
>  	old_req->data = NULL;
>  
>  	/* don't access old fsf_req after releasing the abort_lock */

[Severity: Critical]
Is it safe to unmap scpnt->prot_sdb->table and scpnt->sdb.table in
zfcp_scsi_eh_abort_handler() before the FCP adapter has actually aborted
old_req?

Here in drivers/s390/scsi/zfcp_scsi.c, zfcp_scsi_eh_abort_handler() unmaps
the SCSI command's DMA buffers and clears old_req->data before calling
zfcp_fsf_abort_fcp_cmnd(scpnt) and waiting on abrt_req->completion:

	zfcp_fsf_req_unmap_sg(old_req, &scpnt->sdb.table,
			      scpnt->sc_data_direction);

	old_req->data = NULL;

	/* don't access old fsf_req after releasing the abort_lock */
	write_unlock_irqrestore(&adapter->abort_lock, flags);

	while (retry--) {
		abrt_req = zfcp_fsf_abort_fcp_cmnd(scpnt);

Until the adapter processes the abort command (or if zfcp_fsf_abort_fcp_cmnd()
cannot be sent, fc_block_scsi_eh() returns early, or the abort fails and
returns FAILED), old_req is still active on the FCP channel and may continue
DMA transfers into or out of the unmapped buffers and released SWIOTLB bounce
pages.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021083351.1626409-1-nikh1092@linux.ibm.com?part=2