[PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Mario Limonciello (AMD)]

commit 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in
hibernation_snapshot()") added a restrict GFP mask call that leads to
mismatch when using the platform for hibernation.  As part of calling
hibernation_platform_enter() the mask will be restricted when calling
dpm_suspend_start().

This is a similar problem as occurred with hybrid sleep that was fixed
by commit 469d80a3712c6 ("PM: hibernate: Fix hybrid-sleep").

Restore GFP maks as part of power_down() in HIBERNATION_PLATFORM case
to fix the mismatch.

Reported-by: Askar Safin <safinaskar@gmail.com>
Closes: https://lore.kernel.org/linux-pm/20251025050812.421905-1-safinaskar@gmail.com/
Fixes: 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in hibernation_snapshot()")
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
 kernel/power/hibernate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 14e85ff235512..e15907f28c4cd 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -721,6 +721,7 @@ static void power_down(void)
 		kernel_restart(NULL);
 		break;
 	case HIBERNATION_PLATFORM:
+		pm_restore_gfp_mask();
 		error = hibernation_platform_enter();
 		if (error == -EAGAIN || error == -EBUSY) {
 			events_check_enabled = false;
-- 
2.43.0

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Askar Safin]

On Sun, Oct 26, 2025 at 6:31 AM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
> commit 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in
> hibernation_snapshot()") added a restrict GFP mask call that leads to
> mismatch when using the platform for hibernation.  As part of calling
> hibernation_platform_enter() the mask will be restricted when calling
> dpm_suspend_start().

I tested the fix in Qemu. It indeed works.

-- 
Askar Safin

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Rafael J. Wysocki]

On Sun, Oct 26, 2025 at 4:31 AM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
>
> commit 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in
> hibernation_snapshot()") added a restrict GFP mask call that leads to
> mismatch when using the platform for hibernation.  As part of calling
> hibernation_platform_enter() the mask will be restricted when calling
> dpm_suspend_start().
>
> This is a similar problem as occurred with hybrid sleep that was fixed
> by commit 469d80a3712c6 ("PM: hibernate: Fix hybrid-sleep").
>
> Restore GFP maks as part of power_down() in HIBERNATION_PLATFORM case
> to fix the mismatch.
>
> Reported-by: Askar Safin <safinaskar@gmail.com>
> Closes: https://lore.kernel.org/linux-pm/20251025050812.421905-1-safinaskar@gmail.com/
> Fixes: 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in hibernation_snapshot()")
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>  kernel/power/hibernate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 14e85ff235512..e15907f28c4cd 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -721,6 +721,7 @@ static void power_down(void)
>                 kernel_restart(NULL);
>                 break;
>         case HIBERNATION_PLATFORM:
> +               pm_restore_gfp_mask();
>                 error = hibernation_platform_enter();
>                 if (error == -EAGAIN || error == -EBUSY) {
>                         events_check_enabled = false;
> --

Applied as 6.18-rc material, thanks!

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Askar Safin]

"Mario Limonciello (AMD)" <superm1@kernel.org>:
> commit 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in
> hibernation_snapshot()") added a restrict GFP mask call that leads to
> mismatch when using the platform for hibernation.  As part of calling
> hibernation_platform_enter() the mask will be restricted when calling
> dpm_suspend_start().

Are you sure this is proper solution?

As well as I understand, pm_restore_gfp_mask will make pm_suspended_storage
to return false. And this will enable swapping. Thus it is possible that
we will write some pages to swap after this pm_restore_gfp_mask call, and thus
we will damage swap.

Note: I'm not sure that my explanation is correct. Anyway I think you
should explain in commit message or comment why we will not damage swap
here.

-- 
Askar Safin

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Rafael J. Wysocki]

