Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

On 9/27/22 18:09, Ming Lei wrote:
> On Wed, Sep 14, 2022 at 03:56:20PM -0700, Bart Van Assche wrote:
> > Some kernel modules call device_del() from their module exit code and
> > schedule asynchronous work from inside the .release callback without waiting
> > until that callback has finished. As an example, many SCSI LLD drivers call
>
> It isn't only related with device, any kobject has such issue, or any
> reference counter usage has similar potential risk, see previous discussion:
>
> https://lore.kernel.org/lkml/YsZm7lSXYAHT14ui@T590/
>
> IMO, it is one fundamental problem wrt. module vs. reference counting or
> kobject uses at least, since the callback depends on module code
> segment.
>
> > scsi_remove_host() from their module exit code. scsi_remove_host() may
> > invoke scsi_device_dev_release_usercontext() asynchronously.
> > scsi_device_dev_release_usercontext() uses the host template pointer and
> > that pointer usually exists in static storage in the SCSI LLD. Support
> > using the module reference count to keep the module around until
> > asynchronous module exiting has completed by waiting in the delete_module()
> > system call until the module reference count drops to zero.
>
> The issue can't be addressed by the normal mod->refcnt, since user need
> to unload module when the device isn't used.

Hi Ming,

How about removing support for calling scsi_device_put() from atomic context
as is done in the untested patch below?

Thanks,

Bart.

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index c59eac7a32f2..661753a10b47 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -561,6 +561,8 @@ EXPORT_SYMBOL(scsi_report_opcode);
  */
 int scsi_device_get(struct scsi_device *sdev)
 {
+	might_sleep();
+
 	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
 		goto fail;
 	if (!get_device(&sdev->sdev_gendev))
@@ -588,6 +590,7 @@ void scsi_device_put(struct scsi_device *sdev)
 {
 	struct module *mod = sdev->host->hostt->module;

+	might_sleep();
 	put_device(&sdev->sdev_gendev);
 	module_put(mod);
 }
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index a3aaafdeac1d..4cfc9317b4ad 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -441,7 +441,7 @@ static void scsi_device_cls_release(struct device *class_dev)
 	put_device(&sdev->sdev_gendev);
 }

-static void scsi_device_dev_release_usercontext(struct work_struct *work)
+static void scsi_device_dev_release(struct device *dev)
 {
 	struct scsi_device *sdev;
 	struct device *parent;
@@ -450,11 +450,8 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	struct scsi_vpd *vpd_pg0 = NULL, *vpd_pg89 = NULL;
 	struct scsi_vpd *vpd_pgb0 = NULL, *vpd_pgb1 = NULL, *vpd_pgb2 = NULL;
 	unsigned long flags;
-	struct module *mod;
-
-	sdev = container_of(work, struct scsi_device, ew.work);

-	mod = sdev->host->hostt->module;
+	sdev = to_scsi_device(dev);

 	parent = sdev->sdev_gendev.parent;

@@ -516,19 +513,6 @@ 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);
 }

 static struct class sdev_class = {