| Message ID | 2646e8a3-cc9f-c2c5-e4d6-c86de6e1b739@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/4] char: misc: allow calling open() callback without misc_mtx held | expand |
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
On 2022/07/11 17:12, Greg KH wrote: > creating a > locking loop like this should be resolved first, Rafael and Arjan, can we agree with removing wait_for_device_probe() from snapshot_open() ?
On 7/11/2022 3:44 AM, Tetsuo Handa wrote: > On 2022/07/11 17:12, Greg KH wrote: >> creating a >> locking loop like this should be resolved first, > > Rafael and Arjan, can we agree with removing wait_for_device_probe() from snapshot_open() ? > we can probably remove it. the "fun" is then that devices you need might not be ready once you remove it. so if we otherwise would panic, we should at least try again after some delay... (since a panic() is very nasty to debug for all the obvious reasons.. especially is the screen isn't on yet)
On Sun, Jul 10, 2022 at 4:25 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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. Not really. If you are resuming from hibernation and the image cannot be reached (which is the situation described above), failing and continuing to boot means discarding the image and possible user data loss. There is no "graceful failure" in this case. > Users would not want to wait for hours if device stopped responding. If the device holding the image is not responding, we should better wait for it or panic(). Or let the user make the system reboot.
On Mon, Jul 11, 2022 at 1:21 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2022/07/11 17:12, Greg KH wrote: > > creating a > > locking loop like this should be resolved first, > > Rafael and Arjan, can we agree with removing wait_for_device_probe() from snapshot_open() ? No, we can't.
On 2022/07/12 3:14, Rafael J. Wysocki wrote: > On Mon, Jul 11, 2022 at 1:21 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> On 2022/07/11 17:12, Greg KH wrote: >>> creating a >>> locking loop like this should be resolved first, >> >> Rafael and Arjan, can we agree with removing wait_for_device_probe() from snapshot_open() ? > > No, we can't. Then, can we defer wait_for_device_probe() till first write()/ioctl() which is called without locks? kernel/power/user.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/kernel/power/user.c b/kernel/power/user.c index a00a728ddfc1..92aecb989c76 100644 --- a/kernel/power/user.c +++ b/kernel/power/user.c @@ -26,6 +26,7 @@ #include "power.h" +static bool need_wait; static struct snapshot_data { struct snapshot_handle handle; @@ -78,7 +79,7 @@ static int snapshot_open(struct inode *inode, struct file *filp) * Resuming. We may need to wait for the image device to * appear. */ - wait_for_device_probe(); + need_wait = true; data->swap = -1; data->mode = O_WRONLY; @@ -168,6 +169,11 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf, ssize_t res; loff_t pg_offp = *offp & ~PAGE_MASK; + if (need_wait) { + wait_for_device_probe(); + need_wait = false; + } + lock_system_sleep(); data = filp->private_data; @@ -244,6 +250,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, loff_t size; sector_t offset; + if (need_wait) { + wait_for_device_probe(); + need_wait = false; + } + if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC) return -ENOTTY; if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR)
On Tue, Jul 12, 2022 at 3:52 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2022/07/12 3:14, Rafael J. Wysocki wrote: > > On Mon, Jul 11, 2022 at 1:21 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> > >> On 2022/07/11 17:12, Greg KH wrote: > >>> creating a > >>> locking loop like this should be resolved first, > >> > >> Rafael and Arjan, can we agree with removing wait_for_device_probe() from snapshot_open() ? > > > > No, we can't. > > Then, can we defer wait_for_device_probe() till first write()/ioctl() > which is called without locks? Yes, wait_for_device_probe() can be deferred to right before the first actual access. > kernel/power/user.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/kernel/power/user.c b/kernel/power/user.c > index a00a728ddfc1..92aecb989c76 100644 > --- a/kernel/power/user.c > +++ b/kernel/power/user.c > @@ -26,6 +26,7 @@ > > #include "power.h" > > +static bool need_wait; > > static struct snapshot_data { > struct snapshot_handle handle; > @@ -78,7 +79,7 @@ static int snapshot_open(struct inode *inode, struct file *filp) > * Resuming. We may need to wait for the image device to > * appear. > */ > - wait_for_device_probe(); > + need_wait = true; > > data->swap = -1; > data->mode = O_WRONLY; > @@ -168,6 +169,11 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf, > ssize_t res; > loff_t pg_offp = *offp & ~PAGE_MASK; > > + if (need_wait) { > + wait_for_device_probe(); > + need_wait = false; > + } > + > lock_system_sleep(); > > data = filp->private_data; > @@ -244,6 +250,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, > loff_t size; > sector_t offset; > > + if (need_wait) { > + wait_for_device_probe(); > + need_wait = false; > + } > + > if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC) > return -ENOTTY; > if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR) > -- > 2.18.4
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); break; }
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(-)