Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

DISCUSSION Domenico Andreoli 3 participants 11 messages Feb 13, 2020 – Mar 17, 2020

Subsystems (6)

Memory management VFS Page cache Suspend to RAM +2 more subsystems

lore

Share & tools

lore

Domenico Andreoli

author

2020-02-13 17:23 UTC #0 lore raw

Hi,

at some point between 5.2 and 5.3 my laptop started to refuse
hibernating and come back to a full functional state. It's fully 100% reproducible, no oopses or any other damage to the state seems to happen.

It took me a while to follow the trail down to this commit. If I revert it from v5.6-rc1, the hibernation is back as in the old times.

commit e6bc9de714972cac34daa1dc1567ee48a47a9342
Merge: b6c0d3577246 dc617f29dbe5
Author: Linus Torvalds <[email protected]>
Date: Wed Sep 18 17:35:20 2019 -0700

Merge tag 'vfs-5.4-merge-1' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux

Pull swap access updates from Darrick Wong:
"Prohibit writing to active swap files and swap partitions.

There's no non-malicious use case for allowing userspace to scribble on storage that the kernel thinks it owns"

* tag 'vfs-5.4-merge-1' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux:
vfs: don't allow writes to swap files
mm: set S_SWAPFILE on blockdev swap devices

Is it possible to do anything?

Regards,
Domenico

Signature · 2 lines

rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Darrick J. Wong

2020-02-13 17:58 UTC #1 lore raw

Quoted 8 lines · Domenico Andreoli

Hi,
at some point between 5.2 and 5.3 my laptop started to refuse
hibernating and come back to a full functional state. It's fully 100% reproducible, no oopses or any other damage to the state seems to happen.
It took me a while to follow the trail down to this commit. If I revert it from v5.6-rc1, the hibernation is back as in the old times.

Hmm, do you know which hibernation mechanism your computer is using?

--D

Quoted 27 lines

commit e6bc9de714972cac34daa1dc1567ee48a47a9342
Merge: b6c0d3577246 dc617f29dbe5
Author: Linus Torvalds <[email protected]>
Date: Wed Sep 18 17:35:20 2019 -0700
Merge tag 'vfs-5.4-merge-1' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux
Pull swap access updates from Darrick Wong:
"Prohibit writing to active swap files and swap partitions.
There's no non-malicious use case for allowing userspace to scribble on storage that the kernel thinks it owns"
* tag 'vfs-5.4-merge-1' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux:
vfs: don't allow writes to swap files
mm: set S_SWAPFILE on blockdev swap devices
Is it possible to do anything?
Regards,
Domenico
Signature · 2 lines
rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Domenico Andreoli

author

2020-02-13 18:35 UTC #2 lore raw

Quoted 7 lines · Darrick J. Wong

Quoted 6 lines · Domenico Andreoli
Hi,
at some point between 5.2 and 5.3 my laptop started to refuse
hibernating and come back to a full functional state. It's fully 100% reproducible, no oopses or any other damage to the state seems to happen.

Quoted 6 lines

Quoted 2 lines
It took me a while to follow the trail down to this commit. If I revert it from v5.6-rc1, the hibernation is back as in the old times.
Hmm, do you know which hibernation mechanism your computer is using?
--D

s2disk/uswsusp. Any other tool I could use as alternative?

Thanks,
Domenico

Signature · 2 lines

rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Darrick J. Wong

2020-02-13 19:34 UTC #3 lore raw

Quoted 8 lines · Domenico Andreoli

Quoted 7 lines · Darrick J. Wong
Quoted 6 lines · Domenico Andreoli
Hi,
at some point between 5.2 and 5.3 my laptop started to refuse
hibernating and come back to a full functional state. It's fully 100% reproducible, no oopses or any other damage to the state seems to happen.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
It took me a while to follow the trail down to this commit. If I revert it from v5.6-rc1, the hibernation is back as in the old times.
Hmm, do you know which hibernation mechanism your computer is using?
--D
s2disk/uswsusp. Any other tool I could use as alternative?

Well ... you could try the in-kernel hibernate (which I think is what 'systemctl hibernate' does), though you'd lose the nifty features of µswsusp.

In the end, though, I'll probably have to revert all those IS_SWAPFILE checks (at least if CONFIG_HIBERNATION=y) since it's not fair to force you to totally reconfigure your hibernation setup.

--D

Quoted 6 lines

Thanks,
Domenico

Signature · 2 lines

rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Darrick J. Wong

2020-02-13 19:41 UTC #4 lore raw

Quoted 18 lines · Darrick J. Wong

Quoted 8 lines · Domenico Andreoli
Quoted 7 lines · Darrick J. Wong
Quoted 6 lines · Domenico Andreoli
Hi,
at some point between 5.2 and 5.3 my laptop started to refuse
hibernating and come back to a full functional state. It's fully 100% reproducible, no oopses or any other damage to the state seems to happen.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
It took me a while to follow the trail down to this commit. If I revert it from v5.6-rc1, the hibernation is back as in the old times.
Hmm, do you know which hibernation mechanism your computer is using?
--D
s2disk/uswsusp. Any other tool I could use as alternative?

Quoted 7 lines

Well ... you could try the in-kernel hibernate (which I think is what 'systemctl hibernate' does), though you'd lose the nifty features of µswsusp.
In the end, though, I'll probably have to revert all those IS_SWAPFILE checks (at least if CONFIG_HIBERNATION=y) since it's not fair to force you to totally reconfigure your hibernation setup.

Also, does the following partial revert fix uswsusp for you? It'll allow the direct writes that uswsusp wants to do, while leaving the rest (mmap writes) in place.

--D

fs/block_dev.c +2 -0

index 69bf2fb6f7cd..077d9fa6b87d 100644
@@ -2001,8 +2001,10 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (bdev_read_only(I_BDEV(bd_inode)))
 		return -EPERM;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(bd_inode))
 		return -ETXTBSY;
+#endif
 
 	if (!iov_iter_count(from))
 		return 0;

mm/filemap.c +2 -0

index 1784478270e1..3df3211abe25 100644
@@ -2920,8 +2920,10 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 	loff_t count;
 	int ret;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(inode))
 		return -ETXTBSY;
+#endif
 
 	if (!iov_iter_count(from))
 		return 0;

Domenico Andreoli

author

2020-02-14 21:15 UTC #5 lore raw

[ added linux-pm ]

Quoted 5 lines · Darrick J. Wong

Quoted 4 lines · Darrick J. Wong
Well ... you could try the in-kernel hibernate (which I think is what 'systemctl hibernate' does), though you'd lose the nifty features of µswsusp.

Indeed 'systemctl hibernate' works perfectly with v5.6-rc1 in my setup.

Quoted 15 lines

Quoted 3 lines
In the end, though, I'll probably have to revert all those IS_SWAPFILE checks (at least if CONFIG_HIBERNATION=y) since it's not fair to force you to totally reconfigure your hibernation setup.
Also, does the following partial revert fix uswsusp for you? It'll allow the direct writes that uswsusp wants to do, while leaving the rest (mmap writes) in place.
--D
fs/block_dev.c +0 -0
index 69bf2fb6f7cd..077d9fa6b87d 100644

fs/block_dev.c ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from) · line 2007

Quoted 7 lines

fs/block_dev.c +2 -0

@@ -2001,8 +2001,10 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (bdev_read_only(I_BDEV(bd_inode)))
 		return -EPERM;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(bd_inode))
 		return -ETXTBSY;
+#endif

This alone is enough to make uswsusp work again.

I propose this alternative:

fs/block_dev.c +2 -1

@@ -2001,7 +2001,8 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
        if (bdev_read_only(I_BDEV(bd_inode)))
                return -EPERM;
 
-       if (IS_SWAPFILE(bd_inode))
+       /* Hibernation might happen via uswsusp, let it write to the swap */
+       if (IS_SWAPFILE(bd_inode) && !IS_ENABLED(CONFIG_HIBERNATION))
                return -ETXTBSY;
 
        if (!iov_iter_count(from))

I looked for a more selective way to enable writes to swap at runtime, so I tried with system_entering_hibernation() but it's not yet armed at the point in which uswsusp wants to write to the swap and therefore it does not work.

fs/block_dev.c +3 -1

@@ -34,6 +34,7 @@
 #include <linux/task_io_accounting_ops.h>
 #include <linux/falloc.h>
 #include <linux/uaccess.h>
