Messages in this thread | | | Date | Mon, 11 Jul 2022 10:12:51 +0200 | From | Greg KH <> | Subject | Re: [PATCH v2 3/4] PM: hibernate: allow wait_for_device_probe() to timeout when resuming from hibernation |
| |
On Sun, Jul 10, 2022 at 11:25:08AM +0900, Tetsuo Handa wrote: > syzbot is reporting hung task at misc_open() [1], for there is a race > window of AB-BA deadlock which involves probe_count variable. > > Even with "char: misc: allow calling open() callback without misc_mtx > held" and "PM: hibernate: call wait_for_device_probe() without > system_transition_mutex held", wait_for_device_probe() from snapshot_open() > can sleep forever if probe_count cannot become 0. > > Since snapshot_open() is a userland-driven hibernation/resume request, > it should be acceptable to fail if something is wrong. Users would not > want to wait for hours if device stopped responding. Therefore, introduce > killable version of wait_for_device_probe() with timeout. > > According to Oliver Neukum, there are SCSI commands that can run for more > than 60 seconds. Therefore, this patch choose 5 minutes for timeout. > > Link: https://syzkaller.appspot.com/bug?extid=358c9ab4c93da7b7238c [1] > Reported-by: syzbot <syzbot+358c9ab4c93da7b7238c@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: Greg KH <gregkh@linuxfoundation.org> > Cc: Oliver Neukum <oneukum@suse.com> > Cc: Wedson Almeida Filho <wedsonaf@google.com> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Arjan van de Ven <arjan@linux.intel.com> > --- > drivers/base/dd.c | 14 ++++++++++++++ > include/linux/device/driver.h | 1 + > kernel/power/user.c | 9 +++++++-- > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 70f79fc71539..3136b8403bb0 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -724,6 +724,20 @@ void wait_for_device_probe(void) > } > EXPORT_SYMBOL_GPL(wait_for_device_probe); > > +void wait_for_device_probe_killable_timeout(unsigned long timeout) > +{ > + /* wait for probe timeout */ > + wait_event(probe_timeout_waitqueue, !driver_deferred_probe_timeout); > + > + /* wait for the deferred probe workqueue to finish */ > + flush_work(&deferred_probe_work); > + > + /* wait for the known devices to complete their probing */ > + wait_event_killable_timeout(probe_waitqueue, > + atomic_read(&probe_count) == 0, timeout); > + async_synchronize_full(); > +} > + > static int __driver_probe_device(struct device_driver *drv, struct device *dev) > { > int ret = 0; > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 7acaabde5396..4ee909144470 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -129,6 +129,7 @@ extern struct device_driver *driver_find(const char *name, > struct bus_type *bus); > extern int driver_probe_done(void); > extern void wait_for_device_probe(void); > +extern void wait_for_device_probe_killable_timeout(unsigned long timeout); > void __init wait_for_init_devices_probe(void); > > /* sysfs interface for exporting driver attributes */ > diff --git a/kernel/power/user.c b/kernel/power/user.c > index db98a028dfdd..32dd5a855e8c 100644 > --- a/kernel/power/user.c > +++ b/kernel/power/user.c > @@ -58,8 +58,13 @@ static int snapshot_open(struct inode *inode, struct file *filp) > /* The image device should be already ready. */ > break; > default: /* Resuming */ > - /* We may need to wait for the image device to appear. */ > - wait_for_device_probe(); > + /* > + * Since the image device might not be ready, try to wait up to > + * 5 minutes. We should not wait forever, for we might get stuck > + * due to unresponsive devices and/or new probe events which > + * are irrelevant to the image device keep coming in. > + */ > + wait_for_device_probe_killable_timeout(300 * HZ);
5 minutes is a total random choice. anything that calls wait_for_device_probe() feels wrong for other reasons, creating a locking loop like this should be resolved first, not just papering over it by allowing 5 minutes to pass before it resolves itself. 5 minutes is forever and any sane watchdog detector would have rebooted the machine by then.
thanks,
greg k-h
| |