[PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

The system refused to do a test_resume because it found that the
swap device has already been taken by someone else. Specificly,
the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
do this check.

Steps to reproduce:
 dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo | 
       awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
 mkswap /swapfile
 swapon /swapfile
 swap-offset /swapfile
 echo 34816 > /sys/power/resume_offset
 echo test_resume > /sys/power/disk
 echo disk > /sys/power/state

 PM: Using 3 thread(s) for compression
 PM: Compressing and saving image data (293150 pages)...
 PM: Image saving progress:   0%
 PM: Image saving progress:  10%
 ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
 ata1.00: configured for UDMA/100
 ata2: SATA link down (SStatus 0 SControl 300)
 ata5: SATA link down (SStatus 0 SControl 300)
 ata6: SATA link down (SStatus 0 SControl 300)
 ata3: SATA link down (SStatus 0 SControl 300)
 ata4: SATA link down (SStatus 0 SControl 300)
 PM: Image saving progress:  20%
 PM: Image saving progress:  30%
 PM: Image saving progress:  40%
 PM: Image saving progress:  50%
 pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
 PM: Image saving progress:  60%
 PM: Image saving progress:  70%
 PM: Image saving progress:  80%
 PM: Image saving progress:  90%
 PM: Image saving done
 PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
 PM: S|
 PM: hibernation: Basic memory bitmaps freed
 PM: Image not found (code -16)

This is because when using the swapfile as the hibernation storage,
the block device where the swapfile is located has already been mounted
by the OS distribution(usually been mounted as the rootfs). This is not
an issue for normal hibernation, because software_resume()->swsusp_check()
happens before the block device(rootfs) mount. But it is a problem for the
test_resume mode. Because when test_resume happens, the block device has
been mounted already.

Thus remove the FMODE_EXCL for test_resume mode. This would not be a
problem because in test_resume stage, the processes have already been
frozen, and the race condition described in
Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
is unlikely to happen.

Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
Reported-by: Yifan Li <yifan2.li@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 kernel/power/hibernate.c | 18 +++++++++++-------
 kernel/power/power.h     |  2 +-
 kernel/power/swap.c      | 10 +++++++---
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 793c55a2becb..f50456e72f0a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -683,22 +683,26 @@ static void power_down(void)
 		cpu_relax();
 }
 
-static int load_image_and_restore(void)
+static int load_image_and_restore(bool safe)
 {
+	fmode_t mode = FMODE_READ;
 	int error;
 	unsigned int flags;
 
 	pm_pr_dbg("Loading hibernation image.\n");
 
+	if (!safe)
+		mode |= FMODE_EXCL;
+
 	lock_device_hotplug();
 	error = create_basic_memory_bitmaps();
 	if (error) {
-		swsusp_close(FMODE_READ | FMODE_EXCL);
+		swsusp_close(mode);
 		goto Unlock;
 	}
 
 	error = swsusp_read(&flags);
-	swsusp_close(FMODE_READ | FMODE_EXCL);
+	swsusp_close(mode);
 	if (!error)
 		error = hibernation_restore(flags & SF_PLATFORM_MODE);
 
@@ -785,9 +789,9 @@ int hibernate(void)
 	unlock_device_hotplug();
 	if (snapshot_test) {
 		pm_pr_dbg("Checking hibernation image\n");
-		error = swsusp_check();
+		error = swsusp_check(true);
 		if (!error)
-			error = load_image_and_restore();
+			error = load_image_and_restore(true);
 	}
 	thaw_processes();
 
@@ -983,7 +987,7 @@ static int software_resume(void)
 		MAJOR(swsusp_resume_device), MINOR(swsusp_resume_device));
 
 	pm_pr_dbg("Looking for hibernation image.\n");
-	error = swsusp_check();
+	error = swsusp_check(false);
 	if (error)
 		goto Unlock;
 
@@ -1011,7 +1015,7 @@ static int software_resume(void)
 		goto Close_Finish;
 	}
 
-	error = load_image_and_restore();
+	error = load_image_and_restore(false);
 	thaw_processes();
  Finish:
 	pm_notifier_call_chain(PM_POST_RESTORE);
diff --git a/kernel/power/power.h b/kernel/power/power.h
index b4f433943209..66a7595ad3e7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -173,7 +173,7 @@ extern int swsusp_swap_in_use(void);
 #define SF_HW_SIG		8
 
 /* kernel/power/hibernate.c */
-extern int swsusp_check(void);
+extern int swsusp_check(bool safe);
 extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 36a1df48280c..1be0257da8ab 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1514,13 +1514,17 @@ int swsusp_read(unsigned int *flags_p)
  *      swsusp_check - Check for swsusp signature in the resume device
  */
 
-int swsusp_check(void)
+int swsusp_check(bool safe)
 {
+	fmode_t mode = FMODE_READ;
 	int error;
 	void *holder;
 
+	if (!safe)
+		mode |= FMODE_EXCL;
+
 	hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device,
-					    FMODE_READ | FMODE_EXCL, &holder);
+					    mode, &holder);
 	if (!IS_ERR(hib_resume_bdev)) {
 		set_blocksize(hib_resume_bdev, PAGE_SIZE);
 		clear_page(swsusp_header);
@@ -1547,7 +1551,7 @@ int swsusp_check(void)
 
 put:
 		if (error)
-			blkdev_put(hib_resume_bdev, FMODE_READ | FMODE_EXCL);
+			blkdev_put(hib_resume_bdev, mode);
 		else
 			pr_debug("Image signature found, resuming\n");
 	} else {
-- 
2.25.1

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Pavan Kondeti]

