Re: [PATCH -v3] x86: Make sure free_init_pages() free pages in boundary

[Yinghai Lu <yinghai@kernel.org>]

On 03/27/2010 05:03 PM, Johannes Weiner wrote:
> Hi,
> 
> On Fri, Mar 26, 2010 at 06:19:33PM -0700, Yinghai Lu wrote:
>>
>>
>> When CONFIG_NO_BOOTMEM, it could use memory more effient, or more compact.
>>
>> Example is:
>> Allocated new RAMDISK: 00ec2000 - 0248ce57
>> Move RAMDISK from 000000002ea04000 - 000000002ffcee56 to 00ec2000 - 0248ce56
>>
>> The new RAMDISK's end is not page aligned.
>> Last page could use shared with other user.
>>
>> When free_init_pages are called for initrd or .init, the page could be freed
>> could have chance to corrupt other data.
>>
>> code segment in free_init_pages()
>> |        for (; addr < end; addr += PAGE_SIZE) {
>> |                ClearPageReserved(virt_to_page(addr));
>> |                init_page_count(virt_to_page(addr));
>> |                memset((void *)(addr & ~(PAGE_SIZE-1)),
>> |                        POISON_FREE_INITMEM, PAGE_SIZE);
>> |                free_page(addr);
>> |                totalram_pages++;
>> |        }
>> last half page could be used as one whole free page.
>>
>> Try to make the boundaries to be page aligned.
>>
>> -v2: make the original initramdisk to be aligned, according to Johannes.
>>      otherwise we have chance to lose one page.
>>      we still need to keep initrd_end not aligned, otherwise it could
>>      confuse decompresser.
>> -v3: change to WARN_ON instead according to Johannes.
>>
>> Reported-by: Stanislaw Gruszka <sgruszka@redhat.com>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Here is what I had in mind when I wrote what you did not read, maybe diff
> works better?
> 
> Main differences:
>   o only fix the area allocation in relocate_initrd(), no need to do
>     copy the alignment bits
>   o keep alignment fixups in free_init_pages() out of line
>   o use PAGE_SIZE(); you might dislike the name, it is still the proper
>     operation here.  if you want to fix it, please do it properly
...
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index e71c5cb..018e793 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -336,6 +336,11 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
>  	if (addr >= end)
>  		return;
>  
> +	if (WARN_ON(addr & ~PAGE_MASK || end & ~PAGE_MASK)) {
> +		addr = PAGE_ALIGN(addr);
> +		end &= PAGE_MASK;
> +	}
> +
>  	/*
>  	 * If debugging page accesses then do not free this memory but
>  	 * mark them not present - any buggy init-section access will
> @@ -355,11 +360,10 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
>  
>  	printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
>  
> -	for (; addr < end; addr += PAGE_SIZE) {
> +	for (; addr != end; addr += PAGE_SIZE) {
>  		ClearPageReserved(virt_to_page(addr));
>  		init_page_count(virt_to_page(addr));
> -		memset((void *)(addr & ~(PAGE_SIZE-1)),
> -			POISON_FREE_INITMEM, PAGE_SIZE);
> +		memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
>  		free_page(addr);
>  		totalram_pages++;
>  	}
something wrong here, if someone pass (0x10, 0x20), the will be aligned to [0x1000, 0]
you will get dead loop 

will update that.

YH