On Sat, Feb 29, 2020 at 07:07:16PM +0100, Domenico Andreoli wrote:
> On Sat, Feb 29, 2020 at 09:08:25AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > It turns out that there /is/ one use case for programs being able to
> > write to swap devices, and that is the userspace hibernation code. The
> > uswsusp ioctls allow userspace to lease parts of swap devices, so turn
> > S_SWAPFILE off when invoking suspend.
> >
> > Fixes: 1638045c3677 ("mm: set S_SWAPFILE on blockdev swap devices")
> > Reported-by: Domenico Andreoli <domenico.andreoli@xxxxxxxxx>
> > Reported-by: Marian Klein <mkleinsoft@xxxxxxxxx>
>
> I also tested it yesterday but was not satisfied, unfortunately I did
> not come with my comment in time.
>
> Yes, I confirm that the uswsusp works again but also checked that
> swap_relockall() is not triggered at all and therefore after the first
> hibernation cycle the S_SWAPFILE bit remains cleared and the whole
> swap_relockall() is useless.
>
> I'm not sure this patch should be merged in the current form.
NNGGHHGGHGH /me is rapidly losing his sanity and will soon just revert
the whole security feature because I'm getting fed up with people
yelling at me *while I'm on vacation* trying to *restore* my sanity. I
really don't want to be QAing userspace-directed hibernation right now.
...right, the patch is broken because we have to relock the swapfiles in
whatever code executes after we jump back to the restored kernel, not in
the one that's doing the restoring. Does this help?
OTOH, maybe we should just leave the swapfiles unlocked after resume.
Userspace has clearly demonstrated the one usecase for writing to the
swapfile, which means anyone could have jumped in while uswsusp was
running and written whatever crap they wanted to the parts of the swap
file that weren't leased for the hibernate image.
--D
From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Subject: [PATCH] hibernate: unlock swap bdev for writing when uswsusp is active
It turns out that there /is/ one use case for programs being able to
write to swap devices, and that is the userspace hibernation code. The
uswsusp ioctls allow userspace to lease parts of swap devices, so turn
S_SWAPFILE off when invoking suspend.
Fixes: 1638045c3677 ("mm: set S_SWAPFILE on blockdev swap devices")
Reported-by: Domenico Andreoli <domenico.andreoli@xxxxxxxxx>
Reported-by: Marian Klein <mkleinsoft@xxxxxxxxx>
Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
---
include/linux/swap.h | 1 +
kernel/power/hibernate.c | 4 ++++
kernel/power/user.c | 9 ++++++++-
mm/swapfile.c | 26 ++++++++++++++++++++++++++
4 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 1e99f7ac1d7e..add93e205850 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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);
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 6dbeedb7354c..aa5a6701614d 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -31,6 +31,7 @@
#include <linux/genhd.h>
#include <linux/ktime.h>
#include <linux/security.h>
+#include <linux/swap.h>
#include <trace/events/power.h>
#include "power.h"
@@ -399,6 +400,9 @@ int hibernation_snapshot(int platform_mode)
* image creation has failed and (2) after a successful restore.
*/
+ /* Lock the swap files, just in case uswsusp was active. */
+ swap_relockall();
+
/* We may need to release the preallocated image pages here. */
if (error || !in_suspend)
swsusp_free();
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 77438954cc2b..a3ae9cbbfcf0 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -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;
diff --git a/mm/swapfile.c b/mm/swapfile.c
index b2a2e45c9a36..439bfb7263d3 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -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(bdev->bd_inode);
+ bdev->bd_inode->i_flags |= S_SWAPFILE;
+ inode_unlock(bdev->bd_inode);
+ bdput(bdev);
+ }
+ 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).
