Re: [PATCH 2/2] scsi: scsi_error: Fix device reset is not triggered

[Wenchao Hao]

On 2023/9/22 23:18, Bart Van Assche wrote:
> On 9/22/23 02:36, Wenchao Hao wrote:
> > Fix the issue of skipping scsi_try_bus_device_reset() for devices
> > which is in progress of removing in following order:
> >
> > T1:                    T2:scsi_error_handle
> > __scsi_remove_device
> >    scsi_device_set_state(sdev, SDEV_DEL)
> >                     // would skip device with SDEV_DEL state
> >                        shost_for_each_device()
> >                       scsi_try_bus_device_reset
> >                     flush all commands
> >   ...
> >   scsi_device is released
> >
> > Some drivers like smartpqi only implement eh_device_reset_handler,
> > if device reset is skipped, the commands which had been sent to
> > firmware or devices hardware are not cleared. The error handle
> > would flush all these commands in scsi_unjam_host().
> >
> > When the commands are finished by hardware, use after free issue is
> > triggered.
> >
> > Add parameter "check_state" to macro shost_for_each_device() to
> > determine if check device status when traversal scsi_device
> > of Scsi_Host, and set this parameter to false when traversal
> > in scsi_error_handle to address this issue.
>
> The above is incomprehensible to me. Please explain more clearly why this change is needed.

This is added for scsi error to not skip scsi_device's which is
in progress of removing, mostly, shost_for_each_device() is called
with check_state to 1, excepting some functions in scsi_error.c

> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index d0911bc28663..db8b9e42267c 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -704,6 +704,23 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
> >       return 0;
> >   }
> > +static int __scsi_device_get(struct scsi_device *sdev, bool check_state)
>
> "check_state" is a bad argument name because it does not clearly explain the purpose of this argument. Would "include_deleted" perhaps be a better name?

I took "include_deleted" in consideration too, but I am not
sure if more state check would be introduced, and we need following
check if name the it to "include_deleted"

if (!include_deleted &&	(sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL))
	goto fail;

Addition "!" is needed. So finally "check_state" is chosen.

If you think "include_deleted" is better, I would change in my next version.

> > +{
> > +    if (check_state &&
> > +        (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL))
> > +        goto fail;
> > +    if (!try_module_get(sdev->host->hostt->module))
> > +        goto fail;
> > +    if (!get_device(&sdev->sdev_gendev))
> > +        goto fail_put_module;
> > +    return 0;
> > +
> > +fail_put_module:
> > +    module_put(sdev->host->hostt->module);
> > +fail:
> > +    return -ENXIO;
> > +}
>
> Looking at the above code, I think we need two functions: one that does not include the sdev->sdev_state check and a second function that includes the sdev->sdev_state check (scsi_device_get()) and calls the first. That will result in code that is easier to read than calls to a function with a boolean argument.

Yes, that's what I did.

> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index c498a12f7715..e166d053c839 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -389,21 +389,25 @@ extern void __starget_for_each_device(struct scsi_target *, void *,
> >   /* only exposed to implement shost_for_each_device */
> >   extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
> > -                          struct scsi_device *);
> > +                          struct scsi_device *,
> > +                          bool);
> >   /**
> >    * shost_for_each_device - iterate over all devices of a host
> >    * @sdev: the &struct scsi_device to use as a cursor
> >    * @shost: the &struct scsi_host to iterate over
> > + * @check_state: if skip check scsi_device's state to skip some devices
> > + *               scsi_device with SDEV_DEL or SDEV_CANCEL would be skipped
> > + *               if this is true
> >    *
> >    * Iterator that returns each device attached to @shost.  This loop
> >    * takes a reference on each device and releases it at the end.  If
> >    * you break out of the loop, you must call scsi_device_put(sdev).
> >    */
> > -#define shost_for_each_device(sdev, shost) \
> > -    for ((sdev) = __scsi_iterate_devices((shost), NULL); \
> > +#define shost_for_each_device(sdev, shost, check_state) \
> > +    for ((sdev) = __scsi_iterate_devices((shost), NULL, check_state); \
> >            (sdev); \
> > -         (sdev) = __scsi_iterate_devices((shost), (sdev)))
> > +         (sdev) = __scsi_iterate_devices((shost), (sdev), check_state))
> >   /**
> >    * __shost_for_each_device - iterate over all devices of a host (UNLOCKED)
>
> Since only the SCSI error handler passes 0 as 'check_state' argument to shost_for_each_device(), instead of adding a boolean argument to that macro, please do the following:
> * Introduce a new macro for the check_state = 1 case.
> * Keep the semantics for shost_for_each_device().
>
> With this approach no SCSI LLDs will have to be modified.

Originally, I added a new macro _shost_for_each_device(), which looks like following:

#define _shost_for_each_device(sdev, shost, check_state)

But I found another __shost_for_each_device(), too much shost_for_each_device(), so
I do not want to add another one any more...

Modify shost_for_each_device() directly looks not so good too, I want to add another
macro named "shost_for_each_device_all_state()" which do not skip scsi_device's with
DEL or CANCEL state, what do you think?

Thanks.

> Thanks,
>
> Bart.