Skip to contentSkip to conversation

Don't explode on swsusp failure to find swap

PATCH In review Benjamin Herrenschmidt 4 participants 9 messages May 31, 2005 – Jun 1, 2005

Subsystems (2)
Suspend to RAM Hibernation
lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/1 Don't explode on swsusp failure to find swap

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 <[email protected]>
kernel/power/swsusp.c +3 -0
Index: linux-work/kernel/power/swsusp.c
===================================================================
@@ -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;
 }
#1 lore raw

Hi!

Quoted 6 lines

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

author
#2 lore raw
Quoted 7 lines · Pavel Machek

Hi!

Quoted 5 lines

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.

Quoted 6 lines
Quoted 1 line

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.

author
#3 lore raw
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 9 lines · Pavel Machek

Hi!

Quoted 6 lines

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 ?

Quoted 7 lines
Quoted 4 lines

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.

#4 lore raw

Hi,

Quoted 24 lines · Benjamin Herrenschmidt
Quoted 4 lines · Benjamin Herrenschmidt
Quoted 3 lines · Pavel Machek

Hi!

Quoted 1 line

If we specify a swap device for swsusp using resume= kernel

argument and

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines

explode when trying to free it.

Pavel, does that look right ?

It looks like a workaround. We should not call swsusp_free in

case

Quoted 1 line
Quoted 1 line

device does not exists. Quick look did not reveal where the bug

comes

Quoted 4 lines
Quoted 2 lines

from, can you try to trace it?
Pavel

Well, the bug comes from arch code calling swsusp_save() which

fails,

Quoted 1 line

then we call swsusp_free()

Quoted 7 lines

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

#5 lore raw

Hi!

Quoted 5 lines
Quoted 5 lines

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:

Quoted 7 lines
Quoted 2 lines

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

#6 lore raw
In reply to Benjamin Herrenschmidt

Hi!

Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

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 ?

Quoted 7 lines
Quoted 4 lines

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()

Quoted 7 lines

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?

kernel/power/swsusp.c +9 -9
@@ -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);
#7 lore raw

Hi,

Quoted 19 lines · Pavel Machek

Hi!

Quoted 17 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

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 ?

Quoted 7 lines
Quoted 4 lines

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().

Quoted 8 lines
Quoted 5 lines

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

Quoted 8 lines

Does something like this help?

kernel/power/swsusp.c +0 -0
@@ -975,13 +975,6 @@ extern asmlinkage int swsusp_arch_resume
 
 asmlinkage int swsusp_save(void)

kernel/power/swsusp.c extern asmlinkage int swsusp_arch_resume · line 979

Quoted 10 lines
kernel/power/swsusp.c +0 -7
 {
-	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

Signature · 3 lines
- 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"
#8 lore raw

Hi!

Quoted 5 lines
Quoted 5 lines
kernel/power/swsusp.c +0 -0
@@ -975,13 +975,6 @@ extern asmlinkage int swsusp_arch_resume
Quoted 14 lines

kernel/power/swsusp.c extern asmlinkage int swsusp_arch_resume · line 979

Quoted 11 lines
kernel/power/swsusp.c +0 -7
 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