On 7/7/22 1:21 PM, Bart Van Assche wrote:
> From: Ming Lei <ming.lei@xxxxxxxxxx>
>
> Fix the race conditions between SCSI LLD kernel module unloading and SCSI
> device and target removal by making sure that SCSI hosts are destroyed after
> all associated target and device objects have been freed.
>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxx>
> Cc: John Garry <john.garry@xxxxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> [ bvanassche: Reworked Ming's patch ]
> ---
> drivers/scsi/hosts.c | 9 +++++++++
> drivers/scsi/scsi.c | 9 ++++++---
> drivers/scsi/scsi_scan.c | 7 +++++++
> drivers/scsi/scsi_sysfs.c | 9 ---------
> include/scsi/scsi_host.h | 3 +++
> 5 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index ef6c0e37acce..e0a56a8f1f74 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -168,6 +168,7 @@ void scsi_remove_host(struct Scsi_Host *shost)
>
> mutex_lock(&shost->scan_mutex);
> spin_lock_irqsave(shost->host_lock, flags);
> + /* Prevent that new SCSI targets or SCSI devices are added. */
> if (scsi_host_set_state(shost, SHOST_CANCEL))
> if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) {
> spin_unlock_irqrestore(shost->host_lock, flags);
> @@ -190,6 +191,13 @@ void scsi_remove_host(struct Scsi_Host *shost)
> transport_unregister_device(&shost->shost_gendev);
> device_unregister(&shost->shost_dev);
> device_del(&shost->shost_gendev);
> +
> + /*
> + * After scsi_remove_host() has returned the scsi LLD module can be
> + * unloaded and/or the host resources can be released. Hence wait until
> + * the dependent SCSI targets and devices are gone before returning.
> + */
> + wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0);
Do we still have a possible use after free at the target removal level?
If you have a driver supports multiple targets and target removal (any of
he FC ones, HW iscsi, etc), then you can still hit:
1. thread1 does sysfs device delete. It's now waiting in blk_cleanup_queue
which is waiting on a cmd that has the SCSI error handler running on it or
for whatever reason.
2. thread2 decides to delete the target (dev loss tmo or user request). That
hits __scsi_remove_device for the device in #1 and sees it's in SDEV_DEL
state so it returns.
3. scsi_remove_target returns. The transport/driver then frees it's rport/session
for that target.
4. The thread1 in then makes progress in the EH callback and wants to reference
the rport/session struct we deleted in #3.
The drivers want to know that after scsi_remove_target has returned that nothing
is using devices under it similar to the scsi_remove_host case.
Every scsi_device has a scsi_target as a parent (scsi_device -> scsi_target) or
grandparent (scsi_device -> transport class struct like rport/session ->
scsi_target) now right. I was thinking maybe like 20 years ago when scsi_forget_host
was made we didn't?
If so, could we move what are doing in this patch down a level? Put the wait in
scsi_remove_target and wake in scsi_target_dev_release. Instead of a target_count
you have a scsi_target sdev_count.
scsi_forget_host would then need to loop over the targets and do
scsi_target_remove on them instead of doing it at the scsi_device level.