+#include <linux/suspend.h>
 #include "internal.h"
 
 struct bdev_inode {

@@ -2001,7 +2002,8 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
        if (bdev_read_only(I_BDEV(bd_inode)))
                return -EPERM;
 
-       if (IS_SWAPFILE(bd_inode))
+       /* Hibernation might happen via uswsusp, let it write to the swap */
+       if (IS_SWAPFILE(bd_inode) && !system_entering_hibernation())
                return -ETXTBSY;
 
        if (!iov_iter_count(from))

Quoted 7 lines

fs/block_dev.c +0 -0

 	if (!iov_iter_count(from))
 		return 0;

mm/filemap.c +0 -0

index 1784478270e1..3df3211abe25 100644

mm/filemap.c inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 2929

Quoted 10 lines

mm/filemap.c +2 -0

@@ -2920,8 +2920,10 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 	loff_t count;
 	int ret;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(inode))
 		return -ETXTBSY;
+#endif
 
 	if (!iov_iter_count(from))
 		return 0;

The above is not needed in my case but I'm not sure it would not be needed in some other configuration of uswsusp.

Dom

Signature · 2 lines

rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Darrick J. Wong

2020-02-19 05:03 UTC #6 lore raw

Quoted 10 lines · Domenico Andreoli

[ added linux-pm ]
Quoted 5 lines · Darrick J. Wong
Quoted 4 lines · Darrick J. Wong
Well ... you could try the in-kernel hibernate (which I think is what 'systemctl hibernate' does), though you'd lose the nifty features of µswsusp.
Indeed 'systemctl hibernate' works perfectly with v5.6-rc1 in my setup.

As I suspected, the in-kernel hibernate can write the memory image to the swap file just fine...

Quoted 30 lines

Quoted 15 lines
Quoted 3 lines
In the end, though, I'll probably have to revert all those IS_SWAPFILE checks (at least if CONFIG_HIBERNATION=y) since it's not fair to force you to totally reconfigure your hibernation setup.
Also, does the following partial revert fix uswsusp for you? It'll allow the direct writes that uswsusp wants to do, while leaving the rest (mmap writes) in place.
--D
fs/block_dev.c +0 -0
index 69bf2fb6f7cd..077d9fa6b87d 100644
fs/block_dev.c ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from) · line 2007
Quoted 7 lines
fs/block_dev.c +2 -0
@@ -2001,8 +2001,10 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (bdev_read_only(I_BDEV(bd_inode)))
 		return -EPERM;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(bd_inode))
 		return -ETXTBSY;
+#endif
This alone is enough to make uswsusp work again.
I propose this alternative:

fs/block_dev.c ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from) · line 2005

Quoted 6 lines

fs/block_dev.c +2 -1

@@ -2001,7 +2001,8 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
        if (bdev_read_only(I_BDEV(bd_inode)))
                return -EPERM;
 
-       if (IS_SWAPFILE(bd_inode))
+       /* Hibernation might happen via uswsusp, let it write to the swap */
+       if (IS_SWAPFILE(bd_inode) && !IS_ENABLED(CONFIG_HIBERNATION))

...but maybe we could do something a little more targeted?

Quoted 8 lines

fs/block_dev.c +0 -0
                return -ETXTBSY;
 
        if (!iov_iter_count(from))
I looked for a more selective way to enable writes to swap at runtime, so I tried with system_entering_hibernation() but it's not yet armed at the point in which uswsusp wants to write to the swap and therefore it does not work.

Hmm. I was poking around in the uswsusp code and I /think/ it prepares the kernel for the userspace-driven hibernation by calling the ioctl SNAPSHOT_SET_SWAP_FILE on the device that it's going to use. If that's true, we could have it clear S_SWAPFILE on the chosen device, though I'd have to do some more digging to figure out how to restore the flag after resuming from hibernation.

--D

Quoted 48 lines

fs/block_dev.c +3 -1

@@ -34,6 +34,7 @@
 #include <linux/task_io_accounting_ops.h>
 #include <linux/falloc.h>
 #include <linux/uaccess.h>
+#include <linux/suspend.h>
 #include "internal.h"
 
 struct bdev_inode {

@@ -2001,7 +2002,8 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
        if (bdev_read_only(I_BDEV(bd_inode)))
                return -EPERM;
 
-       if (IS_SWAPFILE(bd_inode))
+       /* Hibernation might happen via uswsusp, let it write to the swap */
+       if (IS_SWAPFILE(bd_inode) && !system_entering_hibernation())
                return -ETXTBSY;
 
        if (!iov_iter_count(from))

Quoted 8 lines

fs/block_dev.c +0 -0

 	if (!iov_iter_count(from))
 		return 0;

mm/filemap.c +0 -0

index 1784478270e1..3df3211abe25 100644

mm/filemap.c inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 2929

Quoted 10 lines

mm/filemap.c +2 -0

@@ -2920,8 +2920,10 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 	loff_t count;
 	int ret;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(inode))
 		return -ETXTBSY;
