[PATCH] PM: hibernate: Drain trailing zero pages on userspace restore

[PATCH] PM: hibernate: Drain trailing zero pages on userspace restore

[Alberto Garcia]

Commit 005e8dddd497 ("PM: hibernate: don't store zero pages in the
image file") added an optimization to skip zero-filled pages in the
hibernation image. On restore, zero pages are handled internally by
snapshot_write_next() in a loop that processes them without returning
to the caller.

With the userspace restore interface, writing the last non-zero page
to /dev/snapshot is followed by the SNAPSHOT_ATOMIC_RESTORE ioctl. At
this point there are no more calls to snapshot_write_next() so any
trailing zero pages are not processed, snapshot_image_loaded() fails
because handle->cur is smaller than expected, the ioctl returns -EPERM
and the image is not restored.

The in-kernel restore path is not affected by this because the loop in
load_image() in swap.c calls snapshot_write_next() until it returns 0.
It is this final call that drains any trailing zero pages.

Fixed by calling snapshot_write_next() in the SNAPSHOT_ATOMIC_RESTORE
handler before snapshot_write_finalize(), giving the kernel the chance
to process any trailing zero pages.

Fixes: 005e8dddd497 ("PM: hibernate: don't store zero pages in the image file")
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 kernel/power/user.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 4401cfe26e5c..d5f189c4c93e 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -319,6 +319,14 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		break;
 
 	case SNAPSHOT_ATOMIC_RESTORE:
+		/*
+		 * We need to call snapshot_write_next() one last time
+		 * before finalizing in order to process any trailing
+		 * zero pages.
+		 */
+		error = snapshot_write_next(&data->handle);
+		if (error < 0)
+			break;
 		error = snapshot_write_finalize(&data->handle);
 		if (error)
 			break;
-- 
2.47.3

Re: [PATCH] PM: hibernate: Drain trailing zero pages on userspace restore

[Alberto Garcia]

On Mon, Mar 09, 2026 at 01:12:50AM +0100, Alberto Garcia wrote:
>  	case SNAPSHOT_ATOMIC_RESTORE:
> +		/*
> +		 * We need to call snapshot_write_next() one last time
> +		 * before finalizing in order to process any trailing
> +		 * zero pages.
> +		 */
> +		error = snapshot_write_next(&data->handle);
> +		if (error < 0)
> +			break;

I realized that this patch assumes that userspace calls
SNAPSHOT_ATOMIC_RESTORE only after having written the full image,
but if that happens earlier I'm not sure that it's safe to call
snapshot_write_next().

An alternative solution would be to do it at the beginning of
snapshot_write_finalize() if handle->cur > nr_meta_pages + 1.

If you think that's a better idea I can send v2 of the patch.

Regards,

Berto

Re: [PATCH] PM: hibernate: Drain trailing zero pages on userspace restore

[Brian Geffon]

On Mon, Mar 9, 2026 at 7:26 AM Alberto Garcia <berto@igalia.com> wrote:
>
> On Mon, Mar 09, 2026 at 01:12:50AM +0100, Alberto Garcia wrote:
> >       case SNAPSHOT_ATOMIC_RESTORE:
> > +             /*
> > +              * We need to call snapshot_write_next() one last time
> > +              * before finalizing in order to process any trailing
> > +              * zero pages.
> > +              */
> > +             error = snapshot_write_next(&data->handle);
> > +             if (error < 0)
> > +                     break;
>
> I realized that this patch assumes that userspace calls
> SNAPSHOT_ATOMIC_RESTORE only after having written the full image,
> but if that happens earlier I'm not sure that it's safe to call
> snapshot_write_next().
>
> An alternative solution would be to do it at the beginning of
> snapshot_write_finalize() if handle->cur > nr_meta_pages + 1.

I think this makes sense, but if snapshot_write_next() returns
PAGE_SIZE then we need to indicate some error (eg. -ENODATA) in that
the kernel was still expecting another copy page.

Also it appears that the kernel restore path and the userspace restore
path return two different error codes for !snapshot_image_loaded(),
the former does -ENODATA and the latter -EPERM.

>
> If you think that's a better idea I can send v2 of the patch.
>
> Regards,
>
> Berto

Re: [PATCH] PM: hibernate: Drain trailing zero pages on userspace restore

[Alberto Garcia]

On Mon, Mar 09, 2026 at 11:01:15AM -0400, Brian Geffon wrote:
> > An alternative solution would be to do it at the beginning of
> > snapshot_write_finalize() if handle->cur > nr_meta_pages + 1.
> 
> I think this makes sense, but if snapshot_write_next() returns
> PAGE_SIZE then we need to indicate some error (eg. -ENODATA) in that
> the kernel was still expecting another copy page.

This would fail anyway in snapshot_image_loaded() because of this
condition:

   handle->cur <= nr_meta_pages + nr_copy_pages + nr_zero_pages

But I think it make sense to return an error directly from
snapshot_write_finalize() as you suggest, I can use -ENODATA.

> Also it appears that the kernel restore path and the userspace restore
> path return two different error codes for !snapshot_image_loaded(),
> the former does -ENODATA and the latter -EPERM.

Yes, -EPERM is the one that I hit in my tests.

Does this need to be corrected?

Berto

Re: [PATCH] PM: hibernate: Drain trailing zero pages on userspace restore

[Brian Geffon]

On Mon, Mar 9, 2026 at 11:57 AM Alberto Garcia <berto@igalia.com> wrote:
>
> On Mon, Mar 09, 2026 at 11:01:15AM -0400, Brian Geffon wrote:
> > > An alternative solution would be to do it at the beginning of
> > > snapshot_write_finalize() if handle->cur > nr_meta_pages + 1.
> >
> > I think this makes sense, but if snapshot_write_next() returns
> > PAGE_SIZE then we need to indicate some error (eg. -ENODATA) in that
> > the kernel was still expecting another copy page.
>
> This would fail anyway in snapshot_image_loaded() because of this
> condition:
>
>    handle->cur <= nr_meta_pages + nr_copy_pages + nr_zero_pages

A subtle bug exists where if you had exactly 1 page remaining (which
was to be copied) handle->cur would have been incremented in
snapshot_write_next(), however, no data would have been copied into
that page.

>
> But I think it make sense to return an error directly from
> snapshot_write_finalize() as you suggest, I can use -ENODATA.

Yes, I think the safest option is if you call
snapshot_write_finalize() and snapshot_write_next() returns a positive
value you return -ENODATA.

>
> > Also it appears that the kernel restore path and the userspace restore
> > path return two different error codes for !snapshot_image_loaded(),
> > the former does -ENODATA and the latter -EPERM.
>
> Yes, -EPERM is the one that I hit in my tests.
>
> Does this need to be corrected?

Yes, I think we should change that code to check if
!snapshot_image_loaded() and explicitly return -ENODATA rather than
-EPERM which is meant for cases where the operation is neither
write-only nor ready. This new explicit check should happen after
that.

>
> Berto