[PATCH 0/2] SCSI: fix race between releasing shost and unloading LLD module

[PATCH 0/2] SCSI: fix race between releasing shost and unloading LLD module

[Ming Lei]

Hello,

Patch 1 allows put_device() to return if the device is really released.

Patch 2 fixes one race between releasing shost and unloading LLD module
by making sure that LLD module refcnt is dropped after the scsi device
is released.

Fix the kernel panic of 'BUG: unable to handle page fault for address'
reported by Changhui and Yi.


Ming Lei (2):
  driver core: tell caller if the device/kboject is really released
  scsi: core: put LLD module refcnt after SCSI device is released

 drivers/base/core.c        |  5 +++--
 drivers/scsi/scsi.c        | 14 ++++++++++++--
 drivers/scsi/scsi_sysfs.c  |  8 ++++++++
 include/linux/device.h     |  2 +-
 include/linux/kobject.h    |  2 +-
 include/scsi/scsi_device.h |  1 +
 lib/kobject.c              |  5 +++--
 7 files changed, 29 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH 1/2] driver core: tell caller if the device/kboject is really released

[Ming Lei]

Return if the device/kobject is really released to caller.

One use case is scsi_device_put() and the scsi device's release handler
runs async work to clean up things. We have to piggyback the module_put()
into the async work for avoiding to touch unmapped module page.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/base/core.c     | 5 +++--
 include/linux/device.h  | 2 +-
 include/linux/kobject.h | 2 +-
 lib/kobject.c           | 5 +++--
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index e65dd803a453..cd1365a934b9 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3459,11 +3459,12 @@ EXPORT_SYMBOL_GPL(get_device);
  * put_device - decrement reference count.
  * @dev: device in question.
  */
