linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
@ 2016-07-07  6:40 Chen Yu
  2016-07-13  9:50 ` Pavel Machek
  0 siblings, 1 reply; 13+ messages in thread
From: Chen Yu @ 2016-07-07  6:40 UTC (permalink / raw)
  To: linux-pm
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, linux-kernel, Chen Yu

This mode is to verify if the snapshot data written to
swap device can be successfully restored to memory. It
is useful to ease the debugging process on hibernation,
since this mode can not only bypass the BIOSen/bootloader,
but also the system re-initialization.

For example:
$ sudo echo snapshot > /sys/power/disk
$ sudo echo disk > /sys/power/state

/* manual resume.*/
$ sudo echo 8:3 > /sys/power/resume

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 kernel/power/hibernate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fca9254..667d926 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -52,6 +52,7 @@ enum {
 #ifdef CONFIG_SUSPEND
 	HIBERNATION_SUSPEND,
 #endif
+	HIBERNATION_SNAPSHOT,
 	/* keep last */
 	__HIBERNATION_AFTER_LAST
 };
@@ -631,6 +632,9 @@ static void power_down(void)
 			                "Try swapon -a.\n");
 		return;
 #endif
+	case HIBERNATION_SNAPSHOT:
+		/* Do nothing. */
+		return;
 	}
 	kernel_halt();
 	/*
@@ -878,6 +882,7 @@ static const char * const hibernation_modes[] = {
 #ifdef CONFIG_SUSPEND
 	[HIBERNATION_SUSPEND]	= "suspend",
 #endif
+	[HIBERNATION_SNAPSHOT]	= "snapshot",
 };
 
 /*
@@ -924,6 +929,7 @@ static ssize_t disk_show(struct kobject *kobj, struct kobj_attribute *attr,
 #ifdef CONFIG_SUSPEND
 		case HIBERNATION_SUSPEND:
 #endif
+		case HIBERNATION_SNAPSHOT:
 			break;
 		case HIBERNATION_PLATFORM:
 			if (hibernation_ops)
@@ -970,6 +976,7 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 #ifdef CONFIG_SUSPEND
 		case HIBERNATION_SUSPEND:
 #endif
+		case HIBERNATION_SNAPSHOT:
 			hibernation_mode = mode;
 			break;
 		case HIBERNATION_PLATFORM:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-07  6:40 [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation Chen Yu
@ 2016-07-13  9:50 ` Pavel Machek
  2016-07-13 10:20   ` Chen Yu
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2016-07-13  9:50 UTC (permalink / raw)
  To: Chen Yu; +Cc: linux-pm, Rafael J. Wysocki, Len Brown, linux-kernel

On Thu 2016-07-07 14:40:58, Chen Yu wrote:
> This mode is to verify if the snapshot data written to
> swap device can be successfully restored to memory. It
> is useful to ease the debugging process on hibernation,
> since this mode can not only bypass the BIOSen/bootloader,
> but also the system re-initialization.
> 
> For example:
> $ sudo echo snapshot > /sys/power/disk
> $ sudo echo disk > /sys/power/state
> 
> /* manual resume.*/
> $ sudo echo 8:3 > /sys/power/resume

Your examples will not work, will they?

This is also quite tricky/dangerous. If you do this with filesystems
mounted R/W, it is "good bye, filesystems".

I guess updating documentation would be welcome from my side,
otherwise it should be ok.

Acked-by: Pavel Machek <pavel@ucw.cz>

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13  9:50 ` Pavel Machek
@ 2016-07-13 10:20   ` Chen Yu
  2016-07-13 10:21     ` Pavel Machek
  0 siblings, 1 reply; 13+ messages in thread
From: Chen Yu @ 2016-07-13 10:20 UTC (permalink / raw)
  To: Pavel Machek; +Cc: linux-pm, Rafael J. Wysocki, Len Brown, linux-kernel

Hi Pavel,
thanks for your reply,