On Sun, Apr 02, 2023 at 12:55:40AM +0800, Chen Yu wrote:
> The system refused to do a test_resume because it found that the
> swap device has already been taken by someone else. Specificly,
> the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> do this check.
> 
> Steps to reproduce:
>  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo | 
>        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
>  mkswap /swapfile
>  swapon /swapfile
>  swap-offset /swapfile
>  echo 34816 > /sys/power/resume_offset
>  echo test_resume > /sys/power/disk
>  echo disk > /sys/power/state
> 
>  PM: Using 3 thread(s) for compression
>  PM: Compressing and saving image data (293150 pages)...
>  PM: Image saving progress:   0%
>  PM: Image saving progress:  10%
>  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>  ata1.00: configured for UDMA/100
>  ata2: SATA link down (SStatus 0 SControl 300)
>  ata5: SATA link down (SStatus 0 SControl 300)
>  ata6: SATA link down (SStatus 0 SControl 300)
>  ata3: SATA link down (SStatus 0 SControl 300)
>  ata4: SATA link down (SStatus 0 SControl 300)
>  PM: Image saving progress:  20%
>  PM: Image saving progress:  30%
>  PM: Image saving progress:  40%
>  PM: Image saving progress:  50%
>  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
>  PM: Image saving progress:  60%
>  PM: Image saving progress:  70%
>  PM: Image saving progress:  80%
>  PM: Image saving progress:  90%
>  PM: Image saving done
>  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
>  PM: S|
>  PM: hibernation: Basic memory bitmaps freed
>  PM: Image not found (code -16)
> 
> This is because when using the swapfile as the hibernation storage,
> the block device where the swapfile is located has already been mounted
> by the OS distribution(usually been mounted as the rootfs). This is not
> an issue for normal hibernation, because software_resume()->swsusp_check()
> happens before the block device(rootfs) mount. But it is a problem for the
> test_resume mode. Because when test_resume happens, the block device has
> been mounted already.
> 
> Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> problem because in test_resume stage, the processes have already been
> frozen, and the race condition described in
> Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> is unlikely to happen.
> 
> Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> Reported-by: Yifan Li <yifan2.li@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  kernel/power/hibernate.c | 18 +++++++++++-------
>  kernel/power/power.h     |  2 +-
>  kernel/power/swap.c      | 10 +++++++---
>  3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 793c55a2becb..f50456e72f0a 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -683,22 +683,26 @@ static void power_down(void)
>  		cpu_relax();
>  }
>  
> -static int load_image_and_restore(void)
> +static int load_image_and_restore(bool safe)
>  {
> +	fmode_t mode = FMODE_READ;
>  	int error;
>  	unsigned int flags;
>  
>  	pm_pr_dbg("Loading hibernation image.\n");
>  
> +	if (!safe)
> +		mode |= FMODE_EXCL;
> +
>  	lock_device_hotplug();
>  	error = create_basic_memory_bitmaps();
>  	if (error) {
> -		swsusp_close(FMODE_READ | FMODE_EXCL);
> +		swsusp_close(mode);
>  		goto Unlock;
>  	}
>  
>  	error = swsusp_read(&flags);
> -	swsusp_close(FMODE_READ | FMODE_EXCL);
> +	swsusp_close(mode);
>  	if (!error)
>  		error = hibernation_restore(flags & SF_PLATFORM_MODE);
>  
> @@ -785,9 +789,9 @@ int hibernate(void)
>  	unlock_device_hotplug();
>  	if (snapshot_test) {
>  		pm_pr_dbg("Checking hibernation image\n");
> -		error = swsusp_check();
> +		error = swsusp_check(true);
>  		if (!error)
> -			error = load_image_and_restore();
> +			error = load_image_and_restore(true);
>  	}
>  	thaw_processes();
>  
> @@ -983,7 +987,7 @@ static int software_resume(void)
>  		MAJOR(swsusp_resume_device), MINOR(swsusp_resume_device));
>  
>  	pm_pr_dbg("Looking for hibernation image.\n");
> -	error = swsusp_check();
> +	error = swsusp_check(false);
>  	if (error)
>  		goto Unlock;
>  
> @@ -1011,7 +1015,7 @@ static int software_resume(void)
>  		goto Close_Finish;
>  	}
>  
> -	error = load_image_and_restore();
> +	error = load_image_and_restore(false);
>  	thaw_processes();
>   Finish:
>  	pm_notifier_call_chain(PM_POST_RESTORE);
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index b4f433943209..66a7595ad3e7 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -173,7 +173,7 @@ extern int swsusp_swap_in_use(void);
>  #define SF_HW_SIG		8
>  
>  /* kernel/power/hibernate.c */
> -extern int swsusp_check(void);
> +extern int swsusp_check(bool safe);
>  extern void swsusp_free(void);
>  extern int swsusp_read(unsigned int *flags_p);
>  extern int swsusp_write(unsigned int flags);
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 36a1df48280c..1be0257da8ab 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -1514,13 +1514,17 @@ int swsusp_read(unsigned int *flags_p)
>   *      swsusp_check - Check for swsusp signature in the resume device
>   */
>  
> -int swsusp_check(void)
> +int swsusp_check(bool safe)
>  {
> +	fmode_t mode = FMODE_READ;
>  	int error;
>  	void *holder;
>  
> +	if (!safe)
> +		mode |= FMODE_EXCL;
> +
>  	hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device,
> -					    FMODE_READ | FMODE_EXCL, &holder);
> +					    mode, &holder);
>  	if (!IS_ERR(hib_resume_bdev)) {
>  		set_blocksize(hib_resume_bdev, PAGE_SIZE);
>  		clear_page(swsusp_header);
> @@ -1547,7 +1551,7 @@ int swsusp_check(void)
>  
>  put:
>  		if (error)
> -			blkdev_put(hib_resume_bdev, FMODE_READ | FMODE_EXCL);
> +			blkdev_put(hib_resume_bdev, mode);
>  		else
>  			pr_debug("Image signature found, resuming\n");
>  	} else {

The patch looks good to me and it works. I have just one
question/comment.

What is "safe" here? Because I worked on this problem [1], so I
understood it. but it is not very clear / explicit. One approach I
thought would be to the codepaths aware of "test_resume" via a
global variable called "snapshot_testing" similar to freezer_test_done.
if snapshot_testing is true, don't use exclusive flags.

Thanks,
Pavan

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

Hi Pavan,
On 2023-04-05 at 12:30:00 +0530, Pavan Kondeti wrote:
> On Sun, Apr 02, 2023 at 12:55:40AM +0800, Chen Yu wrote:
> > The system refused to do a test_resume because it found that the
> > swap device has already been taken by someone else. Specificly,
> > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > do this check.
> > 
> > Steps to reproduce:
> >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo | 
> >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> >  mkswap /swapfile
> >  swapon /swapfile
> >  swap-offset /swapfile
> >  echo 34816 > /sys/power/resume_offset
> >  echo test_resume > /sys/power/disk
> >  echo disk > /sys/power/state
> > 
> >  PM: Using 3 thread(s) for compression
> >  PM: Compressing and saving image data (293150 pages)...
> >  PM: Image saving progress:   0%
> >  PM: Image saving progress:  10%
> >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> >  ata1.00: configured for UDMA/100
> >  ata2: SATA link down (SStatus 0 SControl 300)
> >  ata5: SATA link down (SStatus 0 SControl 300)
> >  ata6: SATA link down (SStatus 0 SControl 300)
> >  ata3: SATA link down (SStatus 0 SControl 300)
> >  ata4: SATA link down (SStatus 0 SControl 300)
> >  PM: Image saving progress:  20%
> >  PM: Image saving progress:  30%
> >  PM: Image saving progress:  40%
> >  PM: Image saving progress:  50%
> >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> >  PM: Image saving progress:  60%
> >  PM: Image saving progress:  70%
> >  PM: Image saving progress:  80%
> >  PM: Image saving progress:  90%
> >  PM: Image saving done
> >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> >  PM: S|
> >  PM: hibernation: Basic memory bitmaps freed
> >  PM: Image not found (code -16)
> > 
> > This is because when using the swapfile as the hibernation storage,
> > the block device where the swapfile is located has already been mounted
> > by the OS distribution(usually been mounted as the rootfs). This is not
> > an issue for normal hibernation, because software_resume()->swsusp_check()
> > happens before the block device(rootfs) mount. But it is a problem for the
> > test_resume mode. Because when test_resume happens, the block device has
> > been mounted already.
> > 
> > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > problem because in test_resume stage, the processes have already been
> > frozen, and the race condition described in
> > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > is unlikely to happen.
> > 
> > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > Reported-by: Yifan Li <yifan2.li@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > +int swsusp_check(bool safe)
> >  {
> > +	fmode_t mode = FMODE_READ;
> >  	int error;
> >  	void *holder;
> >  
> > +	if (!safe)
> > +		mode |= FMODE_EXCL;
> > +
> >  	hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device,
> > -					    FMODE_READ | FMODE_EXCL, &holder);
> > +					    mode, &holder);
> >  	if (!IS_ERR(hib_resume_bdev)) {
> >  		set_blocksize(hib_resume_bdev, PAGE_SIZE);
> >  		clear_page(swsusp_header);
> > @@ -1547,7 +1551,7 @@ int swsusp_check(void)
> >  
> >  put:
> >  		if (error)
> > -			blkdev_put(hib_resume_bdev, FMODE_READ | FMODE_EXCL);
> > +			blkdev_put(hib_resume_bdev, mode);
> >  		else
> >  			pr_debug("Image signature found, resuming\n");
> >  	} else {
> 
> The patch looks good to me and it works. I have just one
> question/comment.
> 
> What is "safe" here? Because I worked on this problem [1], so I
> understood it. but it is not very clear / explicit. 
I see.
> One approach I thought would be to the codepaths aware of "test_resume" via a
> global variable called "snapshot_testing" similar to freezer_test_done.
> if snapshot_testing is true, don't use exclusive flags.
This looks reasonable, with this change, we don't have to add "safe" parameter to
swsusp_check() and load_image_and_restore().

thanks,
Chenyu
> 
> Thanks,
> Pavan
> 

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Rafael J. Wysocki]

