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 3 potential issue(s) to consider:
- [Critical] scsi: zfcp: premature DMA unmap in zfcp_scsi_eh_abort_handler()
- [High] scsi: zfcp: double DMA unmap of scsi_cmnd in zfcp_scsi_eh_abort_handler()
- [High] scsi: zfcp: type confusion on TMF requests in zfcp_scsi_forget_cmnd()
--

commit 9c1fb6b4aad79a57f42f4a774fc0a9866fc4009c
Author: M Nikhil <nikh1092@linux.ibm.com>

zfcp: Use DMA streaming API to map I/O

This commit converts zFCP I/O buffer mappings for QTCB, SRB, CT/ELS, and
SCSI I/O requests to use the DMA streaming API via dma_map_sgtable() and
dma_unmap_sgtable(). It also updates the SCSI abort and reset paths to
unmap the scatter-gather lists associated with aborted or forgotten SCSI
commands.

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index ea4b968352f4d..99baca26c9bf0 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -184,6 +184,15 @@ 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 */
>  	}
> +
> +	/*
> +	 * Suppress the normal completion handler's unmap by clearing
> +	 * req->data now, under the abort_lock write-side.  The actual
> +	 * DMA unmap is deferred until the hardware has stopped using
> +	 * the buffers (after wait_for_completion or after ERP has
> +	 * quiesced the adapter), so that we never call dma_unmap_sgtable
> +	 * while the FCP channel may still be writing into the pages.
> +	 */
>  	old_req->data = NULL;

[Severity: High]
Can this unmap scpnt->sdb.table and scpnt->prot_sdb->table a second time if
zfcp_scsi_forget_cmnd() has already run for old_req?

When a LUN or target reset TMF completes in zfcp_scsi_task_mgmt_function(),
for instance via an SG_SCSI_RESET ioctl in scsi_ioctl_reset(), it calls
zfcp_scsi_forget_cmnds() -> zfcp_scsi_forget_cmnd() under
adapter->abort_lock. That function unmaps scpnt->prot_sdb->table and
scpnt->sdb.table and sets old_req->data = NULL, while leaving old_req in
adapter->req_list, scpnt->host_scribble set to old_req->req_id, and
sgt->nents non-zero.

Because old_req->data is now NULL, zfcp_fsf_fcp_cmnd_handler() will not
call scsi_done(scpnt). If scpnt subsequently times out and enters
zfcp_scsi_eh_abort_handler(scpnt) while old_req is still in
adapter->req_list, zfcp_reqlist_find() still finds old_req:

drivers/s390/scsi/zfcp_scsi.c:zfcp_scsi_eh_abort_handler()
	write_lock_irqsave(&adapter->abort_lock, flags);

	old_req = zfcp_reqlist_find(adapter->req_list, old_reqid);
	if (!old_req) {
		write_unlock_irqrestore(&adapter->abort_lock, flags);
		zfcp_dbf_scsi_abort("abrt_or", scpnt, NULL);
		return FAILED; /* completion could be in progress */
	}
	...
	old_req->data = NULL;

Unlike zfcp_fsf_fcp_cmnd_handler() and zfcp_scsi_forget_cmnd(),
zfcp_scsi_eh_abort_handler() does not check whether old_req->data is
already NULL before proceeding to out_unmap:, where zfcp_fsf_req_unmap_sg()
is called on scpnt->prot_sdb->table and scpnt->sdb.table again.

Should zfcp_scsi_eh_abort_handler() check if (!old_req || !old_req->data)
under adapter->abort_lock before taking ownership of the unmap?