On Tue, Oct 28, 2025 at 12:17 PM Askar Safin <safinaskar@gmail.com> wrote:
>
> "Mario Limonciello (AMD)" <superm1@kernel.org>:
> > commit 449c9c02537a1 ("PM: hibernate: Restrict GFP mask in
> > hibernation_snapshot()") added a restrict GFP mask call that leads to
> > mismatch when using the platform for hibernation.  As part of calling
> > hibernation_platform_enter() the mask will be restricted when calling
> > dpm_suspend_start().
>
> Are you sure this is proper solution?
>
> As well as I understand, pm_restore_gfp_mask will make pm_suspended_storage
> to return false. And this will enable swapping. Thus it is possible that
> we will write some pages to swap after this pm_restore_gfp_mask call, and thus
> we will damage swap.

What kind of damage are you talking about, specifically?

> Note: I'm not sure that my explanation is correct. Anyway I think you
> should explain in commit message or comment why we will not damage swap
> here.

Well, if you have a specific failing scenario in mind, you need to say
what it is.  Otherwise you are effectively saying something like
"there is a bug in your code, but you need to find it yourself".

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Askar Safin]

On Tue, Oct 28, 2025 at 2:49 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> What kind of damage are you talking about, specifically?

Again: "pm_restore_gfp_mask" will restore "gfp_allowed_mask" to its
normal value,
which will contain __GFP_IO and __GFP_FS. Thus "pm_suspended_storage" will
start to return false.

But "pm_suspended_storage" is called here:
https://elixir.bootlin.com/linux/v6.18-rc3/source/mm/swapfile.c#L1895