On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@intel.com> wrote:
>
> The system refused to do a test_resume because it found that the
> swap device has already been taken by someone else. Specificly,
> the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> do this check.
>
> Steps to reproduce:
>  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
>        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
>  mkswap /swapfile
>  swapon /swapfile
>  swap-offset /swapfile
>  echo 34816 > /sys/power/resume_offset
>  echo test_resume > /sys/power/disk
>  echo disk > /sys/power/state
>
>  PM: Using 3 thread(s) for compression
>  PM: Compressing and saving image data (293150 pages)...
>  PM: Image saving progress:   0%
>  PM: Image saving progress:  10%
>  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>  ata1.00: configured for UDMA/100
>  ata2: SATA link down (SStatus 0 SControl 300)
>  ata5: SATA link down (SStatus 0 SControl 300)
>  ata6: SATA link down (SStatus 0 SControl 300)
>  ata3: SATA link down (SStatus 0 SControl 300)
>  ata4: SATA link down (SStatus 0 SControl 300)
>  PM: Image saving progress:  20%
>  PM: Image saving progress:  30%
>  PM: Image saving progress:  40%
>  PM: Image saving progress:  50%
>  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
>  PM: Image saving progress:  60%
>  PM: Image saving progress:  70%
>  PM: Image saving progress:  80%
>  PM: Image saving progress:  90%
>  PM: Image saving done
>  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
>  PM: S|
>  PM: hibernation: Basic memory bitmaps freed
>  PM: Image not found (code -16)
>
> This is because when using the swapfile as the hibernation storage,
> the block device where the swapfile is located has already been mounted
> by the OS distribution(usually been mounted as the rootfs). This is not
> an issue for normal hibernation, because software_resume()->swsusp_check()
> happens before the block device(rootfs) mount. But it is a problem for the
> test_resume mode. Because when test_resume happens, the block device has
> been mounted already.
>
> Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> problem because in test_resume stage, the processes have already been
> frozen, and the race condition described in
> Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> is unlikely to happen.
>
> Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> Reported-by: Yifan Li <yifan2.li@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  kernel/power/hibernate.c | 18 +++++++++++-------
>  kernel/power/power.h     |  2 +-
>  kernel/power/swap.c      | 10 +++++++---
>  3 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 793c55a2becb..f50456e72f0a 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -683,22 +683,26 @@ static void power_down(void)
>                 cpu_relax();
>  }
>
> -static int load_image_and_restore(void)
> +static int load_image_and_restore(bool safe)

It is not very clear why the argument is called "safe".

Either this needs to be explained in a comment, or I would just call
it "exclusive" and rework the checks accordingly.