>  
>  	/* don't access old fsf_req after releasing the abort_lock */
> @@ -198,18 +207,34 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  		zfcp_erp_wait(adapter);
>  		ret = fc_block_scsi_eh(scpnt);
>  		if (ret) {
> +			/*
> +			 * fc_block_scsi_eh returned FAST_IO_FAIL: the
> +			 * fast_io_fail_tmo fired.  ERP has already called
> +			 * zfcp_fsf_req_dismiss_all() (which requires QDIO
> +			 * to be down first), so the hardware is no longer
> +			 * using the buffers.  Pass FAST_IO_FAIL back to
> +			 * scsi_eh, but unmap first.
> +			 */
>  			zfcp_dbf_scsi_abort("abrt_bl", scpnt, NULL);
> -			return ret;
> +			retval = ret;
> +			goto out_unmap;
>  		}
>  		if (!(atomic_read(&adapter->status) &
>  		      ZFCP_STATUS_COMMON_RUNNING)) {
> +			/*
> +			 * Adapter is gone; ERP shut down QDIO and dismissed
> +			 * all requests before clearing RUNNING.  Safe to
> +			 * unmap now.
> +			 */
>  			zfcp_dbf_scsi_abort("abrt_ru", scpnt, NULL);
> -			return SUCCESS;
> +			retval = SUCCESS;
> +			goto out_unmap;
>  		}
>  	}
>  	if (!abrt_req) {
>  		zfcp_dbf_scsi_abort("abrt_ar", scpnt, NULL);
> -		return FAILED;
> +		retval = FAILED;
> +		goto out_unmap;
>  	}
>  
>  	wait_for_completion(&abrt_req->completion);
> @@ -224,6 +249,28 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
>  	}
>  	zfcp_dbf_scsi_abort(dbf_tag, scpnt, abrt_req);
>  	zfcp_fsf_req_free(abrt_req);
> +
> +out_unmap:
> +	/*
> +	 * The hardware has stopped using the SCSI command's DMA buffers:
> +	 * either the adapter confirmed the abort, ERP quiesced the adapter
> +	 * (zfcp_fsf_req_dismiss_all requires QDIO down), or fc_block_scsi_eh
> +	 * signalled link-level quiesce.  Unmap here, directly from scpnt,
> +	 * because old_req must not be touched after the abort_lock was
> +	 * released.  The normal completion handler (zfcp_fsf_fcp_cmnd_handler)
> +	 * skips its own unmap when it sees req->data == NULL, so there is no
> +	 * double-unmap.
> +	 */
> +	if ((scsi_get_prot_op(scpnt) != SCSI_PROT_NORMAL) &&
> +	    scsi_prot_sg_count(scpnt))
> +		zfcp_fsf_req_unmap_sg(&adapter->ccw_device->dev,
> +				      &scpnt->prot_sdb->table,
> +				      scpnt->sc_data_direction);
> +
> +	zfcp_fsf_req_unmap_sg(&adapter->ccw_device->dev,
> +			      &scpnt->sdb.table,
> +			      scpnt->sc_data_direction);
> +
>  	return retval;
>  }

[Severity: Critical]
Could jumping to out_unmap: on the failure and FAST_IO_FAIL paths in
zfcp_scsi_eh_abort_handler() unmap scpnt->sdb.table and
scpnt->prot_sdb->table while the FCP channel is still accessing those
buffers?

There appear to be three paths reaching out_unmap: where the hardware has
not yet stopped using scpnt's DMA buffers:

1. When all retries of zfcp_fsf_abort_fcp_cmnd(scpnt) return NULL (!abrt_req,
dbf_tag = "abrt_ar", retval = FAILED) while adapter->status still has
ZFCP_STATUS_COMMON_RUNNING and fc_block_scsi_eh(scpnt) returns 0 (for
example, if zfcp_qdio_sbal_get() or zfcp_fsf_req_create() fails, or if
zfcp_sdev->status lacks ZFCP_STATUS_COMMON_UNBLOCKED during LUN-level ERP
while the rport remains ONLINE). No abort command was sent, QDIO remains
active, and old_req is still in flight on the channel.

2. When abrt_req completes without ZFCP_STATUS_FSFREQ_ABORTSUCCEEDED or
ZFCP_STATUS_FSFREQ_ABORTNOTNEEDED (dbf_tag = "abrt_fa", retval = FAILED),
such as on FSF_ADAPTER_STATUS_AVAILABLE, FSF_LUN_BOXED, FSF_PORT_BOXED,
FSF_LUN_HANDLE_NOT_VALID, FSF_PORT_HANDLE_NOT_VALID, or when abrt_req times
out in zfcp_fsf_request_timeout_handler(). The adapter has not confirmed
that old_req was terminated, and any ERP recovery is only scheduled
asynchronously without waiting before out_unmap: runs.

3. When fc_block_scsi_eh(scpnt) returns FAST_IO_FAIL (dbf_tag = "abrt_bl").
fc_block_scsi_eh() waits on the rport's FC_PORTSTATE_BLOCKED state, which
occurs during port-level ERP or link-down handling without shutting down
QDIO or calling zfcp_fsf_req_dismiss_all(). In addition,
fc_timeout_fail_rport_io() sets FC_RPORT_FAST_FAIL_TIMEDOUT before calling
fc_terminate_rport_io():

