Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[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.


commit e6bc9de714972cac34daa1dc1567ee48a47a9342
Merge: b6c0d3577246 dc617f29dbe5
Author: Linus Torvalds <torvalds@linux-foundation.org>
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

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

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Darrick J. Wong]

On Thu, Feb 13, 2020 at 06:23:51PM +0100, Domenico Andreoli wrote:
> 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

> 
> commit e6bc9de714972cac34daa1dc1567ee48a47a9342
> Merge: b6c0d3577246 dc617f29dbe5
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> 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
> 
> -- 
> rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
> ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Domenico Andreoli]

On Thu, Feb 13, 2020 at 09:57:53AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 13, 2020 at 06:23:51PM +0100, Domenico Andreoli wrote:
> > 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

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

Thanks,
Domenico

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

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Darrick J. Wong]

On Thu, Feb 13, 2020 at 07:35:15PM +0100, Domenico Andreoli wrote:
> On Thu, Feb 13, 2020 at 09:57:53AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 13, 2020 at 06:23:51PM +0100, Domenico Andreoli wrote:
> > > 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
> 
> 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

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

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Darrick J. Wong]

On Thu, Feb 13, 2020 at 11:34:10AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 13, 2020 at 07:35:15PM +0100, Domenico Andreoli wrote:
> > On Thu, Feb 13, 2020 at 09:57:53AM -0800, Darrick J. Wong wrote:
> > > On Thu, Feb 13, 2020 at 06:23:51PM +0100, Domenico Andreoli wrote:
> > > > 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
> > 
> > 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.

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

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 69bf2fb6f7cd..077d9fa6b87d 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -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;
diff --git a/mm/filemap.c b/mm/filemap.c
index 1784478270e1..3df3211abe25 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -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;

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Domenico Andreoli]

[ added linux-pm ]

On Thu, Feb 13, 2020 at 11:41:35AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 13, 2020 at 11:34:10AM -0800, Darrick J. Wong wrote:
> > 
> > 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.

> > 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
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 69bf2fb6f7cd..077d9fa6b87d 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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:

--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -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.

--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -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))

>  	if (!iov_iter_count(from))
>  		return 0;
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1784478270e1..3df3211abe25 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -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

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

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Darrick J. Wong]

On Fri, Feb 14, 2020 at 10:15:24PM +0100, Domenico Andreoli wrote:
> [ added linux-pm ]
> 
> On Thu, Feb 13, 2020 at 11:41:35AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 13, 2020 at 11:34:10AM -0800, Darrick J. Wong wrote:
> > > 
> > > 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...

> > > 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
> > 
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 69bf2fb6f7cd..077d9fa6b87d 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -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:
> 
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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?

>                 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

> 
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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))
> 
> >  	if (!iov_iter_count(from))
> >  		return 0;
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 1784478270e1..3df3211abe25 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -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
> 
> -- 
> rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
> ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Darrick J. Wong]

On Fri, Feb 14, 2020 at 10:15:24PM +0100, Domenico Andreoli wrote:
> [ added linux-pm ]
> 
> On Thu, Feb 13, 2020 at 11:41:35AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 13, 2020 at 11:34:10AM -0800, Darrick J. Wong wrote:
> > > 
> > > 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.
> 
> > > 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
> > 
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 69bf2fb6f7cd..077d9fa6b87d 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -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:
> 
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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.
> 
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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))
> 
> >  	if (!iov_iter_count(from))
> >  		return 0;
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 1784478270e1..3df3211abe25 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -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.

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

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;

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Domenico Andreoli]

On Fri, Feb 21, 2020 at 04:23:19PM -0800, Darrick J. Wong wrote:
> 
> 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.

> 
> --D
> 
> 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;

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

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Darrick J. Wong]

On Sun, Feb 23, 2020 at 08:03:11PM +0100, Domenico Andreoli wrote:
> On Fri, Feb 21, 2020 at 04:23:19PM -0800, Darrick J. Wong wrote:
> > 
> > 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

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/user.c b/kernel/power/user.c
index 77438954cc2b..b11f7037ce5e 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -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;
diff --git a/mm/swapfile.c b/mm/swapfile.c
index b2a2e45c9a36..a64dcba10db6 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(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).

Re: Regression: hibernation is broken since e6bc9de714972cac34daa1dc1567ee48a47a9342

[Brad Campbell]

On 26/2/20 4:26 am, Darrick J. Wong wrote:
> On Sun, Feb 23, 2020 at 08:03:11PM +0100, Domenico Andreoli wrote:
>> On Fri, Feb 21, 2020 at 04:23:19PM -0800, Darrick J. Wong wrote:
>>>
>>> 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.
> 

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 <lists2009@fnarfbargle.com>

> --D
> 
> 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/user.c b/kernel/power/user.c
> index 77438954cc2b..b11f7037ce5e 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -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;
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index b2a2e45c9a36..a64dcba10db6 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(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).
>