>  {
> +       fmode_t mode = FMODE_READ;
>         int error;
>         unsigned int flags;
>
>         pm_pr_dbg("Loading hibernation image.\n");
>
> +       if (!safe)
> +               mode |= FMODE_EXCL;
> +
>         lock_device_hotplug();
>         error = create_basic_memory_bitmaps();
>         if (error) {
> -               swsusp_close(FMODE_READ | FMODE_EXCL);
> +               swsusp_close(mode);
>                 goto Unlock;
>         }
>
>         error = swsusp_read(&flags);
> -       swsusp_close(FMODE_READ | FMODE_EXCL);
> +       swsusp_close(mode);
>         if (!error)
>                 error = hibernation_restore(flags & SF_PLATFORM_MODE);
>
> @@ -785,9 +789,9 @@ int hibernate(void)
>         unlock_device_hotplug();
>         if (snapshot_test) {
>                 pm_pr_dbg("Checking hibernation image\n");
> -               error = swsusp_check();
> +               error = swsusp_check(true);
>                 if (!error)
> -                       error = load_image_and_restore();
> +                       error = load_image_and_restore(true);
>         }
>         thaw_processes();
>
> @@ -983,7 +987,7 @@ static int software_resume(void)
>                 MAJOR(swsusp_resume_device), MINOR(swsusp_resume_device));
>
>         pm_pr_dbg("Looking for hibernation image.\n");
> -       error = swsusp_check();
> +       error = swsusp_check(false);
>         if (error)
>                 goto Unlock;
>
> @@ -1011,7 +1015,7 @@ static int software_resume(void)
>                 goto Close_Finish;
>         }
>
> -       error = load_image_and_restore();
> +       error = load_image_and_restore(false);
>         thaw_processes();
>   Finish:
>         pm_notifier_call_chain(PM_POST_RESTORE);
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index b4f433943209..66a7595ad3e7 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -173,7 +173,7 @@ extern int swsusp_swap_in_use(void);
>  #define SF_HW_SIG              8
>
>  /* kernel/power/hibernate.c */
> -extern int swsusp_check(void);
> +extern int swsusp_check(bool safe);
>  extern void swsusp_free(void);
>  extern int swsusp_read(unsigned int *flags_p);
>  extern int swsusp_write(unsigned int flags);
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 36a1df48280c..1be0257da8ab 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -1514,13 +1514,17 @@ int swsusp_read(unsigned int *flags_p)
>   *      swsusp_check - Check for swsusp signature in the resume device
>   */
>
> -int swsusp_check(void)
> +int swsusp_check(bool safe)

An analogous comment applies here.

>  {
> +       fmode_t mode = FMODE_READ;
>         int error;
>         void *holder;
>
> +       if (!safe)
> +               mode |= FMODE_EXCL;
> +
>         hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device,
> -                                           FMODE_READ | FMODE_EXCL, &holder);
> +                                           mode, &holder);
>         if (!IS_ERR(hib_resume_bdev)) {
>                 set_blocksize(hib_resume_bdev, PAGE_SIZE);
>                 clear_page(swsusp_header);
> @@ -1547,7 +1551,7 @@ int swsusp_check(void)
>
>  put:
>                 if (error)
> -                       blkdev_put(hib_resume_bdev, FMODE_READ | FMODE_EXCL);
> +                       blkdev_put(hib_resume_bdev, mode);
>                 else
>                         pr_debug("Image signature found, resuming\n");
>         } else {
> --

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

Hi Rafael,
On 2023-04-05 at 20:37:32 +0200, Rafael J. Wysocki wrote:
> On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > The system refused to do a test_resume because it found that the
> > swap device has already been taken by someone else. Specificly,
> > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > do this check.
> >
> > Steps to reproduce:
> >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
> >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> >  mkswap /swapfile
> >  swapon /swapfile
> >  swap-offset /swapfile
> >  echo 34816 > /sys/power/resume_offset
> >  echo test_resume > /sys/power/disk
> >  echo disk > /sys/power/state
> >
> >  PM: Using 3 thread(s) for compression
> >  PM: Compressing and saving image data (293150 pages)...
> >  PM: Image saving progress:   0%
> >  PM: Image saving progress:  10%
> >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> >  ata1.00: configured for UDMA/100
> >  ata2: SATA link down (SStatus 0 SControl 300)
> >  ata5: SATA link down (SStatus 0 SControl 300)
> >  ata6: SATA link down (SStatus 0 SControl 300)
> >  ata3: SATA link down (SStatus 0 SControl 300)
> >  ata4: SATA link down (SStatus 0 SControl 300)
> >  PM: Image saving progress:  20%
> >  PM: Image saving progress:  30%
> >  PM: Image saving progress:  40%
> >  PM: Image saving progress:  50%
> >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> >  PM: Image saving progress:  60%
> >  PM: Image saving progress:  70%
> >  PM: Image saving progress:  80%
> >  PM: Image saving progress:  90%
> >  PM: Image saving done
> >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> >  PM: S|
> >  PM: hibernation: Basic memory bitmaps freed
> >  PM: Image not found (code -16)
> >
> > This is because when using the swapfile as the hibernation storage,
> > the block device where the swapfile is located has already been mounted
> > by the OS distribution(usually been mounted as the rootfs). This is not
> > an issue for normal hibernation, because software_resume()->swsusp_check()
> > happens before the block device(rootfs) mount. But it is a problem for the
> > test_resume mode. Because when test_resume happens, the block device has
> > been mounted already.
> >
> > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > problem because in test_resume stage, the processes have already been
> > frozen, and the race condition described in
> > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > is unlikely to happen.
> >
> > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > Reported-by: Yifan Li <yifan2.li@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  kernel/power/hibernate.c | 18 +++++++++++-------
> >  kernel/power/power.h     |  2 +-
> >  kernel/power/swap.c      | 10 +++++++---
> >  3 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index 793c55a2becb..f50456e72f0a 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -683,22 +683,26 @@ static void power_down(void)
> >                 cpu_relax();
> >  }
> >
> > -static int load_image_and_restore(void)
> > +static int load_image_and_restore(bool safe)
> 
> It is not very clear why the argument is called "safe".
> 
> Either this needs to be explained in a comment, or I would just call
> it "exclusive" and rework the checks accordingly.
> 
OK, I can change it to "exclusive". Pavan proposed to add a global
variable snapshot_testing to indicate that the system is in test_resume mode,
and we can check this flag to decide whether to open the block device
exclusively or not. Then we don't have to add parameter for load_image_and_restore()
nor swsusp_check(). Could you please give advice whether this is applicable?
If yes I can change the code accordingly, otherwise I can change the "safe"
to "exclusive" and add some comments.

thanks,
Chenyu

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Rafael J. Wysocki]

