linux-kernel - Re: [PATCH 1/2] PM: Add a switch for disabling/enabling sync() before, suspend

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:   Mon, 16 Dec 2019 17:54:49 +0100
From:   Jonas Meurer <jonas@...esources.org>
To:     Pavel Machek <pavel@....cz>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <len.brown@...el.com>,
        Tim Dittler <tim.dittler@...temli.org>,
        Yannik Sembritzki <yannik@...britzki.me>
Subject: Re: [PATCH 1/2] PM: Add a switch for disabling/enabling sync()
 before, suspend

Hello Pavel,

Pavel Machek:
> On Mon 2019-12-02 18:07:05, Jonas Meurer wrote:
>> The switch allows to enable or disable the final sync() from the suspend.c
>> Linux Kernel system suspend implementation. This is useful to avoid race
>> conditions if block devices have been suspended before. Be aware that you
>> have to take care of sync() yourself before suspending the system if you
>> disable it here.
>>
>> Signed-off-by: Jonas Meurer <jonas@...esources.org>
>> ---
>>  Documentation/ABI/testing/sysfs-power | 14 ++++++++++++
>>  include/linux/suspend.h               |  2 ++
>>  kernel/power/main.c                   | 33 +++++++++++++++++++++++++++
>>  kernel/power/suspend.c                |  2 +-
>>  4 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index 6f87b9dd384b..f164a364e89a 100644
>> --- a/Documentation/ABI/testing/sysfs-power
>> +++ b/Documentation/ABI/testing/sysfs-power
>> @@ -407,3 +407,17 @@ Contact:	Kalesh Singh <kaleshsingh96@...il.com>
>>  Description:
>>  		The /sys/power/suspend_stats/last_failed_step file contains
>>  		the last failed step in the suspend/resume path.
>> +
>> +What:		/sys/power/sync_on_suspend
>> +Date:		October 2019
>> +Contact:	Jonas Meurer <jonas@...esources.org>
>> +Description:
>> +		This file controls the switch to enable or disable the final
>> +		sync() before system suspend. This is useful to avoid race
>> +		conditions if block devices have been suspended before. Be
>> +		aware that you have to take care of sync() yourself before
>> +		suspending the system if you disable it here.
>> +
>> +		Writing a "1" (default) to this file enables the sync() and
>> +		writing a "0" disables it. Reads from the file return the
>> +		current value.
> 
> This pushes kernel's responsibility on the user, and adds to the mess
> we already have there.

I hear you and I agree that the current situation is not the cleanest one.

> Just... don't do it? Move sync somewhere where it can be done?

Can you elaborate on that? What do you mean with "where it can be done"?

I don't agree that removing sync from the suspend function is a good
idea. In the majority of use cases, the kernel actually is *expected* to
sync before suspend in order to prevent potential data loss, no?

So it's rather about the special cases like embedded world (where
suspend is used a lot for very short periods) and our use case, where
block devices are suspended before system suspend, where we don't want
the kernel to do the sync.

Also, if the sync was removed from kernel suspend function, all
userspace tools that trigger system suspend would have to be changed in
order to do it themselves. It would be a rather intrusive and
backwards-incompatible change in my eyes.

Cheers
 jonas