On Thu, Jun 30, 2022 at 02:37:32PM -0700, Bart Van Assche wrote:
> Prepare for freeing the host tag set earlier by making scsi_forget_host()
> wait until all activity on the host tag set has stopped.
I think it is correct to free the host tag during removing host.
>
> 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: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/scsi/scsi_scan.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 5c3bb4ceeac3..c8331ccdde95 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1961,6 +1961,16 @@ void scsi_scan_host(struct Scsi_Host *shost)
> }
> EXPORT_SYMBOL(scsi_scan_host);
>
> +/**
> + * scsi_forget_host() - Remove all SCSI devices from a host.
> + * @shost: SCSI host to remove devices from.
> + *
> + * Removes all SCSI devices that have not yet been removed. For the SCSI devices
> + * for which removal started before scsi_forget_host(), wait until the
> + * associated request queue has reached the "dead" state. In that state it is
> + * guaranteed that no new requests will be allocated and also that no requests
> + * are in progress anymore.
> + */
> void scsi_forget_host(struct Scsi_Host *shost)
> {
> struct scsi_device *sdev;
> @@ -1970,8 +1980,21 @@ void scsi_forget_host(struct Scsi_Host *shost)
> restart:
> spin_lock_irq(shost->host_lock);
> list_for_each_entry(sdev, &shost->__devices, siblings) {
> - if (sdev->sdev_state == SDEV_DEL)
> + if (sdev->sdev_state == SDEV_DEL &&
> + blk_queue_dead(sdev->request_queue)) {
> continue;
> + }
> + if (sdev->sdev_state == SDEV_DEL) {
> + get_device(&sdev->sdev_gendev);
> + spin_unlock_irq(shost->host_lock);
> +
> + while (!blk_queue_dead(sdev->request_queue))
> + msleep(10);
Thinking of further, this report on UAF on SRP resource and Changhui's
report on UAF of host->hostt should belong to same kind of issue:
1) after scsi_remove_host() returns, either the HBA driver module can be
unloaded and hostt can't be used, or any HBA resources can be freed, both
may cause UAF in scsi_mq_exit_request, so moving freeing host tagset to
scsi_remove_host() is correct.
static void scsi_mq_exit_request()
{
...
if (shost->hostt->exit_cmd_priv)
shost->hostt->exit_cmd_priv(shost, cmd);
}
2) checking request queue dead here isn't good, which not only relies
on block layer implementation detail, but also can't avoid UAF on ->hostt,
I'd suggest to fix them all. The attached patch may drain all sdevs, which
can replace the 2nd patch if you don't mind, but the 3rd patch is still needed:
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index e1cb187402fd..6445718c2b18 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -189,6 +189,14 @@ void scsi_remove_host(struct Scsi_Host *shost)
transport_unregister_device(&shost->shost_gendev);
device_unregister(&shost->shost_dev);
device_del(&shost->shost_gendev);
+
+ /*
+ * Once returning, the scsi LLD module can be unloaded, so we have
+ * to wait until our descendants are released, otherwise our host
+ * device reference can be grabbed by them, then use-after-free
+ * on shost->hostt will be triggered
+ */
+ wait_for_completion(&shost->targets_completion);
}
EXPORT_SYMBOL(scsi_remove_host);
@@ -393,6 +401,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
INIT_LIST_HEAD(&shost->starved_list);
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
+ init_completion(&shost->targets_completion);
index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
if (index < 0) {
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 10e5bffc34aa..b6e6354da396 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -545,10 +545,8 @@ EXPORT_SYMBOL(scsi_device_get);
*/
void scsi_device_put(struct scsi_device *sdev)
{
- struct module *mod = sdev->host->hostt->module;
-
+ module_put(sdev->host->hostt->module);
put_device(&sdev->sdev_gendev);
- module_put(mod);
}
EXPORT_SYMBOL(scsi_device_put);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 2ef78083f1ef..931333a48372 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -406,9 +406,14 @@ static void scsi_target_destroy(struct scsi_target *starget)
static void scsi_target_dev_release(struct device *dev)
{
struct device *parent = dev->parent;
+ struct Scsi_Host *shost = dev_to_shost(parent);
struct scsi_target *starget = to_scsi_target(dev);
kfree(starget);
+
+ if (!atomic_dec_return(&shost->nr_targets))
+ complete_all(&shost->targets_completion);
+
put_device(parent);
}
@@ -521,6 +526,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
starget->state = STARGET_CREATED;
starget->scsi_level = SCSI_2;
starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+ atomic_inc(&shost->nr_targets);
retry:
spin_lock_irqsave(shost->host_lock, flags);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 94091d5281ba..28c51ef0ea54 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -449,12 +449,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL;
struct scsi_vpd *vpd_pg0 = NULL, *vpd_pg89 = NULL;
unsigned long flags;
- struct module *mod;
sdev = container_of(work, struct scsi_device, ew.work);
- mod = sdev->host->hostt->module;
-
scsi_dh_release_device(sdev);
parent = sdev->sdev_gendev.parent;
@@ -505,17 +502,11 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
if (parent)
put_device(parent);
- module_put(mod);
}
static void scsi_device_dev_release(struct device *dev)
{
struct scsi_device *sdp = to_scsi_device(dev);
-
- /* Set module pointer as NULL in case of module unloading */
- if (!try_module_get(sdp->host->hostt->module))
- sdp->host->hostt->module = NULL;
-
execute_in_process_context(scsi_device_dev_release_usercontext,
&sdp->ew);
}
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 37718373defc..d0baffd62287 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -710,6 +710,9 @@ struct Scsi_Host {
/* ldm bits */
struct device shost_gendev, shost_dev;
+ atomic_t nr_targets;
+ struct completion targets_completion;
+
/*
* Points to the transport data (if any) which is allocated
* separately
Thanks,
Ming
