Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation

[Rafael J. Wysocki]

On Thu, Jul 14, 2016 at 12:00 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Wed, Jul 13, 2016 at 11:45 PM, Pavel Machek <pavel@xxxxxx> wrote:
>> On Wed 2016-07-13 22:44:24, Rafael J. Wysocki wrote:
>>> On Wed, Jul 13, 2016 at 10:26 PM, Pavel Machek <pavel@xxxxxx> wrote:
>>> > On Wed 2016-07-13 22:04:27, Rafael J. Wysocki wrote:
>>> >> On Wed, Jul 13, 2016 at 7:01 PM, Pavel Machek <pavel@xxxxxx> wrote:
>>> >> > Hi!
>>> >> >
>>> >> >> >>and then swapon the swap device, and do a testing. This should be safer?
>>> >> >> >Yeah, that's the way. Read-only root is other option.
>>> >> >> >
>>> >> >> >>>I guess updating documentation would be welcome from my side,
>>> >> >> >>>otherwise it should be ok.
>>> >> >> >>OK, I'll  update the documents.
>>> >> >> >Just add fat warning into the documentation.
>>> >> >> OK.
>>> >> >
>>> >> > Actually... If you could add
>>> >> >
>>> >> > printk(KERN_ALERT "Hibernation image written. If you have any
>>> >> > filesystems mounted read-write and attempt to resume, you'll corrupt
>>> >> > your data. To prevent that, remove the hibernation image.\n")
>>> >> >
>>> >> > ...I guess that would save someone's filesystem. (Yes, very high
>>> >> > loglevel. If you attempt to do this from anything else then singleuser
>>> >> > or initrd, you are asking for problems, so... lets make sure user sees
>>> >> > it.)
>>> >>
>>> >> Please see the new version of this patch:
>>> >> https://patchwork.kernel.org/patch/9226837/
>>> >
>>> > New version changes nothing, right? You still need to be sure
>>> > filesystems are not mounted r/w. So I would still like to see printk()
>>> > with warning.
>>>
>>> It shouldn't matter how they are mounted, because the contents of
>>> persistent storage don't change.
>>
>> @@ -721,6 +724,9 @@  int hibernate(void)
>>    atomic_inc(&snapshot_device_available);
>>      Unlock:
>>         unlock_system_sleep();
>>         +       if (snapshot_test)
>>         +          software_resume();
>>         +
>>                 return error;
>>                  }
>>
>> Aha, I see, immediate wakeup here. Makes sense. ... ...
>>
>> No.
>>
>> AFAICT, freezer is used in hibernation_snapshot, which means at
>> Unlock:, kernel threads are running; software_resume() freezes them
>> again, but they had chance to run and potentially corrupt the
>> persistent storage... right?
>
> OK, there is a bug.
>
> The thawing of user space is potentially dangerous, so in the
> "snapshot" test mode hibernate() should just call
> free_basic_memory_bitmaps() and from there invoke the code below the
> Check_image label in software_resume(), roughly.

Or rather call free_basic_memory_bitmaps() and
unlock_device_hotplug(), then do swsusp_check() and invoke the code
starting with the "PM: Loading hibernation image.\n" message in
software_resume().

Thanks,
Rafael