drivers/scsi/scsi_transport_fc.c:fc_timeout_fail_rport_io()
	if (rport->port_state != FC_PORTSTATE_BLOCKED)
		return;

	rport->flags |= FC_RPORT_FAST_FAIL_TIMEDOUT;
	fc_terminate_rport_io(rport);

And zfcp_scsi_terminate_rport_io() only queues zfcp_erp_port_forced_reopen()
asynchronously without waiting for the port reopen to complete.

Furthermore, because old_req->data was already cleared to NULL at the start
of zfcp_scsi_eh_abort_handler(), neither zfcp_scsi_forget_cmnd() (during
subsequent SCSI EH LUN/target reset escalation) nor
zfcp_fsf_fcp_cmnd_handler() (during adapter reopen /
zfcp_fsf_req_dismiss_all()) will unmap the buffers once the hardware
actually quiesces.

Should old_req->data remain set and the DMA unmap be skipped when
zfcp_scsi_eh_abort_handler() does not confirm that old_req was aborted or
that the adapter was quiesced, leaving the unmap to the subsequent reset or
request dismissal path?

>  
> @@ -237,6 +284,7 @@ static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
>  {
>  	struct zfcp_scsi_req_filter *filter =
>  		(struct zfcp_scsi_req_filter *)data;
> +	struct scsi_cmnd *scpnt;
>  
>  	/* already aborted - prevent side-effects - or not a SCSI command */
>  	if (old_req->data == NULL ||
> @@ -244,6 +292,8 @@ static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
>  	    old_req->qtcb->header.fsf_command != FSF_QTCB_FCP_CMND)
>  		return;
>  
> +	scpnt = old_req->data;
> +
>  	/* (tmf_scope == FCP_TMF_TGT_RESET || tmf_scope == FCP_TMF_LUN_RESET) */
>  	if (old_req->qtcb->header.port_handle != filter->port_handle)
>  		return;
> @@ -253,6 +303,17 @@ static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
>  		return;
>  
>  	zfcp_dbf_scsi_nullcmnd((struct scsi_cmnd *)old_req->data, old_req);
> +
> +	if ((scsi_get_prot_op(scpnt) != SCSI_PROT_NORMAL) &&
> +	    scsi_prot_sg_count(scpnt))
> +		zfcp_fsf_req_unmap_sg(&old_req->adapter->ccw_device->dev,
> +				      &scpnt->prot_sdb->table,
> +				      scpnt->sc_data_direction);
> +
> +	zfcp_fsf_req_unmap_sg(&old_req->adapter->ccw_device->dev,
> +			      &scpnt->sdb.table,
> +			      scpnt->sc_data_direction);
> +
>  	old_req->data = NULL;
>  }

[Severity: High]
What happens in zfcp_scsi_forget_cmnd() if another FCP task management
request is in flight in adapter->req_list for the same port or LUN?

In zfcp_fsf_fcp_task_mgmt(), task management requests are also created with
fsf_command == FSF_QTCB_FCP_CMND, but req->data holds a struct scsi_device *
rather than a struct scsi_cmnd *:

drivers/s390/scsi/zfcp_fsf.c:zfcp_fsf_fcp_task_mgmt()
	req = zfcp_fsf_req_create(qdio, FSF_QTCB_FCP_CMND,
				  SBAL_SFLAGS0_TYPE_WRITE,
				  qdio->adapter->pool.scsi_req);
	...
	req->data = sdev;
	req->handler = zfcp_fsf_fcp_task_mgmt_handler;
	req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
	req->qtcb->header.port_handle = zfcp_sdev->port->handle;
	req->qtcb->bottom.io.data_direction = FSF_DATADIR_CMND;

If two TMF requests are in flight concurrently (for example, an
SG_SCSI_RESET ioctl in scsi_ioctl_reset() racing with scsi_error_handler()
or another SG_SCSI_RESET ioctl), zfcp_scsi_forget_cmnds() will invoke
zfcp_scsi_forget_cmnd() on the other in-flight TMF request, cast its
struct scsi_device * in old_req->data to struct scsi_cmnd *, pass fields of
struct scsi_device to zfcp_fsf_req_unmap_sg(), and clear old_req->data to
NULL (which zfcp_fsf_fcp_task_mgmt_handler() later dereferences).

Should zfcp_scsi_forget_cmnd() also check that old_req is not a TMF request
(for example, by checking old_req->qtcb->bottom.io.data_direction !=
FSF_DATADIR_CMND or old_req->status & ZFCP_STATUS_FSFREQ_CLEANUP)?

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