+#endif
 
 	if (!iov_iter_count(from))
 		return 0;

The above is not needed in my case but I'm not sure it would not be needed in some other configuration of uswsusp.

Dom

Signature · 2 lines

rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Darrick J. Wong

2020-02-22 00:23 UTC #7 lore raw

In reply to Domenico Andreoli

Quoted 92 lines · Domenico Andreoli

[ added linux-pm ]
Quoted 5 lines · Darrick J. Wong
Quoted 4 lines · Darrick J. Wong
Well ... you could try the in-kernel hibernate (which I think is what 'systemctl hibernate' does), though you'd lose the nifty features of µswsusp.
Indeed 'systemctl hibernate' works perfectly with v5.6-rc1 in my setup.
Quoted 15 lines
Quoted 3 lines
In the end, though, I'll probably have to revert all those IS_SWAPFILE checks (at least if CONFIG_HIBERNATION=y) since it's not fair to force you to totally reconfigure your hibernation setup.
Also, does the following partial revert fix uswsusp for you? It'll allow the direct writes that uswsusp wants to do, while leaving the rest (mmap writes) in place.
--D
fs/block_dev.c +0 -0
index 69bf2fb6f7cd..077d9fa6b87d 100644
fs/block_dev.c ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from) · line 2007
Quoted 7 lines
fs/block_dev.c +2 -0
@@ -2001,8 +2001,10 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (bdev_read_only(I_BDEV(bd_inode)))
 		return -EPERM;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(bd_inode))
 		return -ETXTBSY;
+#endif
This alone is enough to make uswsusp work again.
I propose this alternative:
fs/block_dev.c +2 -1
@@ -2001,7 +2001,8 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
        if (bdev_read_only(I_BDEV(bd_inode)))
                return -EPERM;
 
-       if (IS_SWAPFILE(bd_inode))
+       /* Hibernation might happen via uswsusp, let it write to the swap */
+       if (IS_SWAPFILE(bd_inode) && !IS_ENABLED(CONFIG_HIBERNATION))
                return -ETXTBSY;
 
        if (!iov_iter_count(from))
I looked for a more selective way to enable writes to swap at runtime, so I tried with system_entering_hibernation() but it's not yet armed at the point in which uswsusp wants to write to the swap and therefore it does not work.
fs/block_dev.c +3 -1
@@ -34,6 +34,7 @@
 #include <linux/task_io_accounting_ops.h>
 #include <linux/falloc.h>
 #include <linux/uaccess.h>
