[PATCH] Don't explode on swsusp failure to find swap

[PATCH] Don't explode on swsusp failure to find swap

[Benjamin Herrenschmidt]

Hi !

If we specify a swap device for swsusp using resume= kernel argument and
that device doesn't exist in the swap list, we end up calling
swsusp_free() before we have allocated pagedir_save. That causes us to
explode when trying to free it.

Pavel, does that look right ?

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/kernel/power/swsusp.c
===================================================================
--- linux-work.orig/kernel/power/swsusp.c	2005-05-31 16:29:22.000000000 +1000
+++ linux-work/kernel/power/swsusp.c	2005-05-31 16:57:30.000000000 +1000
@@ -730,10 +730,13 @@
 
 void swsusp_free(void)
 {
+	if (pagedir_save == NULL)
+		return;
 	BUG_ON(PageNosave(virt_to_page(pagedir_save)));
 	BUG_ON(PageNosaveFree(virt_to_page(pagedir_save)));
 	free_image_pages();
 	free_pagedir(pagedir_save);
+	pagedir_save = NULL;
 }
 
 

Re: [PATCH] Don't explode on swsusp failure to find swap

[Pavel Machek]

Hi!

> If we specify a swap device for swsusp using resume= kernel argument and
> that device doesn't exist in the swap list, we end up calling
> swsusp_free() before we have allocated pagedir_save. That causes us to
> explode when trying to free it.
> 
> Pavel, does that look right ?

It looks like a workaround. We should not call swsusp_free in case
device does not exists. Quick look did not reveal where the bug comes
from, can you try to trace it?
								Pavel

Re: [PATCH] Don't explode on swsusp failure to find swap

[Benjamin Herrenschmidt]

On Tue, 2005-05-31 at 12:36 +0200, Pavel Machek wrote:
> Hi!
> 
> > If we specify a swap device for swsusp using resume= kernel argument and
> > that device doesn't exist in the swap list, we end up calling
> > swsusp_free() before we have allocated pagedir_save. That causes us to
> > explode when trying to free it.
> > 
> > Pavel, does that look right ?
> 
> It looks like a workaround. We should not call swsusp_free in case
> device does not exists. Quick look did not reveal where the bug comes
> from, can you try to trace it?
> 								Pavel

Well, the bug comes from arch code calling swsusp_save() which fails,
then we call swsusp_free()

Ben.

Re: [PATCH] Don't explode on swsusp failure to find swap

[Benjamin Herrenschmidt]

On Wed, 2005-06-01 at 00:45 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2005-05-31 at 12:36 +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > If we specify a swap device for swsusp using resume= kernel argument and
> > > that device doesn't exist in the swap list, we end up calling
> > > swsusp_free() before we have allocated pagedir_save. That causes us to
> > > explode when trying to free it.
> > > 
> > > Pavel, does that look right ?
> > 
> > It looks like a workaround. We should not call swsusp_free in case
> > device does not exists. Quick look did not reveal where the bug comes
> > from, can you try to trace it?
> > 								Pavel
> 
> Well, the bug comes from arch code calling swsusp_save() which fails,
> then we call swsusp_free()

More specifically, arch suspend calls swsusp_save().

It fails and returns the error to the arch asm code, which itself
returns it to it's caller swsusp_suspend(), which does that:

 	if ((error = swsusp_arch_suspend()))
		swsusp_free();

Ben.

Re: [linux-pm] Re: [PATCH] Don't explode on swsusp failure to find swap

[Shaohua Li]

Hi,
On Wed, 2005-06-01 at 07:50 +0800, Benjamin Herrenschmidt wrote:
> On Wed, 2005-06-01 at 00:45 +1000, Benjamin Herrenschmidt wrote: 
> > On Tue, 2005-05-31 at 12:36 +0200, Pavel Machek wrote: 
> > > Hi! 
> > >  
> > > > If we specify a swap device for swsusp using resume= kernel
> argument and 
> > > > that device doesn't exist in the swap list, we end up calling 
> > > > swsusp_free() before we have allocated pagedir_save. That causes
> us to 
> > > > explode when trying to free it. 
> > > >  
> > > > Pavel, does that look right ? 
> > >  
> > > It looks like a workaround. We should not call swsusp_free in
> case 
> > > device does not exists. Quick look did not reveal where the bug
> comes 
> > > from, can you try to trace it? 
> > >                                                             Pavel 
> >  
> > Well, the bug comes from arch code calling swsusp_save() which
> fails, 
> > then we call swsusp_free()
> 
> More specifically, arch suspend calls swsusp_save().
> 
> It fails and returns the error to the arch asm code, which itself 
> returns it to it's caller swsusp_suspend(), which does that:
> 
>         if ((error = swsusp_arch_suspend())) 
>                 swsusp_free();
I encounter a similar issue, when swsusp_swap_check failed.
It seems the swsusp_free isn't required in the failure case,
suspend_prepare_image has correctly handled the failure case to me.
Other arch? I wonder why swsusp_free is called after device_power_down
failed as well. No pages are allocated before device_power_down.

Thanks,
Shaohua

Re: [PATCH] Don't explode on swsusp failure to find swap

[Pavel Machek]

Hi!