On 2016年07月13日 17:50, Pavel Machek wrote:
> On Thu 2016-07-07 14:40:58, Chen Yu wrote:
>> This mode is to verify if the snapshot data written to
>> swap device can be successfully restored to memory. It
>> is useful to ease the debugging process on hibernation,
>> since this mode can not only bypass the BIOSen/bootloader,
>> but also the system re-initialization.
>>
>> For example:
>> $ sudo echo snapshot > /sys/power/disk
>> $ sudo echo disk > /sys/power/state
>>
>> /* manual resume.*/
>> $ sudo echo 8:3 > /sys/power/resume
> Your examples will not work, will they?
It works on my platform, although I did not tested it for too many rounds.
And here's a revised version of v2, which introduced a new test mode
in pm_test, thus users do not need to run a manual resume.
https://patchwork.kernel.org/patch/9226837/
>
> This is also quite tricky/dangerous. If you do this with filesystems
> mounted R/W, it is "good bye, filesystems".
Ah, yes, this is quite tricky, maybe we can use this option as a debug 
method,
for example, boot with rootfs = initrd, without mounting any disks,
and then swapon the swap device, and do a testing. This should be safer?
>
> I guess updating documentation would be welcome from my side,
> otherwise it should be ok.
OK, I'll  update the documents.
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
thanks.
>
> Best regards,
> 								Pavel
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 10:20   ` Chen Yu
@ 2016-07-13 10:21     ` Pavel Machek
  2016-07-13 10:39       ` Chen Yu
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2016-07-13 10:21 UTC (permalink / raw)
  To: Chen Yu; +Cc: linux-pm, Rafael J. Wysocki, Len Brown, linux-kernel

Hi!

> On 2016???07???13??? 17:50, Pavel Machek wrote:
> >On Thu 2016-07-07 14:40:58, Chen Yu wrote:
> >>This mode is to verify if the snapshot data written to
> >>swap device can be successfully restored to memory. It
> >>is useful to ease the debugging process on hibernation,
> >>since this mode can not only bypass the BIOSen/bootloader,
> >>but also the system re-initialization.
> >>
> >>For example:
> >>$ sudo echo snapshot > /sys/power/disk
> >>$ sudo echo disk > /sys/power/state
> >>
> >>/* manual resume.*/
> >>$ sudo echo 8:3 > /sys/power/resume
> >Your examples will not work, will they?
> It works on my platform, although I did not tested it for too many
> rounds.

Please check again:

sudo echo disk > /sys/power/state
-bash: /sys/power/state: Permission denied

...because bash does the open, not echo.

> >This is also quite tricky/dangerous. If you do this with filesystems
> >mounted R/W, it is "good bye, filesystems".
> Ah, yes, this is quite tricky, maybe we can use this option as a
> debug method,
> for example, boot with rootfs = initrd, without mounting any disks,
> 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.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 10:21     ` Pavel Machek
@ 2016-07-13 10:39       ` Chen Yu
  2016-07-13 17:01         ` Pavel Machek
  0 siblings, 1 reply; 13+ messages in thread
From: Chen Yu @ 2016-07-13 10:39 UTC (permalink / raw)
  To: Pavel Machek; +Cc: linux-pm, Rafael J. Wysocki, Len Brown, linux-kernel



On 2016年07月13日 18:21, Pavel Machek wrote:
> Hi!
>
>> On 2016???07???13??? 17:50, Pavel Machek wrote:
>>> On Thu 2016-07-07 14:40:58, Chen Yu wrote:
>>>> This mode is to verify if the snapshot data written to
>>>> swap device can be successfully restored to memory. It
>>>> is useful to ease the debugging process on hibernation,
>>>> since this mode can not only bypass the BIOSen/bootloader,
>>>> but also the system re-initialization.
>>>>
>>>> For example:
>>>> $ sudo echo snapshot > /sys/power/disk
>>>> $ sudo echo disk > /sys/power/state
>>>>
>>>> /* manual resume.*/
>>>> $ sudo echo 8:3 > /sys/power/resume
>>> Your examples will not work, will they?
>> It works on my platform, although I did not tested it for too many
>> rounds.
> Please check again:
>
> sudo echo disk > /sys/power/state
> -bash: /sys/power/state: Permission denied
>
> ...because bash does the open, not echo.
Sorry, my bad, I logined as root:P
I'll rewrite the commit log.
>>> This is also quite tricky/dangerous. If you do this with filesystems
>>> mounted R/W, it is "good bye, filesystems".
>> Ah, yes, this is quite tricky, maybe we can use this option as a
>> debug method,
>> for example, boot with rootfs = initrd, without mounting any disks,
>> 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.
> Thanks,
> 									Pavel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 10:39       ` Chen Yu
@ 2016-07-13 17:01         ` Pavel Machek
  2016-07-13 20:04           ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2016-07-13 17:01 UTC (permalink / raw)
  To: Chen Yu; +Cc: linux-pm, Rafael J. Wysocki, Len Brown, linux-kernel

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.)

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 17:01         ` Pavel Machek
@ 2016-07-13 20:04           ` Rafael J. Wysocki
  2016-07-13 20:26             ` Pavel Machek
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2016-07-13 20:04 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Chen Yu, Linux PM, Rafael J. Wysocki, Len Brown,
	Linux Kernel Mailing List

On Wed, Jul 13, 2016 at 7:01 PM, Pavel Machek <pavel@ucw.cz> 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/

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 20:04           ` Rafael J. Wysocki
@ 2016-07-13 20:26             ` Pavel Machek
  2016-07-13 20:44               ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2016-07-13 20:26 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Chen Yu, Linux PM, Rafael J. Wysocki, Len Brown,
	Linux Kernel Mailing List

On Wed 2016-07-13 22:04:27, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2016 at 7:01 PM, Pavel Machek <pavel@ucw.cz> 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.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 20:26             ` Pavel Machek
@ 2016-07-13 20:44               ` Rafael J. Wysocki
  2016-07-13 21:45                 ` Pavel Machek
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2016-07-13 20:44 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Rafael J. Wysocki, Chen Yu, Linux PM, Rafael J. Wysocki,
	Len Brown, Linux Kernel Mailing List

