On Thu, Sep 30, 2021 at 04:44:07PM +0800, Ming Lei wrote:

On Thu, Sep 30, 2021 at 10:29:24AM +0200, Greg Kroah-Hartman wrote:

quoted

On Thu, Sep 30, 2021 at 04:20:11PM +0800, Ming Lei wrote:

quoted

On Thu, Sep 30, 2021 at 10:07:44AM +0200, Greg Kroah-Hartman wrote:

quoted

On Thu, Sep 30, 2021 at 03:40:26PM +0800, Ming Lei wrote:

quoted

SCSI host release is triggered when SCSI device is freed, and we have to
make sure that LLD module won't be unloaded before SCSI host instance is
released because shost->hostt is required in host release handler.

So put LLD module refcnt after SCSI device is released.

The real release handler can be run from wq context in case of
in_interrupt(), so add one atomic counter for serializing putting
module via current and wq context. This way is fine since we don't
call scsi_device_put() in fast IO path.

Reported-by: Changhui Zhong <redacted>
Reported-by: Yi Zhang <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ming Lei <redacted>
---
 drivers/scsi/scsi.c        |  8 +++++++-
 drivers/scsi/scsi_sysfs.c  | 10 ++++++++++
 include/scsi/scsi_device.h |  2 ++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b241f9e3885c..b6612161587f 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c

@@ -553,8 +553,14 @@ EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	module_put(sdev->host->hostt->module);
+	struct module *mod = sdev->host->hostt->module;
+
+	atomic_inc(&sdev->put_dev_cnt);

Ick, no!  Why are you making a new lock and reference count for no
reason?

The reason is to make sure that the LLD module is only put from either
scsi_device_put() and scsi_device_dev_release_usercontext().

quoted

quoted

+
 	put_device(&sdev->sdev_gendev);
+
+	if (atomic_dec_if_positive(&sdev->put_dev_cnt) >= 0)
+		module_put(mod);

How do you know if your module pointer is still valid here?

module refcnt is grabbed in scsi_device_get(), so it is valid.

Then you don't need the extra atomic variable.

quoted

quoted

Why do you care?

What problem are you trying to solve and why is it unique to scsi
devices?

See it from the commit log:

	SCSI host release is triggered when SCSI device is freed, and we have to
	make sure that LLD module won't be unloaded before SCSI host instance is
	released because shost->hostt is required in host release handler.

What is "hostt"?

hostt is 'struct scsi_host_template' which is defined in LLD module, and
often allocated as static global variable, that is what try_get_module()
tries to protect.

quoted

quoted

	
	So put LLD module refcnt after SCSI device is released.

Why not just drop it explicitly when you drop the reference count of the
device object?  Like you tried to do here, but no need for the extra
atomic variable.

scsi_device_dev_release_usercontext() may be scheduled via schedule_work from
the device object's release handler for releasing the scsi_device, which may
trigger scsi host's release handler in which hostt is required.

If a release handler can be called from the device release function,
then that is when you need to drop the reference, after that function is
finished being called, right?

thanks,

greg k-h