> > > > If we specify a swap device for swsusp using resume= kernel argument and
> > > > that device doesn't exist in the swap list, we end up calling
> > > > swsusp_free() before we have allocated pagedir_save. That causes us to
> > > > explode when trying to free it.
> > > > 
> > > > Pavel, does that look right ?
> > > 
> > > It looks like a workaround. We should not call swsusp_free in case
> > > device does not exists. Quick look did not reveal where the bug comes
> > > from, can you try to trace it?
> > > 								Pavel
> > 
> > Well, the bug comes from arch code calling swsusp_save() which fails,
> > then we call swsusp_free()
> 
> More specifically, arch suspend calls swsusp_save().
> 
> It fails and returns the error to the arch asm code, which itself
> returns it to it's caller swsusp_suspend(), which does that:
> 
>  	if ((error = swsusp_arch_suspend()))
> 		swsusp_free();

Ugh, swsusp_free should be totally unneccessary at this point; only
error returns are from the time before anything is allocated.

Does something like this help?

diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c
+++ b/kernel/power/swsusp.c
@@ -975,13 +975,6 @@ extern asmlinkage int swsusp_arch_resume
 
 asmlinkage int swsusp_save(void)
 {
-	int error = 0;
-
-	if ((error = swsusp_swap_check())) {
-		printk(KERN_ERR "swsusp: FATAL: cannot find swap device, try "
-				"swapon -a!\n");
-		return error;
-	}
 	return suspend_prepare_image();
 }
 
@@ -999,12 +992,19 @@ int swsusp_suspend(void)
 	 */
 	if ((error = device_power_down(PMSG_FREEZE))) {
 		local_irq_enable();
-		swsusp_free();
 		return error;
 	}
+
+	if ((error = swsusp_swap_check())) {
+		printk(KERN_ERR "swsusp: FATAL: cannot find swap device, try "
+				"swapon -a!\n");
+		local_irq_enable();
+		return error;
+	}
+
 	save_processor_state();
 	if ((error = swsusp_arch_suspend()))
-		swsusp_free();
+		printk("Error %d suspending\n", error);
 	/* Restore control flow magically appears here */
 	restore_processor_state();
 	BUG_ON (nr_copy_pages_check != nr_copy_pages);

Re: [linux-pm] Re: [PATCH] Don't explode on swsusp failure to find swap

[Pavel Machek]

Hi!

> > More specifically, arch suspend calls swsusp_save().
> > 
> > It fails and returns the error to the arch asm code, which itself 
> > returns it to it's caller swsusp_suspend(), which does that:
> > 
> >         if ((error = swsusp_arch_suspend())) 
> >                 swsusp_free();
> I encounter a similar issue, when swsusp_swap_check failed.
> It seems the swsusp_free isn't required in the failure case,
> suspend_prepare_image has correctly handled the failure case to me.
> Other arch? I wonder why swsusp_free is called after device_power_down
> failed as well. No pages are allocated before device_power_down.

Agreed, its wrong. Also there's no reason for the swap check to be
called (even indirectly) from arch code...
								Pavel

Re: [linux-pm] Re: [PATCH] Don't explode on swsusp failure to find swap

[Rafael J. Wysocki]

Hi,

On Wednesday, 1 of June 2005 11:27, Pavel Machek wrote:
> Hi!
> 
> > > > > If we specify a swap device for swsusp using resume= kernel argument and
> > > > > that device doesn't exist in the swap list, we end up calling
> > > > > swsusp_free() before we have allocated pagedir_save. That causes us to
> > > > > explode when trying to free it.
> > > > > 
> > > > > Pavel, does that look right ?
> > > > 
> > > > It looks like a workaround. We should not call swsusp_free in case
> > > > device does not exists. Quick look did not reveal where the bug comes
> > > > from, can you try to trace it?
> > > > 								Pavel
> > > 
> > > Well, the bug comes from arch code calling swsusp_save() which fails,
> > > then we call swsusp_free()
> > 
> > More specifically, arch suspend calls swsusp_save().
> > 
> > It fails and returns the error to the arch asm code, which itself
> > returns it to it's caller swsusp_suspend(), which does that:
> > 
> >  	if ((error = swsusp_arch_suspend()))
> > 		swsusp_free();
> 
> Ugh, swsusp_free should be totally unneccessary at this point; only
> error returns are from the time before anything is allocated.

Oops, it looks like I have introduced these swsusp_free()s, so I'm sorry ...

> Does something like this help?
> 
> diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
> --- a/kernel/power/swsusp.c
> +++ b/kernel/power/swsusp.c
> @@ -975,13 +975,6 @@ extern asmlinkage int swsusp_arch_resume
>  
>  asmlinkage int swsusp_save(void)
>  {
> -	int error = 0;
> -
> -	if ((error = swsusp_swap_check())) {
> -		printk(KERN_ERR "swsusp: FATAL: cannot find swap device, try "
> -				"swapon -a!\n");
> -		return error;
> -	}
>  	return suspend_prepare_image();
>  }

I think we can move the contents of suspend_prepare_image() directly to
swsusp_save().  It's not used anywhere else.

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [linux-pm] Re: [PATCH] Don't explode on swsusp failure to find swap

[Pavel Machek]

Hi!

> > diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
> > --- a/kernel/power/swsusp.c
> > +++ b/kernel/power/swsusp.c
> > @@ -975,13 +975,6 @@ extern asmlinkage int swsusp_arch_resume
> >  
> >  asmlinkage int swsusp_save(void)
> >  {
> > -	int error = 0;
> > -
> > -	if ((error = swsusp_swap_check())) {
> > -		printk(KERN_ERR "swsusp: FATAL: cannot find swap device, try "
> > -				"swapon -a!\n");
> > -		return error;
> > -	}
> >  	return suspend_prepare_image();
> >  }
> 
> I think we can move the contents of suspend_prepare_image() directly to
> swsusp_save().  It's not used anywhere else.

I thought about that, too, but bugfixes first, cleanups next.

									Pavel