[PATCH] check that acpi_wakeup_address is below 1MB

[PATCH] check that acpi_wakeup_address is below 1MB

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

Hi,

this patch had a check that the memory allocated is in the first 1MB.
The check is similar to the one in smp_alloc_memory.


Signed-off-by: "Matthieu CASTET <castet.matthieu@free.fr>"

[-- Attachment #2: acpi.diff --]
[-- Type: text/x-patch, Size: 522 bytes --]

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 6bc815c..65ab23c 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -65,6 +65,10 @@ void __init acpi_reserve_bootmem(void)
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
 	if (!acpi_wakeup_address)
 		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
+
+	/* check if we are in first 1MB of memory */
+	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2)
+		BUG();
 }
 
 

Re: [PATCH] check that acpi_wakeup_address is below 1MB

[Ingo Molnar]

* matthieu castet <castet.matthieu@free.fr> wrote:

> Hi,
>
> this patch had a check that the memory allocated is in the first 1MB.
> The check is similar to the one in smp_alloc_memory.
>
>
> Signed-off-by: "Matthieu CASTET <castet.matthieu@free.fr>"

> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 6bc815c..65ab23c 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -65,6 +65,10 @@ void __init acpi_reserve_bootmem(void)
>  	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
>  	if (!acpi_wakeup_address)
>  		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
> +
> +	/* check if we are in first 1MB of memory */
> +	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2)
> +		BUG();

please never BUG() out unless totally unavoidable - especially in such 
early bootup code - that might prevent people from being able to report 
anything beyond 'my bootup hung'.

print a WARN_ON() (that way kerneloops.org can pick it up), and perhaps 
disable ACPI sleep functionality. BUG()-ing out is way too drastic and 
way too user-unfriendly.

	Ingo

Re: [PATCH] check that acpi_wakeup_address is below 1MB

[Rafael J. Wysocki]

On Wednesday, 6 of February 2008, matthieu castet wrote:
> Hi,

Hi,

> this patch had a check that the memory allocated is in the first 1MB.
> The check is similar to the one in smp_alloc_memory.
> 
> 
> Signed-off-by: "Matthieu CASTET <castet.matthieu@free.fr>"

Please don't attach patches if possible.  It's easier to comment them if they
are in the message body.

> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 6bc815c..65ab23c 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -65,6 +65,10 @@ void __init acpi_reserve_bootmem(void)
>  	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
>  	if (!acpi_wakeup_address)
>  		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
> +
> +	/* check if we are in first 1MB of memory */
> +	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2)
> +		BUG();

I don't like this.

First, you should use BUG_ON(something) rather that "if (something) BUG();".

Second, and more importantly, it's enough to put "acpi_wakeup_address = 0"
to disable the feature without crashing the kernel.
Of course, I'd add an error message describing what's wrong to that.

>  }

Thanks,
Rafael

 





-- 
"Premature optimization is the root of all evil." - Donald Knuth