* [PATCH 0/2] SCSI: fix race between releasing shost and unloading LLD module @ 2021-09-30 5:20 Ming Lei 2021-09-30 5:20 ` [PATCH 1/2] driver core: tell caller if the device/kboject is really released Ming Lei 2021-09-30 5:20 ` [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released Ming Lei 0 siblings, 2 replies; 6+ messages in thread From: Ming Lei @ 2021-09-30 5:20 UTC (permalink / raw) To: linux-kernel, linux-scsi, Greg Kroah-Hartman, Martin K . Petersen Cc: Changhui Zhong, Yi Zhang, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] driver core: tell caller if the device/kboject is really released 2021-09-30 5:20 [PATCH 0/2] SCSI: fix race between releasing shost and unloading LLD module Ming Lei @ 2021-09-30 5:20 ` Ming Lei 2021-09-30 5:51 ` Greg Kroah-Hartman 2021-09-30 5:20 ` [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released Ming Lei 1 sibling, 1 reply; 6+ messages in thread From: Ming Lei @ 2021-09-30 5:20 UTC (permalink / raw) To: linux-kernel, linux-scsi, Greg Kroah-Hartman, Martin K . Petersen Cc: Changhui Zhong, Yi Zhang, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] driver core: tell caller if the device/kboject is really released 2021-09-30 5:20 ` [PATCH 1/2] driver core: tell caller if the device/kboject is really released Ming Lei @ 2021-09-30 5:51 ` Greg Kroah-Hartman 2021-09-30 7:22 ` Ming Lei 0 siblings, 1 reply; 6+ messages in thread From: Greg Kroah-Hartman @ 2021-09-30 5:51 UTC (permalink / raw) To: Ming Lei Cc: linux-kernel, linux-scsi, Martin K . Petersen, Changhui Zhong, Yi Zhang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] driver core: tell caller if the device/kboject is really released 2021-09-30 5:51 ` Greg Kroah-Hartman @ 2021-09-30 7:22 ` Ming Lei 0 siblings, 0 replies; 6+ messages in thread From: Ming Lei @ 2021-09-30 7:22 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, linux-scsi, Martin K . Petersen, Changhui Zhong, Yi Zhang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released 2021-09-30 5:20 [PATCH 0/2] SCSI: fix race between releasing shost and unloading LLD module Ming Lei 2021-09-30 5:20 ` [PATCH 1/2] driver core: tell caller if the device/kboject is really released Ming Lei @ 2021-09-30 5:20 ` Ming Lei 2021-09-30 5:57 ` Greg Kroah-Hartman 1 sibling, 1 reply; 6+ messages in thread From: Ming Lei @ 2021-09-30 5:20 UTC (permalink / raw) To: linux-kernel, linux-scsi, Greg Kroah-Hartman, Martin K . Petersen Cc: Changhui Zhong, Yi Zhang, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released 2021-09-30 5:20 ` [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released Ming Lei @ 2021-09-30 5:57 ` Greg Kroah-Hartman 0 siblings, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2021-09-30 5:57 UTC (permalink / raw) To: Ming Lei Cc: linux-kernel, linux-scsi, Martin K . Petersen, Changhui Zhong, Yi Zhang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-30 7:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-30 5:20 [PATCH 0/2] SCSI: fix race between releasing shost and unloading LLD module Ming Lei 2021-09-30 5:20 ` [PATCH 1/2] driver core: tell caller if the device/kboject is really released Ming Lei 2021-09-30 5:51 ` Greg Kroah-Hartman 2021-09-30 7:22 ` Ming Lei 2021-09-30 5:20 ` [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released Ming Lei 2021-09-30 5:57 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.