On Thu, Apr 6, 2023 at 4:49 AM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Hi Rafael,
> On 2023-04-05 at 20:37:32 +0200, Rafael J. Wysocki wrote:
> > On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@intel.com> wrote:
> > >
> > > The system refused to do a test_resume because it found that the
> > > swap device has already been taken by someone else. Specificly,
> > > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > > do this check.
> > >
> > > Steps to reproduce:
> > >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
> > >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> > >  mkswap /swapfile
> > >  swapon /swapfile
> > >  swap-offset /swapfile
> > >  echo 34816 > /sys/power/resume_offset
> > >  echo test_resume > /sys/power/disk
> > >  echo disk > /sys/power/state
> > >
> > >  PM: Using 3 thread(s) for compression
> > >  PM: Compressing and saving image data (293150 pages)...
> > >  PM: Image saving progress:   0%
> > >  PM: Image saving progress:  10%
> > >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> > >  ata1.00: configured for UDMA/100
> > >  ata2: SATA link down (SStatus 0 SControl 300)
> > >  ata5: SATA link down (SStatus 0 SControl 300)
> > >  ata6: SATA link down (SStatus 0 SControl 300)
> > >  ata3: SATA link down (SStatus 0 SControl 300)
> > >  ata4: SATA link down (SStatus 0 SControl 300)
> > >  PM: Image saving progress:  20%
> > >  PM: Image saving progress:  30%
> > >  PM: Image saving progress:  40%
> > >  PM: Image saving progress:  50%
> > >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> > >  PM: Image saving progress:  60%
> > >  PM: Image saving progress:  70%
> > >  PM: Image saving progress:  80%
> > >  PM: Image saving progress:  90%
> > >  PM: Image saving done
> > >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> > >  PM: S|
> > >  PM: hibernation: Basic memory bitmaps freed
> > >  PM: Image not found (code -16)
> > >
> > > This is because when using the swapfile as the hibernation storage,
> > > the block device where the swapfile is located has already been mounted
> > > by the OS distribution(usually been mounted as the rootfs). This is not
> > > an issue for normal hibernation, because software_resume()->swsusp_check()
> > > happens before the block device(rootfs) mount. But it is a problem for the
> > > test_resume mode. Because when test_resume happens, the block device has
> > > been mounted already.
> > >
> > > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > > problem because in test_resume stage, the processes have already been
> > > frozen, and the race condition described in
> > > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > is unlikely to happen.
> > >
> > > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > Reported-by: Yifan Li <yifan2.li@intel.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > >  kernel/power/hibernate.c | 18 +++++++++++-------
> > >  kernel/power/power.h     |  2 +-
> > >  kernel/power/swap.c      | 10 +++++++---
> > >  3 files changed, 19 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > index 793c55a2becb..f50456e72f0a 100644
> > > --- a/kernel/power/hibernate.c
> > > +++ b/kernel/power/hibernate.c
> > > @@ -683,22 +683,26 @@ static void power_down(void)
> > >                 cpu_relax();
> > >  }
> > >
> > > -static int load_image_and_restore(void)
> > > +static int load_image_and_restore(bool safe)
> >
> > It is not very clear why the argument is called "safe".
> >
> > Either this needs to be explained in a comment, or I would just call
> > it "exclusive" and rework the checks accordingly.
> >
> OK, I can change it to "exclusive". Pavan proposed to add a global
> variable snapshot_testing to indicate that the system is in test_resume mode,
> and we can check this flag to decide whether to open the block device
> exclusively or not. Then we don't have to add parameter for load_image_and_restore()
> nor swsusp_check(). Could you please give advice whether this is applicable?

Well, in that case, why don't you simply check pm_test_level?

> If yes I can change the code accordingly, otherwise I can change the "safe"
> to "exclusive" and add some comments.

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

On 2023-04-06 at 12:02:01 +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 6, 2023 at 4:49 AM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > Hi Rafael,
> > On 2023-04-05 at 20:37:32 +0200, Rafael J. Wysocki wrote:
> > > On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@intel.com> wrote:
> > > >
> > > > The system refused to do a test_resume because it found that the
> > > > swap device has already been taken by someone else. Specificly,
> > > > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > > > do this check.
> > > >
> > > > Steps to reproduce:
> > > >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
> > > >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> > > >  mkswap /swapfile
> > > >  swapon /swapfile
> > > >  swap-offset /swapfile
> > > >  echo 34816 > /sys/power/resume_offset
> > > >  echo test_resume > /sys/power/disk
> > > >  echo disk > /sys/power/state
> > > >
> > > >  PM: Using 3 thread(s) for compression
> > > >  PM: Compressing and saving image data (293150 pages)...
> > > >  PM: Image saving progress:   0%
> > > >  PM: Image saving progress:  10%
> > > >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> > > >  ata1.00: configured for UDMA/100
> > > >  ata2: SATA link down (SStatus 0 SControl 300)
> > > >  ata5: SATA link down (SStatus 0 SControl 300)
> > > >  ata6: SATA link down (SStatus 0 SControl 300)
> > > >  ata3: SATA link down (SStatus 0 SControl 300)
> > > >  ata4: SATA link down (SStatus 0 SControl 300)
> > > >  PM: Image saving progress:  20%
> > > >  PM: Image saving progress:  30%
> > > >  PM: Image saving progress:  40%
> > > >  PM: Image saving progress:  50%
> > > >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> > > >  PM: Image saving progress:  60%
> > > >  PM: Image saving progress:  70%
> > > >  PM: Image saving progress:  80%
> > > >  PM: Image saving progress:  90%
> > > >  PM: Image saving done
> > > >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> > > >  PM: S|
> > > >  PM: hibernation: Basic memory bitmaps freed
> > > >  PM: Image not found (code -16)
> > > >
> > > > This is because when using the swapfile as the hibernation storage,
> > > > the block device where the swapfile is located has already been mounted
> > > > by the OS distribution(usually been mounted as the rootfs). This is not
> > > > an issue for normal hibernation, because software_resume()->swsusp_check()
> > > > happens before the block device(rootfs) mount. But it is a problem for the
> > > > test_resume mode. Because when test_resume happens, the block device has
> > > > been mounted already.
> > > >
> > > > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > > > problem because in test_resume stage, the processes have already been
> > > > frozen, and the race condition described in
> > > > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > is unlikely to happen.
> > > >
> > > > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > Reported-by: Yifan Li <yifan2.li@intel.com>
> > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > ---
> > > >  kernel/power/hibernate.c | 18 +++++++++++-------
> > > >  kernel/power/power.h     |  2 +-
> > > >  kernel/power/swap.c      | 10 +++++++---
> > > >  3 files changed, 19 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > > index 793c55a2becb..f50456e72f0a 100644
> > > > --- a/kernel/power/hibernate.c
> > > > +++ b/kernel/power/hibernate.c
> > > > @@ -683,22 +683,26 @@ static void power_down(void)
> > > >                 cpu_relax();
> > > >  }
> > > >
> > > > -static int load_image_and_restore(void)
> > > > +static int load_image_and_restore(bool safe)
> > >
> > > It is not very clear why the argument is called "safe".
> > >
> > > Either this needs to be explained in a comment, or I would just call
> > > it "exclusive" and rework the checks accordingly.
> > >
> > OK, I can change it to "exclusive". Pavan proposed to add a global
> > variable snapshot_testing to indicate that the system is in test_resume mode,
> > and we can check this flag to decide whether to open the block device
> > exclusively or not. Then we don't have to add parameter for load_image_and_restore()
> > nor swsusp_check(). Could you please give advice whether this is applicable?
> 
> Well, in that case, why don't you simply check pm_test_level?
>
Sorry I overlooked the code, the snapshot_testing is already there.
I'll change the code accordingly.

