Re: [PATCH v3] PM: hibernate: defer device probing when resuming from hibernation

[Rafael J. Wysocki]

On Fri, Jul 15, 2022 at 7:51 AM Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> 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. Currently
> wait_for_device_probe() from snapshot_open() from misc_open() can sleep
> forever with misc_mtx held if probe_count cannot become 0.
>
> When a device is probed by hub_event() work function, probe_count is
> incremented before the probe function starts, and probe_count is
> decremented after the probe function completed.
>
> There are three cases that can prevent probe_count from dropping to 0.
>
>   (a) A device being probed stopped responding (i.e. broken/malicious
>       hardware).
>
>   (b) A process emulating a USB device using /dev/raw-gadget interface
>       stopped responding for some reason.
>
>   (c) New device probe requests keeps coming in before existing device
>       probe requests complete.
>
> The phenomenon syzbot is reporting is (b). A process which is holding
> system_transition_mutex and misc_mtx is waiting for probe_count to become
> 0 inside wait_for_device_probe(), but the probe function which is called
>  from hub_event() work function is waiting for the processes which are
> blocked at mutex_lock(&misc_mtx) to respond via /dev/raw-gadget interface.
>
> This patch mitigates (b) by deferring wait_for_device_probe() from
> snapshot_open() to snapshot_write() and snapshot_ioctl(). Please note that
> the possibility of (b) remains as long as any thread which is emulating a
> USB device via /dev/raw-gadget interface can be blocked by uninterruptible
> blocking operations (e.g. mutex_lock()).
>
> Please also note that (a) and (c) are not addressed. Regarding (c), we
> should change the code to wait for only one device which contains the
> image for resuming from hibernation. I don't know how to address (a), for
> use of timeout for wait_for_device_probe() might result in loss of user
> data in the image. Maybe we should require the userland to wait for the
> image device before opening /dev/snapshot interface.
>
> Link: https://syzkaller.appspot.com/bug?extid=358c9ab4c93da7b7238c [1]
> Reported-by: syzbot <syzbot+358c9ab4c93da7b7238c@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Tested-by: syzbot <syzbot+358c9ab4c93da7b7238c@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Oliver Neukum <oneukum@xxxxxxxx>
> Cc: Wedson Almeida Filho <wedsonaf@xxxxxxxxxx>
> Cc: Rafael J. Wysocki <rjw@xxxxxxx>
> Cc: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> ---
> Changes in v3:
>   Defer wait_for_device_probe() till first write()/ioctl() request.
>
> v2 is at https://lkml.kernel.org/r/03096156-3478-db03-c015-28643479116c@xxxxxxxxxxxxxxxxxxx .
>
> v1 is at https://lkml.kernel.org/r/72e74af9-f1b6-e383-a2c3-6ee8a0aea5e0@xxxxxxxxxxxxxxxxxxx .
>
>  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 ad241b4ff64c..d43c2aa583b2 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)
> --

Applied as 5.20 material, thanks!