On Mon, May 25, 2020 at 12:52:17PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 19, 2020 at 8:14 PM Domenico Andreoli
> <domenico.andreoli@xxxxxxxxx> wrote:
> >
> > From: Domenico Andreoli <domenico.andreoli@xxxxxxxxx>
> >
> > Hibernation via snapshot device requires write permission to the swap
> > block device, the one that more often (but not necessarily) is used to
> > store the hibernation image.
> >
> > With this patch, such permissions are granted iff:
> >
> > 1) snapshot device config option is enabled
> > 2) swap partition is used as resume device
> >
> > In other circumstances the swap device is not writable from userspace.
> >
> > In order to achieve this, every write attempt to a swap device is
> > checked against the device configured as part of the uswsusp API [0]
> > using a pointer to the inode struct in memory. If the swap device being
> > written was not configured for resuming, the write request is denied.
> >
> > NOTE: this implementation works only for swap block devices, where the
> > inode configured by swapon (which sets S_SWAPFILE) is the same used
> > by SNAPSHOT_SET_SWAP_AREA.
> >
> > In case of swap file, SNAPSHOT_SET_SWAP_AREA indeed receives the inode
> > of the block device containing the filesystem where the swap file is
> > located (+ offset in it) which is never passed to swapon and then has
> > not set S_SWAPFILE.
> >
> > As result, the swap file itself (as a file) has never an option to be
> > written from userspace. Instead it remains writable if accessed directly
> > from the containing block device, which is always writeable from root.
> >
> > [0] Documentation/power/userland-swsusp.rst
> >
> > v2:
> > - rename is_hibernate_snapshot_dev() to is_hibernate_resume_dev()
> > - fix description so to correctly refer to the resume device
> >
> > Signed-off-by: Domenico Andreoli <domenico.andreoli@xxxxxxxxx>
> > Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> > Cc: Pavel Machek <pavel@xxxxxx>
> > Cc: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: viro@xxxxxxxxxxxxxxxxxx
> > Cc: tytso@xxxxxxx
> > Cc: len.brown@xxxxxxxxx
> > Cc: linux-pm@xxxxxxxxxxxxxxx
> > Cc: linux-mm@xxxxxxxxx
> > Cc: linux-xfs@xxxxxxxxxxxxxxx
> > Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> >
> > ---
> > fs/block_dev.c | 3 +--
> > include/linux/suspend.h | 6 ++++++
> > kernel/power/user.c | 14 +++++++++++++-
> > 3 files changed, 20 insertions(+), 3 deletions(-)
> >
> > Index: b/include/linux/suspend.h
> > ===================================================================
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -466,6 +466,12 @@ static inline bool system_entering_hiber
> > static inline bool hibernation_available(void) { return false; }
> > #endif /* CONFIG_HIBERNATION */
> >
> > +#ifdef CONFIG_HIBERNATION_SNAPSHOT_DEV
> > +int is_hibernate_resume_dev(const struct inode *);
> > +#else
> > +static inline int is_hibernate_resume_dev(const struct inode *i) { return 0; }
> > +#endif
> > +
> > /* Hibernation and suspend events */
> > #define PM_HIBERNATION_PREPARE 0x0001 /* Going to hibernate */
> > #define PM_POST_HIBERNATION 0x0002 /* Hibernation finished */
> > Index: b/kernel/power/user.c
> > ===================================================================
> > --- a/kernel/power/user.c
> > +++ b/kernel/power/user.c
> > @@ -35,8 +35,14 @@ static struct snapshot_data {
> > bool ready;
> > bool platform_support;
> > bool free_bitmaps;
> > + struct inode *bd_inode;
> > } snapshot_state;
> >
> > +int is_hibernate_resume_dev(const struct inode *bd_inode)
> > +{
> > + return hibernation_available() && snapshot_state.bd_inode == bd_inode;
> > +}
> > +
> > static int snapshot_open(struct inode *inode, struct file *filp)
> > {
> > struct snapshot_data *data;
> > @@ -95,6 +101,7 @@ static int snapshot_open(struct inode *i
> > data->frozen = false;
> > data->ready = false;
> > data->platform_support = false;
> > + data->bd_inode = NULL;
> >
> > Unlock:
> > unlock_system_sleep();
> > @@ -110,6 +117,7 @@ static int snapshot_release(struct inode
> >
> > swsusp_free();
> > data = filp->private_data;
> > + data->bd_inode = NULL;
> > free_all_swap_pages(data->swap);
> > if (data->frozen) {
> > pm_restore_gfp_mask();
> > @@ -202,6 +210,7 @@ struct compat_resume_swap_area {
> > static int snapshot_set_swap_area(struct snapshot_data *data,
> > void __user *argp)
> > {
> > + struct block_device *bdev;
> > sector_t offset;
> > dev_t swdev;
> >
> > @@ -232,9 +241,12 @@ static int snapshot_set_swap_area(struct
> > data->swap = -1;
> > return -EINVAL;
> > }
> > - data->swap = swap_type_of(swdev, offset, NULL);
> > + data->swap = swap_type_of(swdev, offset, &bdev);
> > if (data->swap < 0)
> > return -ENODEV;
> > +
> > + data->bd_inode = bdev->bd_inode;
> > + bdput(bdev);
> > return 0;
> > }
> >
> > Index: b/fs/block_dev.c
> > ===================================================================
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -2023,8 +2023,7 @@ ssize_t blkdev_write_iter(struct kiocb *
> > if (bdev_read_only(I_BDEV(bd_inode)))
> > return -EPERM;
> >
> > - /* uswsusp needs write permission to the swap */
> > - if (IS_SWAPFILE(bd_inode) && !hibernation_available())
> > + if (IS_SWAPFILE(bd_inode) && !is_hibernate_resume_dev(bd_inode))
> > return -ETXTBSY;
> >
> > if (!iov_iter_count(from))
> >
> > --
>
> The patch looks OK to me.
>
> Darrick, what do you think?
Looks fine to me too.
I kinda wonder how uswsusp prevents the bdev from being swapoff'd (or
just plain disappearing) such that bd_inode will never point to a
recycled inode, but I guess since we're only comparing pointer values
it's not a big deal for this patch...
Acked-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
