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?