On Wed, Jul 13, 2016 at 10:26 PM, Pavel Machek <pavel@ucw.cz> 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@ucw.cz> 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.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 20:44               ` Rafael J. Wysocki
@ 2016-07-13 21:45                 ` Pavel Machek
  2016-07-13 22:00                   ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2016-07-13 21:45 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Chen Yu, Linux PM, Rafael J. Wysocki, Len Brown,
	Linux Kernel Mailing List

On Wed 2016-07-13 22:44:24, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2016 at 10:26 PM, Pavel Machek <pavel@ucw.cz> 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@ucw.cz> 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?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 21:45                 ` Pavel Machek
@ 2016-07-13 22:00                   ` Rafael J. Wysocki
  2016-07-13 22:18                     ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2016-07-13 22:00 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Rafael J. Wysocki, Chen Yu, Linux PM, Rafael J. Wysocki,
	Len Brown, Linux Kernel Mailing List

On Wed, Jul 13, 2016 at 11:45 PM, Pavel Machek <pavel@ucw.cz> 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@ucw.cz> 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@ucw.cz> 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.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 22:00                   ` Rafael J. Wysocki
@ 2016-07-13 22:18                     ` Rafael J. Wysocki
  2016-07-14 10:44                       ` Chen Yu
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2016-07-13 22:18 UTC (permalink / raw)
  To: Pavel Machek, Chen Yu
  Cc: Linux PM, Rafael J. Wysocki, Len Brown, Linux Kernel Mailing List

On Thu, Jul 14, 2016 at 12:00 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jul 13, 2016 at 11:45 PM, Pavel Machek <pavel@ucw.cz> 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@ucw.cz> 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@ucw.cz> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation
  2016-07-13 22:18                     ` Rafael J. Wysocki
@ 2016-07-14 10:44                       ` Chen Yu
  0 siblings, 0 replies; 13+ messages in thread
From: Chen Yu @ 2016-07-14 10:44 UTC (permalink / raw)
  To: Rafael J. Wysocki, Pavel Machek
  Cc: Linux PM, Rafael J. Wysocki, Len Brown, Linux Kernel Mailing List

Hi,

On 2016年07月14日 06:18, Rafael J. Wysocki wrote:
> On Thu, Jul 14, 2016 at 12:00 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Jul 13, 2016 at 11:45 PM, Pavel Machek <pavel@ucw.cz> 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@ucw.cz> 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@ucw.cz> 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().
OK, I've used this solution and sent a v3 out.
thanks!
>
> Thanks,
> Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-07-14 10:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-07  6:40 [PATCH] PM / hibernate: Introduce snapshot test mode for hibernation Chen Yu
2016-07-13  9:50 ` Pavel Machek
2016-07-13 10:20   ` Chen Yu
2016-07-13 10:21     ` Pavel Machek
2016-07-13 10:39       ` Chen Yu
2016-07-13 17:01         ` Pavel Machek
2016-07-13 20:04           ` Rafael J. Wysocki
2016-07-13 20:26             ` Pavel Machek
2016-07-13 20:44               ` Rafael J. Wysocki
2016-07-13 21:45                 ` Pavel Machek
2016-07-13 22:00                   ` Rafael J. Wysocki
2016-07-13 22:18                     ` Rafael J. Wysocki
2016-07-14 10:44                       ` Chen Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).