| 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
| ||
|
Date: Mon, 20 Feb 2023 22:18:57 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Michael Nazzareno Trimarchi <michael@...rulasolutions.com>
Cc: John Stultz <jstultz@...gle.com>,
LKML <linux-kernel@...r.kernel.org>,
Stephen Boyd <sboyd@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Michael <michael@...isi.de>, kernel-team@...roid.com,
Peter Zijlstra <peterz@...radead.org>,
"Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK_FREEZABLE to
cleanup freezer handling
Michael!
On Mon, Feb 20 2023 at 19:11, Michael Nazzareno Trimarchi wrote:
> On Mon, Feb 20, 2023 at 6:48 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>> [ 27.349352] alarmtimer_enqueue()
>>
>> U: Before SUSPEND
>>
>> [ 31.353228] PM: suspend entry (s2idle)
>> [ 31.388857] Filesystems sync: 0.033 seconds
>> [ 31.418427] Freezing user space processes
>> [ 31.422406] Freezing user space processes completed (elapsed 0.002 seconds)
>> [ 31.425435] OOM killer disabled.
>> [ 31.426833] Freezing remaining freezable tasks
>> [ 31.429838] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
>> [ 31.432922] printk: Suspending console(s) (use no_console_suspend to debug)
>> [ 31.435912] alarmtimer alarmtimer.0.auto: PM: dpm_run_callback(): platform_pm_suspend+0x0/0x50 returns -16
>> [ 31.435954] alarmtimer alarmtimer.0.auto: PM: failed to suspend: error -16
>>
>> That means the RTC interrupt was raised before the system was able to suspend.
>
> if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
> pm_wakeup_event(dev, 2 * MSEC_PER_SEC);
> return -EBUSY;
> }
>
> I think that above happens to you. So it means that you are too close
> to this limit, can be?
Maybe.
> Yes but the alarm for me was set to be fired just before freezing. Is
> this a valid scenario?
Sure why not?
> Let's say that I set an alarm to be fired just before the userspace
> freeze happens. If I'm close to it then then process will not process
> the signal to enquene again the alarm in the list and then during
> alarm suspend the list will be empty for the above.
Halfways, but slowly your explanations start to make sense. Here is the
dmesg output you provided:
> [ 89.674127] PM: suspend entry (deep)
> [ 89.714916] Filesystems sync: 0.037 seconds
> [ 89.733594] Freezing user space processes
> [ 89.740680] Freezing user space processes completed (elapsed 0.002 seconds)
User space tasks are frozen now.
> [ 89.748593] OOM killer disabled.
> [ 89.752257] Freezing remaining freezable tasks
> [ 89.756807] alarmtimer_fired: called
> [ 89.756831] alarmtimer_dequeue: called <---- HERE
Here fires the underlying hrtimer before devices are suspended, so the
sig_sendqueue() cannot wake up the task because task->state ==
TASK_FROZEN, which means the signal wont be handled and the timer wont
be rearmed until the task is thawed.
And as you correctly observed the alarmtimer_suspend() path won't see a
pending timer anymore because it is dequeued.
So precisely the time between freeze(alarmtask) and alarmtimer_suspend()
is a gaping hole which guarantees lost wakeups.
That's completely unrelated to Johns patches. That hole has been there
forever.
I assume that this horrible freezer hackery was supposed to plug that
hole, but that gem is not solving anything as far as I understand what
it is doing. I'm still failing to understand what it is supposed to
solve, but that's a different story.
Aside of that I can clearly reproduce the issue, now that I understand
what you are trying to tell, on plain 6.2 without and with John's
cleanup.
Something like the below plugs the hole reliably.
Thanks,
tglx
---
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -26,6 +26,7 @@
#include <linux/freezer.h>
#include <linux/compat.h>
#include <linux/module.h>
+#include <linux/suspend.h>
#include <linux/time_namespace.h>
#include "posix-timers.h"
@@ -176,6 +177,7 @@ static void alarmtimer_dequeue(struct al
alarm->state &= ~ALARMTIMER_STATE_ENQUEUED;
}
+static atomic_t alarmtimer_wakeup;
/**
* alarmtimer_fired - Handles alarm hrtimer being fired.
@@ -194,6 +196,8 @@ static enum hrtimer_restart alarmtimer_f
int ret = HRTIMER_NORESTART;
int restart = ALARMTIMER_NORESTART;
+ atomic_inc(&alarmtimer_wakeup);
+
spin_lock_irqsave(&base->lock, flags);
alarmtimer_dequeue(base, alarm);
spin_unlock_irqrestore(&base->lock, flags);
@@ -244,6 +248,16 @@ static int alarmtimer_suspend(struct dev
if (!rtc)
return 0;
+ /*
+ * Handle wakeups which happened between the start of suspend and
+ * now as those wakeups might have tried to wake up a frozen task
+ * which means they are not longer in the alarm timer list.
+ */
+ if (atomic_read(&alarmtimer_wakeup)) {
+ pm_wakeup_event(dev, 0);
+ return -EBUSY;
+ }
+
/* Find the soonest timer to expire*/
for (i = 0; i < ALARM_NUMTYPE; i++) {
struct alarm_base *base = &alarm_bases[i];
@@ -296,6 +310,31 @@ static int alarmtimer_resume(struct devi
return 0;
}
+static int alarmtimer_pm_notifier_fn(struct notifier_block *bl, unsigned long state,
+ void *unused)
+{
+ switch (state) {
+ case PM_HIBERNATION_PREPARE:
+ case PM_POST_HIBERNATION:
+ atomic_set(&alarmtimer_wakeup, 0);
+ break;
+ }
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block alarmtimer_pm_notifier = {
+ .notifier_call = alarmtimer_pm_notifier_fn,
+};
+
+static inline int alarmtimer_register_pm_notifier(void)
+{
+ return register_pm_notifier(&alarmtimer_pm_notifier);
+}
+
+static inline void alarmtimer_unregister_pm_notifier(void)
+{
+ unregister_pm_notifier(&alarmtimer_pm_notifier);
+}
#else
static int alarmtimer_suspend(struct device *dev)
{
@@ -306,6 +345,15 @@ static int alarmtimer_resume(struct devi
{
return 0;
}
+
+static inline int alarmtimer_register_pm_notifier(void)
+{
+ return 0;
+}
+
+static inline void alarmtimer_unregister_pm_notifier(void)
+{
+}
#endif
static void
@@ -904,11 +952,17 @@ static int __init alarmtimer_init(void)
if (error)
return error;
- error = platform_driver_register(&alarmtimer_driver);
+ error = alarmtimer_register_pm_notifier();
if (error)
goto out_if;
+ error = platform_driver_register(&alarmtimer_driver);
+ if (error)
+ goto out_pm;
+
return 0;
+out_pm:
+ alarmtimer_unregister_pm_notifier();
out_if:
alarmtimer_rtc_interface_remove();
return error;
Powered by blists - more mailing lists