thanks,
Chenyu 
> > If yes I can change the code accordingly, otherwise I can change the "safe"
> > to "exclusive" and add some comments.

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

On 2023-04-06 at 12:02:01 +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 6, 2023 at 4:49 AM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > Hi Rafael,
> > On 2023-04-05 at 20:37:32 +0200, Rafael J. Wysocki wrote:
> > > On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@intel.com> wrote:
> > > >
> > > > The system refused to do a test_resume because it found that the
> > > > swap device has already been taken by someone else. Specificly,
> > > > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > > > do this check.
> > > >
> > > > Steps to reproduce:
> > > >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
> > > >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> > > >  mkswap /swapfile
> > > >  swapon /swapfile
> > > >  swap-offset /swapfile
> > > >  echo 34816 > /sys/power/resume_offset
> > > >  echo test_resume > /sys/power/disk
> > > >  echo disk > /sys/power/state
> > > >
> > > >  PM: Using 3 thread(s) for compression
> > > >  PM: Compressing and saving image data (293150 pages)...
> > > >  PM: Image saving progress:   0%
> > > >  PM: Image saving progress:  10%
> > > >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> > > >  ata1.00: configured for UDMA/100
> > > >  ata2: SATA link down (SStatus 0 SControl 300)
> > > >  ata5: SATA link down (SStatus 0 SControl 300)
> > > >  ata6: SATA link down (SStatus 0 SControl 300)
> > > >  ata3: SATA link down (SStatus 0 SControl 300)
> > > >  ata4: SATA link down (SStatus 0 SControl 300)
> > > >  PM: Image saving progress:  20%
> > > >  PM: Image saving progress:  30%
> > > >  PM: Image saving progress:  40%
> > > >  PM: Image saving progress:  50%
> > > >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> > > >  PM: Image saving progress:  60%
> > > >  PM: Image saving progress:  70%
> > > >  PM: Image saving progress:  80%
> > > >  PM: Image saving progress:  90%
> > > >  PM: Image saving done
> > > >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> > > >  PM: S|
> > > >  PM: hibernation: Basic memory bitmaps freed
> > > >  PM: Image not found (code -16)
> > > >
> > > > This is because when using the swapfile as the hibernation storage,
> > > > the block device where the swapfile is located has already been mounted
> > > > by the OS distribution(usually been mounted as the rootfs). This is not
> > > > an issue for normal hibernation, because software_resume()->swsusp_check()
> > > > happens before the block device(rootfs) mount. But it is a problem for the
> > > > test_resume mode. Because when test_resume happens, the block device has
> > > > been mounted already.
> > > >
> > > > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > > > problem because in test_resume stage, the processes have already been
> > > > frozen, and the race condition described in
> > > > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > is unlikely to happen.
> > > >
> > > > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > Reported-by: Yifan Li <yifan2.li@intel.com>
> > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > ---
> > > >  kernel/power/hibernate.c | 18 +++++++++++-------
> > > >  kernel/power/power.h     |  2 +-
> > > >  kernel/power/swap.c      | 10 +++++++---
> > > >  3 files changed, 19 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > > index 793c55a2becb..f50456e72f0a 100644
> > > > --- a/kernel/power/hibernate.c
> > > > +++ b/kernel/power/hibernate.c
> > > > @@ -683,22 +683,26 @@ static void power_down(void)
> > > >                 cpu_relax();
> > > >  }
> > > >
> > > > -static int load_image_and_restore(void)
> > > > +static int load_image_and_restore(bool safe)
> > >
> > > It is not very clear why the argument is called "safe".
> > >
> > > Either this needs to be explained in a comment, or I would just call
> > > it "exclusive" and rework the checks accordingly.
> > >
> > OK, I can change it to "exclusive". Pavan proposed to add a global
> > variable snapshot_testing to indicate that the system is in test_resume mode,
> > and we can check this flag to decide whether to open the block device
> > exclusively or not. Then we don't have to add parameter for load_image_and_restore()
> > nor swsusp_check(). Could you please give advice whether this is applicable?
> 
> Well, in that case, why don't you simply check pm_test_level?
>
After rethink about this further, it seems that the global variable snapshot_testing
can not present the race condition described in 39fbef4b0f77 in a corner case, if
we do like this:

     1. echo test_resume > /sys/power/disk
     2. mkfs.ext4 -O mmp  /dev/sda -b 1024
     3. mount /dev/sda /home/test
     4. echo "/dev/sda" > /sys/power/resume
 
We will still see the kernel crash, because in step4, the software_resume()
will open swap device non-exclusively because step1 has enabled snapshot_testing.

That is to say, to avoid the race condition, we should let software_resume() open
the swap device exclusively no matter what the hibernation mode is.

Maybe fall back to add "exclusive" flag for load_image_and_restore()
and swsusp_check() is simpler.

Pavan, what do you think?

thanks,
Chenyu

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Pavan Kondeti]