-void put_device(struct device *dev)
+int put_device(struct device *dev)
 {
 	/* might_sleep(); */
 	if (dev)
-		kobject_put(&dev->kobj);
+		return kobject_put(&dev->kobj);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(put_device);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index e270cb740b9e..ab089d743667 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -949,7 +949,7 @@ extern int (*platform_notify_remove)(struct device *dev);
  *
  */
 struct device *get_device(struct device *dev);
-void put_device(struct device *dev);
+int put_device(struct device *dev);
 bool kill_device(struct device *dev);
 
 #ifdef CONFIG_DEVTMPFS
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index ea30529fba08..c83cc8a7a170 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -111,7 +111,7 @@ extern int __must_check kobject_move(struct kobject *, struct kobject *);
 extern struct kobject *kobject_get(struct kobject *kobj);
 extern struct kobject * __must_check kobject_get_unless_zero(
 						struct kobject *kobj);
-extern void kobject_put(struct kobject *kobj);
+extern int kobject_put(struct kobject *kobj);
 
 extern const void *kobject_namespace(struct kobject *kobj);
 extern void kobject_get_ownership(struct kobject *kobj,
diff --git a/lib/kobject.c b/lib/kobject.c
index ea53b30cf483..7ebdd6b99064 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -743,15 +743,16 @@ static void kobject_release(struct kref *kref)
  *
  * Decrement the refcount, and if 0, call kobject_cleanup().
  */
-void kobject_put(struct kobject *kobj)
+int kobject_put(struct kobject *kobj)
 {
 	if (kobj) {
 		if (!kobj->state_initialized)
 			WARN(1, KERN_WARNING
 				"kobject: '%s' (%p): is not initialized, yet kobject_put() is being called.\n",
 			     kobject_name(kobj), kobj);
-		kref_put(&kobj->kref, kobject_release);
+		return kref_put(&kobj->kref, kobject_release);
 	}
+	return 0;
 }
 EXPORT_SYMBOL(kobject_put);
 
-- 
2.31.1

[PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released

[Ming Lei]

SCSI host release is triggered when SCSI device is released, and we have to
make sure that LLD module won't be unloaded before SCSI host instance is
released.

So put LLD module refcnt after SCSI device is released.

SCSI device release may be moved into workqueue context if scsi_device_put
is called in interrupt context, and handle this case by piggybacking
putting LLD module refcnt into SCSI device release handler.

Reported-by: Changhui Zhong <czhong@redhat.com>
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi.c        | 14 ++++++++++++--
 drivers/scsi/scsi_sysfs.c  |  8 ++++++++
 include/scsi/scsi_device.h |  1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b241f9e3885c..7cad256ba895 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -553,8 +553,18 @@ EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	module_put(sdev->host->hostt->module);
-	put_device(&sdev->sdev_gendev);
+	struct module *mod = sdev->host->hostt->module;
+	/*
+	 * sdev->sdev_gendev's real release handler will be scheduled into
+	 * user context if we are in interrupt context, and we have to put
+	 * LLD module refcnt after the device is really released.
+	 */
+	preempt_disable();
+	if (put_device(&sdev->sdev_gendev) && in_interrupt())
+		sdev->put_lld_mod_refcnt = 1;
+	else
+		module_put(mod);
+	preempt_enable();
 }
 EXPORT_SYMBOL(scsi_device_put);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 86793259e541..dc056ba5a656 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -449,9 +449,14 @@ 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 *lld_mod;
+	bool put_lld_mod_refcnt;
 
 	sdev = container_of(work, struct scsi_device, ew.work);
 
+	lld_mod = sdev->host->hostt->module;
+	put_lld_mod_refcnt = sdev->put_lld_mod_refcnt;
+
 	scsi_dh_release_device(sdev);
 
 	parent = sdev->sdev_gendev.parent;
@@ -502,6 +507,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 
 	if (parent)
 		put_device(parent);
+
+	if (put_lld_mod_refcnt)
+		module_put(lld_mod);
 }
 
 static void scsi_device_dev_release(struct device *dev)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 430b73bd02ac..9d3fcb9cfd01 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -206,6 +206,7 @@ struct scsi_device {
 	unsigned rpm_autosuspend:1;	/* Enable runtime autosuspend at device
 					 * creation time */
 	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
+	unsigned put_lld_mod_refcnt:1;  /* Put LLD mod refcnt */
 
 	bool offline_already;		/* Device offline message logged */
 
-- 
2.31.1

Re: [PATCH 1/2] driver core: tell caller if the device/kboject is really released

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 01:20:27PM +0800, Ming Lei wrote:
> Return if the device/kobject is really released to caller.
> 
> One use case is scsi_device_put() and the scsi device's release handler
> runs async work to clean up things. We have to piggyback the module_put()
> into the async work for avoiding to touch unmapped module page.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/base/core.c     | 5 +++--
>  include/linux/device.h  | 2 +-
>  include/linux/kobject.h | 2 +-
>  lib/kobject.c           | 5 +++--
>  4 files changed, 8 insertions(+), 6 deletions(-)

I really don't like this as you should not ever care if you are
releasing the last reference on an object or not.

Why are you needing this?

And if you really do need this, you MUST document how this works in the
apis you are changing here, so I can't take this as is sorry.

greg k-h

Re: [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 01:20:28PM +0800, Ming Lei wrote:
> SCSI host release is triggered when SCSI device is released, and we have to
> make sure that LLD module won't be unloaded before SCSI host instance is
> released.
> 
> So put LLD module refcnt after SCSI device is released.
> 
> SCSI device release may be moved into workqueue context if scsi_device_put
> is called in interrupt context, and handle this case by piggybacking
> putting LLD module refcnt into SCSI device release handler.
> 
> Reported-by: Changhui Zhong <czhong@redhat.com>
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/scsi/scsi.c        | 14 ++++++++++++--
>  drivers/scsi/scsi_sysfs.c  |  8 ++++++++
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b241f9e3885c..7cad256ba895 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -553,8 +553,18 @@ EXPORT_SYMBOL(scsi_device_get);
>   */
>  void scsi_device_put(struct scsi_device *sdev)
>  {
> -	module_put(sdev->host->hostt->module);
> -	put_device(&sdev->sdev_gendev);
> +	struct module *mod = sdev->host->hostt->module;
> +	/*
> +	 * sdev->sdev_gendev's real release handler will be scheduled into
> +	 * user context if we are in interrupt context, and we have to put
> +	 * LLD module refcnt after the device is really released.
> +	 */
> +	preempt_disable();
> +	if (put_device(&sdev->sdev_gendev) && in_interrupt())

Why does in_interrupt() matter here?  And is this even set if you have
threaded interrupts?

This feels very wrong as you are doing something different if this is
called depending on the context and you really do not have control over
the context of when this is called at all.

What problem is this solving?  How is a host controller driver being
unloaded before the children it controls are removed?  Who is holding a
reference on them and why is this happening only now?

And who cares about unloading the kernel module in this fashion?

thanks,

greg k-h

Re: [PATCH 1/2] driver core: tell caller if the device/kboject is really released

[Ming Lei]

On Thu, Sep 30, 2021 at 07:51:35AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 01:20:27PM +0800, Ming Lei wrote:
> > Return if the device/kobject is really released to caller.
> > 
> > One use case is scsi_device_put() and the scsi device's release handler
> > runs async work to clean up things. We have to piggyback the module_put()
> > into the async work for avoiding to touch unmapped module page.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/base/core.c     | 5 +++--
> >  include/linux/device.h  | 2 +-
> >  include/linux/kobject.h | 2 +-
> >  lib/kobject.c           | 5 +++--
> >  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> I really don't like this as you should not ever care if you are
> releasing the last reference on an object or not.
> 
> Why are you needing this?
> 
> And if you really do need this, you MUST document how this works in the
> apis you are changing here, so I can't take this as is sorry.

Please ignore this series, and I have one better approach to address the
issue, will send it out soon.


Thanks,
Ming