linux-kernel - Re: [PATCH] power: suspend: Add suspend timeout handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Oct 2020 11:01:08 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Joseph Jang <josephjang@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        jonglin@...gle.com, woodylin@...gle.com, markcheng@...gle.com
Subject: Re: [PATCH] power: suspend: Add suspend timeout handler

On Fri 2020-10-16 11:51:09, Joseph Jang wrote:
> From: josephjang <josephjang@...gle.com>
> 
> Add suspend timeout handler to prevent device stuck during suspend/
> resume process. Suspend timeout handler will dump disk sleep task
> at first round timeout and trigger kernel panic at second round timeout.
> The default timer for each round is 30 seconds.

A better solution would be to resume instead of panic().

> Note: Can use following command to simulate suspend hang for testing.
>     adb shell echo 1 > /sys/power/pm_hang

This looks dangerous. It adds a simple way to panic() the system.

First, it should get enabled separately. e.g.
CONFIG_TEST_PM_SLEEP_MONITOR.

Second, I would add it as a module that might get loaded
and unloaded.

> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8b1bb5ee7e5d..6f2679cfd9d1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> +static int suspend_monitor_kthread(void *arg)
> +{
> +	long err;
> +	struct sched_param param = {.sched_priority
> +		= MAX_RT_PRIO-1};
> +	static int timeout_count;
> +	static long timeout;
> +
> +	pr_info("Init ksuspend_mon thread\n");
> +
> +	sched_setscheduler(current, SCHED_FIFO, &param);
> +
> +	timeout_count = 0;
> +	timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	do {
> +		/* Wait suspend timer timeout */
> +		err = wait_event_interruptible_timeout(
> +			power_suspend_waitqueue,
> +			(suspend_mon_toggle != TOGGLE_NONE),
> +			timeout);
> +
> +		mutex_lock(&suspend_mon_lock);
> +		/* suspend monitor state change */
> +		if (suspend_mon_toggle != TOGGLE_NONE) {
> +			if (suspend_mon_toggle == TOGGLE_START) {
> +				timeout = msecs_to_jiffies(
> +					SUSPEND_TIMER_TIMEOUT_MS);
> +				pr_info("Start suspend monitor\n");
> +			} else if (suspend_mon_toggle == TOGGLE_STOP) {
> +				timeout = MAX_SCHEDULE_TIMEOUT;
> +				timeout_count = 0;
> +				pr_info("Stop suspend monitor\n");
> +			}
> +			suspend_mon_toggle = TOGGLE_NONE;
> +			mutex_unlock(&suspend_mon_lock);
> +			continue;
> +		}
> +		mutex_unlock(&suspend_mon_lock);
> +
> +		/* suspend monitor event handler */
> +		if (err == 0) {
> +			timeout_count++;
> +			suspend_timeout(timeout_count);
> +		} else if (err == -ERESTARTSYS) {
> +			pr_info("Exit ksuspend_mon!");
> +			break;
> +		}
> +	} while (1);
> +
> +	return 0;
> +}

Using kthread looks like an overkill to me. I wonder how this actually
works when the kthreads get freezed. It might be enough to implement
just a timer callback. Start the timer in start_suspend_mon() and
delete it in stop_suspend_mon(). Or do I miss anything?

Anyway, the kthread implementation looks a but hairy. If you really
need to use kthread, I suggest to use kthread_worker API. You would
need to run an init work to setup the RT scheduling. Then you
could just call kthread_queue_delayed_work(()
and kthread_cancel_delayed_work_sync() to start and stop
the monitor.


> @@ -114,6 +251,10 @@ static void s2idle_enter(void)
>  	s2idle_state = S2IDLE_STATE_NONE;
>  	raw_spin_unlock_irq(&s2idle_lock);
>  
> +#ifdef CONFIG_PM_SLEEP_MONITOR
> +	start_suspend_mon();
> +#endif

It is better to solve this by defining start_suspend_mon() as empty
function when the config option is disabled. For example, see
how  vgacon_text_force() is defined in console.h.

Best Regards,
Petr