(Also, please, read that big comment at this link.
Well, I have to admit I don't understand it in full.)

This check is needed to prevent swapping out pages during hibernation.

Call chain is so:
swap_writeout -> folio_free_swap -> folio_swapcache_freeable ->
pm_suspended_storage

So by calling "pm_restore_gfp_mask" we allow pages to be swapped out.

But we already wrote hibernation image by that point!

So swapping pages will make our swap partition inconsistent.

Moreover, as well as I understand, whole reason why we deal with GFP mask
in hibernation code is to prevent swapping out pages.

We restrict GFP before creating hibernation image here:
https://elixir.bootlin.com/linux/v6.18-rc3/source/kernel/power/hibernate.c#L463
.

We do this (as well as I understand) to prevent pages from swapping out.

And, starting from that moment, as well as I understand, we should
not restore GFP mask until either:
- we resume
- hibernation will abort for some reason (for example, "wake up event
detected during hibernation")

Again: I'm not trying to insult anybody. I'm just trying to help. And
to understand PM code.

I'm not sure that my explanations are correct.

In any case I think it will be good idea to add comment explaining why
restoring GFP mask
is safe there.

I'm attempting to write code to fix this bug:
https://lore.kernel.org/linux-pm/20251023112920.133897-1-safinaskar@gmail.com/

And this is why I'm trying to understand PM code.

-- 
Askar Safin

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Rafael J. Wysocki]

On Tue, Oct 28, 2025 at 1:56 PM Askar Safin <safinaskar@gmail.com> wrote:
>
> On Tue, Oct 28, 2025 at 2:49 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > What kind of damage are you talking about, specifically?
>
> Again: "pm_restore_gfp_mask" will restore "gfp_allowed_mask" to its
> normal value,
> which will contain __GFP_IO and __GFP_FS. Thus "pm_suspended_storage" will
> start to return false.
>
> But "pm_suspended_storage" is called here:
> https://elixir.bootlin.com/linux/v6.18-rc3/source/mm/swapfile.c#L1895
>
> (Also, please, read that big comment at this link.
> Well, I have to admit I don't understand it in full.)
>
> This check is needed to prevent swapping out pages during hibernation.

It is actually more complicated, but fair enough.

Basically, the concern is that a swap page can be freed and then
re-used for storing another page of memory after the image has been
created, so the swap metadata in the image would not match the reality
any more.

> Call chain is so:
> swap_writeout -> folio_free_swap -> folio_swapcache_freeable ->
> pm_suspended_storage
>
> So by calling "pm_restore_gfp_mask" we allow pages to be swapped out.
>
> But we already wrote hibernation image by that point!
>
> So swapping pages will make our swap partition inconsistent.
>
> Moreover, as well as I understand, whole reason why we deal with GFP mask
> in hibernation code is to prevent swapping out pages.

No, it is not the whole reason.  It is also done to avoid accessing
swap devices while they may not be (fully) operational.

> We restrict GFP before creating hibernation image here:
> https://elixir.bootlin.com/linux/v6.18-rc3/source/kernel/power/hibernate.c#L463
> .
>
> We do this (as well as I understand) to prevent pages from swapping out.
>
> And, starting from that moment, as well as I understand, we should
> not restore GFP mask until either:
> - we resume
> - hibernation will abort for some reason (for example, "wake up event
> detected during hibernation")

Again, this is slightly more complicated because the GFP mask is going
to be restricted again shortly after being restored temporarily by the
$subject patch and if no memory allocations happen in the meantime,
nothing bad will happen.  Also, if those memory allocations don't
trigger swapping, nothing bad will happen either.  Now, it is quite
unlikely that anyone will attempt to allocate a lot of memory during
the "prepare" phase of a suspend or power-off transition, so all of
this is not super-worrisome, but I agree that avoiding all of this GFP
mask restrict/restore dance would be better overall.

I'm going to remove the  pm_restrict_gfp_mask() and
pm_restore_gfp_mask() calls from dpm_suspend_start() and
dpm_suspend_end(), respectively, but that's not a change for 6.18.

Thanks!

Re: [PATCH] PM: hibernate: Restore GFP mask in power_down() for HIBERNATION_PLATFORM

[Rafael J. Wysocki]

On Tue, Oct 28, 2025 at 4:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Oct 28, 2025 at 1:56 PM Askar Safin <safinaskar@gmail.com> wrote:
> >
> > On Tue, Oct 28, 2025 at 2:49 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > What kind of damage are you talking about, specifically?
> >
> > Again: "pm_restore_gfp_mask" will restore "gfp_allowed_mask" to its
> > normal value,
> > which will contain __GFP_IO and __GFP_FS. Thus "pm_suspended_storage" will
> > start to return false.
> >
> > But "pm_suspended_storage" is called here:
> > https://elixir.bootlin.com/linux/v6.18-rc3/source/mm/swapfile.c#L1895
> >
> > (Also, please, read that big comment at this link.
> > Well, I have to admit I don't understand it in full.)
> >
> > This check is needed to prevent swapping out pages during hibernation.
>
> It is actually more complicated, but fair enough.
>
> Basically, the concern is that a swap page can be freed and then
> re-used for storing another page of memory after the image has been
> created, so the swap metadata in the image would not match the reality
> any more.
>
> > Call chain is so:
> > swap_writeout -> folio_free_swap -> folio_swapcache_freeable ->
> > pm_suspended_storage
> >
> > So by calling "pm_restore_gfp_mask" we allow pages to be swapped out.
> >
> > But we already wrote hibernation image by that point!
> >
> > So swapping pages will make our swap partition inconsistent.
> >
> > Moreover, as well as I understand, whole reason why we deal with GFP mask
> > in hibernation code is to prevent swapping out pages.
>
> No, it is not the whole reason.  It is also done to avoid accessing
> swap devices while they may not be (fully) operational.
>
> > We restrict GFP before creating hibernation image here:
> > https://elixir.bootlin.com/linux/v6.18-rc3/source/kernel/power/hibernate.c#L463
> > .
> >
> > We do this (as well as I understand) to prevent pages from swapping out.
> >
> > And, starting from that moment, as well as I understand, we should
> > not restore GFP mask until either:
> > - we resume
> > - hibernation will abort for some reason (for example, "wake up event
> > detected during hibernation")
>
> Again, this is slightly more complicated because the GFP mask is going
> to be restricted again shortly after being restored temporarily by the
> $subject patch and if no memory allocations happen in the meantime,
> nothing bad will happen.  Also, if those memory allocations don't
> trigger swapping, nothing bad will happen either.  Now, it is quite
> unlikely that anyone will attempt to allocate a lot of memory during
> the "prepare" phase of a suspend or power-off transition, so all of
> this is not super-worrisome, but I agree that avoiding all of this GFP
> mask restrict/restore dance would be better overall.
>
> I'm going to remove the  pm_restrict_gfp_mask() and
> pm_restore_gfp_mask() calls from dpm_suspend_start() and
> dpm_suspend_end(), respectively, but that's not a change for 6.18.

And I've decided to do something else:

https://lore.kernel.org/linux-pm/5935682.DvuYhMxLoT@rafael.j.wysocki/

because having those calls in dpm_suspend_start() and
dpm_suspend_end(), respectively, makes sense in general as the GFP
mask needs to be restricted after the "prepare" phase of suspend.