+#include <linux/suspend.h>
 #include "internal.h"
 
 struct bdev_inode {
@@ -2001,7 +2002,8 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
        if (bdev_read_only(I_BDEV(bd_inode)))
                return -EPERM;
 
-       if (IS_SWAPFILE(bd_inode))
+       /* Hibernation might happen via uswsusp, let it write to the swap */
+       if (IS_SWAPFILE(bd_inode) && !system_entering_hibernation())
                return -ETXTBSY;
 
        if (!iov_iter_count(from))
Quoted 16 lines
fs/block_dev.c +0 -0
 	if (!iov_iter_count(from))
 		return 0;
mm/filemap.c +2 -0
index 1784478270e1..3df3211abe25 100644
@@ -2920,8 +2920,10 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 	loff_t count;
 	int ret;
 
+#ifndef CONFIG_HIBERNATION
 	if (IS_SWAPFILE(inode))
 		return -ETXTBSY;
+#endif
 

Quoted 5 lines

mm/filemap.c inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 2929
Quoted 2 lines
mm/filemap.c +0 -0
 	if (!iov_iter_count(from))
 		return 0;
The above is not needed in my case but I'm not sure it would not be needed in some other configuration of uswsusp.

Ok, third try. Does the following work? This is a little more selective in that it only disables the write protection on the swap device/file that uswusp is going to write to.

--D

kernel/power/user.c +8 -1

index 77438954cc2b..a3ae9cbbfcf0 100644
@@ -372,10 +372,17 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			 */
 			swdev = new_decode_dev(swap_area.dev);
 			if (swdev) {
+				struct block_device *bd;
+
 				offset = swap_area.offset;
-				data->swap = swap_type_of(swdev, offset, NULL);
+				data->swap = swap_type_of(swdev, offset, &bd);
 				if (data->swap < 0)
 					error = -ENODEV;
+
+				inode_lock(bd->bd_inode);
+				bd->bd_inode->i_flags &= ~S_SWAPFILE;
+				inode_unlock(bd->bd_inode);
+				bdput(bd);
 			} else {
 				data->swap = -1;
 				error = -EINVAL;

Domenico Andreoli

author

2020-02-23 19:03 UTC #8 lore raw

Quoted 4 lines · Darrick J. Wong

Ok, third try. Does the following work? This is a little more selective in that it only disables the write protection on the swap device/file that uswusp is going to write to.

Yes it works but also verified that once the S_SWAPFILE bit is cleared it's never restored, therefore the protecton is gone after the first hibernation.

Quoted 26 lines

--D

kernel/power/user.c +8 -1

index 77438954cc2b..a3ae9cbbfcf0 100644
@@ -372,10 +372,17 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			 */
 			swdev = new_decode_dev(swap_area.dev);
 			if (swdev) {
+				struct block_device *bd;
+
 				offset = swap_area.offset;
-				data->swap = swap_type_of(swdev, offset, NULL);
+				data->swap = swap_type_of(swdev, offset, &bd);
 				if (data->swap < 0)
 					error = -ENODEV;
+
+				inode_lock(bd->bd_inode);
+				bd->bd_inode->i_flags &= ~S_SWAPFILE;
+				inode_unlock(bd->bd_inode);
+				bdput(bd);
 			} else {
 				data->swap = -1;
 				error = -EINVAL;

Signature · 2 lines

rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Darrick J. Wong

2020-02-25 20:26 UTC #9 lore raw

Quoted 9 lines · Domenico Andreoli

Quoted 4 lines · Darrick J. Wong
Ok, third try. Does the following work? This is a little more selective in that it only disables the write protection on the swap device/file that uswusp is going to write to.
Yes it works but also verified that once the S_SWAPFILE bit is cleared it's never restored, therefore the protecton is gone after the first hibernation.

Ok, good. Now can you try the third part, which ought to re-apply S_SWAPFILE after a successful resume, please? Assuming this works, I think we're ready with a fixpatch.

--D

include/linux/swap.h +1 -0

index 1e99f7ac1d7e..add93e205850 100644
@@ -458,6 +458,7 @@ extern void swap_free(swp_entry_t);
 extern void swapcache_free_entries(swp_entry_t *entries, int n);
 extern int free_swap_and_cache(swp_entry_t);
 extern int swap_type_of(dev_t, sector_t, struct block_device **);
+extern void swap_relockall(void);
 extern unsigned int count_swap_pages(int, int);
 extern sector_t map_swap_page(struct page *, struct block_device **);
 extern sector_t swapdev_block(int, pgoff_t);

kernel/power/user.c +10 -1

index 77438954cc2b..b11f7037ce5e 100644
@@ -271,6 +271,8 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			break;
 		}
 		error = hibernation_restore(data->platform_support);
+		if (!error)
+			swap_relockall();
 		break;
 
 	case SNAPSHOT_FREE:

@@ -372,10 +374,17 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			 */
 			swdev = new_decode_dev(swap_area.dev);
 			if (swdev) {
+				struct block_device *bd;
+
 				offset = swap_area.offset;
-				data->swap = swap_type_of(swdev, offset, NULL);
+				data->swap = swap_type_of(swdev, offset, &bd);
 				if (data->swap < 0)
 					error = -ENODEV;
+
+				inode_lock(bd->bd_inode);
+				bd->bd_inode->i_flags &= ~S_SWAPFILE;
+				inode_unlock(bd->bd_inode);
+				bdput(bd);
 			} else {
 				data->swap = -1;
 				error = -EINVAL;

mm/swapfile.c +26 -0

index b2a2e45c9a36..a64dcba10db6 100644
@@ -1799,6 +1799,32 @@ int swap_type_of(dev_t device, sector_t offset, struct block_device **bdev_p)
 	return -ENODEV;
 }
 
+/* Re-lock swap devices after resuming from userspace suspend. */
+void swap_relockall(void)
+{
+	int type;
+
+	spin_lock(&swap_lock);
+	for (type = 0; type < nr_swapfiles; type++) {
+		struct swap_info_struct *sis = swap_info[type];
+		struct block_device *bdev = bdgrab(sis->bdev);
+
+		/*
+		 * uswsusp only knows how to suspend to block devices, so we
+		 * can skip swap files.
+		 */
+		if (!(sis->flags & SWP_WRITEOK) ||
+		    !(sis->flags & SWP_BLKDEV))
+			continue;
+
+		inode_lock(bd->bd_inode);
+		bd->bd_inode->i_flags |= S_SWAPFILE;
+		inode_unlock(bd->bd_inode);
+		bdput(bd);
+	}
+	spin_unlock(&swap_lock);
+}
+
 /*
  * Get the (PAGE_SIZE) block corresponding to given offset on the swapdev
  * corresponding to given index in swap_info (swap type).

Brad Campbell

Tested-by

2020-03-17 16:16 UTC #10 lore raw

Quoted 7 lines · Darrick J. Wong

Quoted 6 lines · Domenico Andreoli
Quoted 4 lines · Darrick J. Wong
Ok, third try. Does the following work? This is a little more selective in that it only disables the write protection on the swap device/file that uswusp is going to write to.

Quoted 8 lines

Quoted 3 lines
Yes it works but also verified that once the S_SWAPFILE bit is cleared it's never restored, therefore the protecton is gone after the first hibernation.
Ok, good. Now can you try the third part, which ought to re-apply S_SWAPFILE after a successful resume, please? Assuming this works, I think we're ready with a fixpatch.

I just bumped up against it upgrading from 5.2 to 5.5 & a long bisection results in apparently the same point :

# first bad commit: [dc617f29dbe5ef0c8ced65ce62c464af1daaab3d] vfs: don't allow writes to swap files

Tested-by Brad Campbell <[email protected]>

Quoted 84 lines

--D

include/linux/swap.h +1 -0

index 1e99f7ac1d7e..add93e205850 100644
@@ -458,6 +458,7 @@ extern void swap_free(swp_entry_t);
  extern void swapcache_free_entries(swp_entry_t *entries, int n);
  extern int free_swap_and_cache(swp_entry_t);
  extern int swap_type_of(dev_t, sector_t, struct block_device **);
+extern void swap_relockall(void);
  extern unsigned int count_swap_pages(int, int);
  extern sector_t map_swap_page(struct page *, struct block_device **);
  extern sector_t swapdev_block(int, pgoff_t);

kernel/power/user.c +10 -1

index 77438954cc2b..b11f7037ce5e 100644
@@ -271,6 +271,8 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
  			break;
  		}
  		error = hibernation_restore(data->platform_support);
+		if (!error)
+			swap_relockall();
  		break;
 
  	case SNAPSHOT_FREE:

@@ -372,10 +374,17 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
  			 */
  			swdev = new_decode_dev(swap_area.dev);
  			if (swdev) {
+				struct block_device *bd;
+
  				offset = swap_area.offset;
-				data->swap = swap_type_of(swdev, offset, NULL);
+				data->swap = swap_type_of(swdev, offset, &bd);
  				if (data->swap < 0)
  					error = -ENODEV;
+
+				inode_lock(bd->bd_inode);
+				bd->bd_inode->i_flags &= ~S_SWAPFILE;
+				inode_unlock(bd->bd_inode);
+				bdput(bd);
  			} else {
  				data->swap = -1;
  				error = -EINVAL;

mm/swapfile.c +26 -0

index b2a2e45c9a36..a64dcba10db6 100644
@@ -1799,6 +1799,32 @@ int swap_type_of(dev_t device, sector_t offset, struct block_device **bdev_p)
  	return -ENODEV;
  }
 
+/* Re-lock swap devices after resuming from userspace suspend. */
+void swap_relockall(void)
+{
+	int type;
+
+	spin_lock(&swap_lock);
+	for (type = 0; type < nr_swapfiles; type++) {
+		struct swap_info_struct *sis = swap_info[type];
+		struct block_device *bdev = bdgrab(sis->bdev);
+
+		/*
+		 * uswsusp only knows how to suspend to block devices, so we
+		 * can skip swap files.
+		 */
+		if (!(sis->flags & SWP_WRITEOK) ||
+		    !(sis->flags & SWP_BLKDEV))
+			continue;
+
+		inode_lock(bd->bd_inode);
+		bd->bd_inode->i_flags |= S_SWAPFILE;
+		inode_unlock(bd->bd_inode);
+		bdput(bd);
+	}
+	spin_unlock(&swap_lock);
+}
+
  /*
   * Get the (PAGE_SIZE) block corresponding to given offset on the swapdev
   * corresponding to given index in swap_info (swap type).