On Sun, Apr 09, 2023 at 10:29:37PM +0800, Chen Yu wrote:
> On 2023-04-06 at 12:02:01 +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 6, 2023 at 4:49 AM Chen Yu <yu.c.chen@intel.com> wrote:
> > >
> > > Hi Rafael,
> > > On 2023-04-05 at 20:37:32 +0200, Rafael J. Wysocki wrote:
> > > > On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@intel.com> wrote:
> > > > >
> > > > > The system refused to do a test_resume because it found that the
> > > > > swap device has already been taken by someone else. Specificly,
> > > > > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > > > > do this check.
> > > > >
> > > > > Steps to reproduce:
> > > > >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
> > > > >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> > > > >  mkswap /swapfile
> > > > >  swapon /swapfile
> > > > >  swap-offset /swapfile
> > > > >  echo 34816 > /sys/power/resume_offset
> > > > >  echo test_resume > /sys/power/disk
> > > > >  echo disk > /sys/power/state
> > > > >
> > > > >  PM: Using 3 thread(s) for compression
> > > > >  PM: Compressing and saving image data (293150 pages)...
> > > > >  PM: Image saving progress:   0%
> > > > >  PM: Image saving progress:  10%
> > > > >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> > > > >  ata1.00: configured for UDMA/100
> > > > >  ata2: SATA link down (SStatus 0 SControl 300)
> > > > >  ata5: SATA link down (SStatus 0 SControl 300)
> > > > >  ata6: SATA link down (SStatus 0 SControl 300)
> > > > >  ata3: SATA link down (SStatus 0 SControl 300)
> > > > >  ata4: SATA link down (SStatus 0 SControl 300)
> > > > >  PM: Image saving progress:  20%
> > > > >  PM: Image saving progress:  30%
> > > > >  PM: Image saving progress:  40%
> > > > >  PM: Image saving progress:  50%
> > > > >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> > > > >  PM: Image saving progress:  60%
> > > > >  PM: Image saving progress:  70%
> > > > >  PM: Image saving progress:  80%
> > > > >  PM: Image saving progress:  90%
> > > > >  PM: Image saving done
> > > > >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> > > > >  PM: S|
> > > > >  PM: hibernation: Basic memory bitmaps freed
> > > > >  PM: Image not found (code -16)
> > > > >
> > > > > This is because when using the swapfile as the hibernation storage,
> > > > > the block device where the swapfile is located has already been mounted
> > > > > by the OS distribution(usually been mounted as the rootfs). This is not
> > > > > an issue for normal hibernation, because software_resume()->swsusp_check()
> > > > > happens before the block device(rootfs) mount. But it is a problem for the
> > > > > test_resume mode. Because when test_resume happens, the block device has
> > > > > been mounted already.
> > > > >
> > > > > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > > > > problem because in test_resume stage, the processes have already been
> > > > > frozen, and the race condition described in
> > > > > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > > is unlikely to happen.
> > > > >
> > > > > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > > Reported-by: Yifan Li <yifan2.li@intel.com>
> > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > > ---
> > > > >  kernel/power/hibernate.c | 18 +++++++++++-------
> > > > >  kernel/power/power.h     |  2 +-
> > > > >  kernel/power/swap.c      | 10 +++++++---
> > > > >  3 files changed, 19 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > > > index 793c55a2becb..f50456e72f0a 100644
> > > > > --- a/kernel/power/hibernate.c
> > > > > +++ b/kernel/power/hibernate.c
> > > > > @@ -683,22 +683,26 @@ static void power_down(void)
> > > > >                 cpu_relax();
> > > > >  }
> > > > >
> > > > > -static int load_image_and_restore(void)
> > > > > +static int load_image_and_restore(bool safe)
> > > >
> > > > It is not very clear why the argument is called "safe".
> > > >
> > > > Either this needs to be explained in a comment, or I would just call
> > > > it "exclusive" and rework the checks accordingly.
> > > >
> > > OK, I can change it to "exclusive". Pavan proposed to add a global
> > > variable snapshot_testing to indicate that the system is in test_resume mode,
> > > and we can check this flag to decide whether to open the block device
> > > exclusively or not. Then we don't have to add parameter for load_image_and_restore()
> > > nor swsusp_check(). Could you please give advice whether this is applicable?
> > 
> > Well, in that case, why don't you simply check pm_test_level?
> >
> After rethink about this further, it seems that the global variable snapshot_testing
> can not present the race condition described in 39fbef4b0f77 in a corner case, if
> we do like this:
> 
>      1. echo test_resume > /sys/power/disk
>      2. mkfs.ext4 -O mmp  /dev/sda -b 1024
>      3. mount /dev/sda /home/test
>      4. echo "/dev/sda" > /sys/power/resume
>  
> We will still see the kernel crash, because in step4, the software_resume()
> will open swap device non-exclusively because step1 has enabled snapshot_testing.
> 
> That is to say, to avoid the race condition, we should let software_resume() open
> the swap device exclusively no matter what the hibernation mode is.
> 
> Maybe fall back to add "exclusive" flag for load_image_and_restore()
> and swsusp_check() is simpler.
> 
> Pavan, what do you think?
> 
Right, If we directly use (hibernation_mode == HIBERNATION_TEST_RESUME)
condition, it would be a problem. I was saying, snapshot_test which
is a local variable in hibernate() needs to be made global so that
block device open / close can use flags approriately. Onething I did not
like was passing flags to swsusp_close(). Thats the reason for me
to cache the flags while opening the block device and using it in
the swsusp_close().

Thanks,
Pavan

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Wang, Wendy]

On 2023-04-02 at 00:55:40 +0800, Chen Yu wrote: 
> The system refused to do a test_resume because it found that the
> swap device has already been taken by someone else. Specificly,
> the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> do this check.
> 
> Steps to reproduce:
>  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo | 
>        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
>  mkswap /swapfile
>  swapon /swapfile
>  swap-offset /swapfile
>  echo 34816 > /sys/power/resume_offset
>  echo test_resume > /sys/power/disk
>  echo disk > /sys/power/state
> 
>  PM: Using 3 thread(s) for compression
>  PM: Compressing and saving image data (293150 pages)...
>  PM: Image saving progress:   0%
>  PM: Image saving progress:  10%
>  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>  ata1.00: configured for UDMA/100
>  ata2: SATA link down (SStatus 0 SControl 300)
>  ata5: SATA link down (SStatus 0 SControl 300)
>  ata6: SATA link down (SStatus 0 SControl 300)
>  ata3: SATA link down (SStatus 0 SControl 300)
>  ata4: SATA link down (SStatus 0 SControl 300)
>  PM: Image saving progress:  20%
>  PM: Image saving progress:  30%
>  PM: Image saving progress:  40%
>  PM: Image saving progress:  50%
>  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
>  PM: Image saving progress:  60%
>  PM: Image saving progress:  70%
>  PM: Image saving progress:  80%
>  PM: Image saving progress:  90%
>  PM: Image saving done
>  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
>  PM: S|
>  PM: hibernation: Basic memory bitmaps freed
>  PM: Image not found (code -16)
> 
> This is because when using the swapfile as the hibernation storage,
> the block device where the swapfile is located has already been mounted
> by the OS distribution(usually been mounted as the rootfs). This is not
> an issue for normal hibernation, because software_resume()->swsusp_check()
> happens before the block device(rootfs) mount. But it is a problem for the
> test_resume mode. Because when test_resume happens, the block device has
> been mounted already.
> 
> Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> problem because in test_resume stage, the processes have already been
> frozen, and the race condition described in
> Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> is unlikely to happen.
> 
> Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> Reported-by: Yifan Li <yifan2.li@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
>
Tested on Intel EMR, PASS. 
Tested-by: Wendy Wang <wendy.wang@intel.com>

[PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

The system refused to do a test_resume because it found that the
swap device has already been taken by someone else. Specificly,
the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
do this check.

Steps to reproduce:
 dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
    awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
 mkswap /swapfile
 swapon /swapfile
 swap-offset /swapfile
 echo 34816 > /sys/power/resume_offset
 echo test_resume > /sys/power/disk
 echo disk > /sys/power/state

 PM: Using 3 thread(s) for compression
 PM: Compressing and saving image data (293150 pages)...
 PM: Image saving progress:   0%
 PM: Image saving progress:  10%
 ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
 ata1.00: configured for UDMA/100
 ata2: SATA link down (SStatus 0 SControl 300)
 ata5: SATA link down (SStatus 0 SControl 300)
 ata6: SATA link down (SStatus 0 SControl 300)
 ata3: SATA link down (SStatus 0 SControl 300)
 ata4: SATA link down (SStatus 0 SControl 300)
 PM: Image saving progress:  20%
 PM: Image saving progress:  30%
 PM: Image saving progress:  40%
 PM: Image saving progress:  50%
 pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
 PM: Image saving progress:  60%
 PM: Image saving progress:  70%
 PM: Image saving progress:  80%
 PM: Image saving progress:  90%
 PM: Image saving done
 PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
 PM: S|
 PM: hibernation: Basic memory bitmaps freed
 PM: Image not found (code -16)

This is because when using the swapfile as the hibernation storage,
the block device where the swapfile is located has already been mounted
by the OS distribution(usually been mounted as the rootfs). This is not
an issue for normal hibernation, because software_resume()->swsusp_check()
happens before the block device(rootfs) mount. But it is a problem for the
test_resume mode. Because when test_resume happens, the block device has
been mounted already.

Thus remove the FMODE_EXCL for test_resume mode. This would not be a
problem because in test_resume stage, the processes have already been
frozen, and the race condition described in
Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
is unlikely to happen.

Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
Reported-by: Yifan Li <yifan2.li@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 kernel/power/hibernate.c |  4 ++--
 kernel/power/power.h     |  2 +-
 kernel/power/swap.c      | 10 +++++++---
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 793c55a2becb..81287bfa7543 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -785,7 +785,7 @@ int hibernate(void)
 	unlock_device_hotplug();
 	if (snapshot_test) {
 		pm_pr_dbg("Checking hibernation image\n");
-		error = swsusp_check();
+		error = swsusp_check(true);
 		if (!error)
 			error = load_image_and_restore();
 	}
@@ -983,7 +983,7 @@ static int software_resume(void)
 		MAJOR(swsusp_resume_device), MINOR(swsusp_resume_device));
 
 	pm_pr_dbg("Looking for hibernation image.\n");
-	error = swsusp_check();
+	error = swsusp_check(false);
 	if (error)
 		goto Unlock;
 
diff --git a/kernel/power/power.h b/kernel/power/power.h
index b4f433943209..66a7595ad3e7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -173,7 +173,7 @@ extern int swsusp_swap_in_use(void);
 #define SF_HW_SIG		8
 
 /* kernel/power/hibernate.c */
-extern int swsusp_check(void);
+extern int swsusp_check(bool safe);
 extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 36a1df48280c..1be0257da8ab 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1514,13 +1514,17 @@ int swsusp_read(unsigned int *flags_p)
  *      swsusp_check - Check for swsusp signature in the resume device
  */
 
-int swsusp_check(void)
+int swsusp_check(bool safe)
 {
+	fmode_t mode = FMODE_READ;
 	int error;
 	void *holder;
 
+	if (!safe)
+		mode |= FMODE_EXCL;
+
 	hib_resume_bdev = blkdev_get_by_dev(swsusp_resume_device,
-					    FMODE_READ | FMODE_EXCL, &holder);
+					    mode, &holder);
 	if (!IS_ERR(hib_resume_bdev)) {
 		set_blocksize(hib_resume_bdev, PAGE_SIZE);
 		clear_page(swsusp_header);
@@ -1547,7 +1551,7 @@ int swsusp_check(void)
 
 put:
 		if (error)
-			blkdev_put(hib_resume_bdev, FMODE_READ | FMODE_EXCL);
+			blkdev_put(hib_resume_bdev, mode);
 		else
 			pr_debug("Image signature found, resuming\n");
 	} else {
-- 
2.25.1

Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode

[Chen Yu]

On 2023-04-01 at 23:48:32 +0800, Chen Yu wrote:
> The system refused to do a test_resume because it found that the
> swap device has already been taken by someone else. Specificly,
> the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> do this check.
> 
> Steps to reproduce:
>  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
>     awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
>  mkswap /swapfile
>  swapon /swapfile
>  swap-offset /swapfile
>  echo 34816 > /sys/power/resume_offset
>  echo test_resume > /sys/power/disk
>  echo disk > /sys/power/state
> 
>  PM: Using 3 thread(s) for compression
>  PM: Compressing and saving image data (293150 pages)...
>  PM: Image saving progress:   0%
>  PM: Image saving progress:  10%
>  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>  ata1.00: configured for UDMA/100
>  ata2: SATA link down (SStatus 0 SControl 300)
>  ata5: SATA link down (SStatus 0 SControl 300)
>  ata6: SATA link down (SStatus 0 SControl 300)
>  ata3: SATA link down (SStatus 0 SControl 300)
>  ata4: SATA link down (SStatus 0 SControl 300)
>  PM: Image saving progress:  20%
>  PM: Image saving progress:  30%
>  PM: Image saving progress:  40%
>  PM: Image saving progress:  50%
>  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
>  PM: Image saving progress:  60%
>  PM: Image saving progress:  70%
>  PM: Image saving progress:  80%
>  PM: Image saving progress:  90%
>  PM: Image saving done
>  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
>  PM: S|
>  PM: hibernation: Basic memory bitmaps freed
>  PM: Image not found (code -16)
> 
> This is because when using the swapfile as the hibernation storage,
> the block device where the swapfile is located has already been mounted
> by the OS distribution(usually been mounted as the rootfs). This is not
> an issue for normal hibernation, because software_resume()->swsusp_check()
> happens before the block device(rootfs) mount. But it is a problem for the
> test_resume mode. Because when test_resume happens, the block device has
> been mounted already.
> 
> Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> problem because in test_resume stage, the processes have already been
> frozen, and the race condition described in
> Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> is unlikely to happen.
> 
> Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> Reported-by: Yifan Li <yifan2.li@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
>
Please ignore this patch, will send a refined version later. Sorry for the noise.

thanks,
Chenyu