Re: [PATCH 03/24] zfcp: open-code fc_block_scsi_eh() for host reset

Steffen Maier
2022-05-03T17:21:42Z

Hi Hannes,

On 5/2/22 23:37, Hannes Reinecke wrote:
> When issuing a host reset we should be waiting for all
> ports to become unblocked; just waiting for one might
> be resulting in host reset to return too early.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> Cc: Steffen Maier <maier@linux.ibm.com>
> Cc: Benjamin Block <bblock@linux.ibm.com>
> ---
> drivers/s390/scsi/zfcp_scsi.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index 526ac240d9fe..fb2b73fca381 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -373,9 +373,11 @@ static int zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)
>
> static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
> {
> - struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
> - struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
> - int ret = SUCCESS, fc_ret;
> + struct Scsi_Host *host = scpnt->device->host;
> + struct zfcp_adapter *adapter = (struct zfcp_adapter *)host->hostdata[0];
> + int ret = SUCCESS;
> + unsigned long flags;
> + struct zfcp_port *port;
>
> if (!(adapter->connection_features & FSF_FEATURE_NPIV_MODE)) {
> zfcp_erp_port_forced_reopen_all(adapter, 0, "schrh_p");
> @@ -383,9 +385,24 @@ static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
> }
> zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
> zfcp_erp_wait(adapter);
> - fc_ret = fc_block_scsi_eh(scpnt);
> - if (fc_ret)
> - ret = fc_ret;
> +retry_rport_blocked:
> + spin_lock_irqsave(host->host_lock, flags);
> + list_for_each_entry(port, &adapter->port_list, list) {

Reading adapter->port_list needs to be protected by
read_lock_irq(&adapter->port_list_lock);

Cf. Benjamin's last review
https://lore.kernel.org/linux-scsi/YRujHScPbb1Aokrj@t480-pf1aa2c2.linux.ibm.com/

> + struct fc_rport *rport = port->rport;

While an rport is blocked, port->rport == NULL [zfcp_scsi_rport_block() and
zfcp_scsi_rport_register()], so below could be a NULL pointer deref.
Or is there evidence we would never have a blocked rport while iterating
port_list here?

See also my previous review comments
https://lore.kernel.org/linux-scsi/a7950ea7-380c-bb01-7f31-5c555217ad2d@linux.vnet.ibm.com/
It also alludes to lock ordering.

> +
> + if (rport->port_state == FC_PORTSTATE_BLOCKED) {
> + if (rport->flags & FC_RPORT_FAST_FAIL_TIMEDOUT)
> + ret = FAST_IO_FAIL;
> + else
> + ret = NEEDS_RETRY;
> + break;
> + }
> + }
> + spin_unlock_irqrestore(host->host_lock, flags);
> + if (ret == NEEDS_RETRY) {
> + msleep(1000);
> + goto retry_rport_blocked;
> + }
>
> zfcp_dbf_scsi_eh("schrh_r", adapter, ~0, ret);
> return ret;

--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z and LinuxONE

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: David Faller
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294