Prev: Add MSG_WAITFORONE flag to recvmmsg
Next: [RFC] Intelligent power sharing driver
From: Johannes Weiner on 26 Mar 2010 19:10
On Fri, Mar 26, 2010 at 03:21:32PM -0700, Yinghai Lu wrote:
> diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
> index adedeef..fe3d953 100644
> --- a/arch/x86/kernel/head32.c
> +++ b/arch/x86/kernel/head32.c
> @@ -47,6 +47,7 @@ void __init i386_start_kernel(void)
> u64 ramdisk_image = boot_params.hdr.ramdisk_image;
> u64 ramdisk_size = boot_params.hdr.ramdisk_size;
> u64 ramdisk_end = ramdisk_image + ramdisk_size;
> + ramdisk_end = PFN_UP(ramdisk_end) << PAGE_SHIFT;

Why not PAGE_ALIGN()?

> reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
> }
> #endif
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index b5a9896..fafba92 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -106,6 +106,7 @@ void __init x86_64_start_reservations(char *real_mode_data)
> unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
> unsigned long ramdisk_size = boot_params.hdr.ramdisk_size;
> unsigned long ramdisk_end = ramdisk_image + ramdisk_size;
> + ramdisk_end = PFN_UP(ramdisk_end) << PAGE_SHIFT;

Dito

> reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
> }
> #endif
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 5d7ba1a..2a1b9d4 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -317,57 +317,61 @@ static void __init relocate_initrd(void)
>
> u64 ramdisk_image = boot_params.hdr.ramdisk_image;
> u64 ramdisk_size = boot_params.hdr.ramdisk_size;
> + u64 ramdisk_end = ramdisk_image + ramdisk_size;
> u64 end_of_lowmem = max_low_pfn_mapped << PAGE_SHIFT;
> + u64 image, size_aligned;
> u64 ramdisk_here;
> unsigned long slop, clen, mapaddr;
> char *p, *q;
>
> + ramdisk_end = PFN_UP(ramdisk_end) << PAGE_SHIFT;

Dito

> + size_aligned = ramdisk_end - ramdisk_image;
> +
> /* We need to move the initrd down into lowmem */
> - ramdisk_here = find_e820_area(0, end_of_lowmem, ramdisk_size,
> + ramdisk_here = find_e820_area(0, end_of_lowmem, size_aligned,
> PAGE_SIZE);
>
> if (ramdisk_here == -1ULL)
> panic("Cannot find place for new RAMDISK of size %lld\n",
> - ramdisk_size);
> + size_aligned);
>
> /* Note: this includes all the lowmem currently occupied by
> the initrd, we rely on that fact to keep the data intact. */
> - reserve_early(ramdisk_here, ramdisk_here + ramdisk_size,
> + reserve_early(ramdisk_here, ramdisk_here + size_aligned,
> "NEW RAMDISK");
> initrd_start = ramdisk_here + PAGE_OFFSET;
> initrd_end = initrd_start + ramdisk_size;
> printk(KERN_INFO "Allocated new RAMDISK: %08llx - %08llx\n",
> - ramdisk_here, ramdisk_here + ramdisk_size);
> + ramdisk_here, ramdisk_here + size_aligned);
>
> q = (char *)initrd_start;
>
> /* Copy any lowmem portion of the initrd */
> - if (ramdisk_image < end_of_lowmem) {
> - clen = end_of_lowmem - ramdisk_image;
> - p = (char *)__va(ramdisk_image);
> + image = ramdisk_image;
> + if (image < end_of_lowmem) {
> + clen = end_of_lowmem - image;
> + p = (char *)__va(image);
> memcpy(q, p, clen);
> q += clen;
> - ramdisk_image += clen;
> - ramdisk_size -= clen;
> + image += clen;
> + size_aligned -= clen;
> }
>
> /* Copy the highmem portion of the initrd */
> - while (ramdisk_size) {
> - slop = ramdisk_image & ~PAGE_MASK;
> - clen = ramdisk_size;
> + while (size_aligned) {
> + slop = image & ~PAGE_MASK;
> + clen = size_aligned;
> if (clen > MAX_MAP_CHUNK-slop)
> clen = MAX_MAP_CHUNK-slop;
> - mapaddr = ramdisk_image & PAGE_MASK;
> + mapaddr = image & PAGE_MASK;
> p = early_memremap(mapaddr, clen+slop);
> memcpy(q, p+slop, clen);
> early_iounmap(p, clen+slop);
> q += clen;
> - ramdisk_image += clen;
> - ramdisk_size -= clen;
> + image += clen;
> + size_aligned -= clen;

Those appear to be a lot of spurious changes. We don't need to
copy the alignment padding as well, so it only matters that the
arguments to find_e820_area() and reserve_early() are aligned, no?

> }
> /* high pages is not converted by early_res_to_bootmem */
> - ramdisk_image = boot_params.hdr.ramdisk_image;
> - ramdisk_size = boot_params.hdr.ramdisk_size;
> printk(KERN_INFO "Move RAMDISK from %016llx - %016llx to"
> " %08llx - %08llx\n",
> ramdisk_image, ramdisk_image + ramdisk_size - 1,
> @@ -385,6 +389,8 @@ static void __init reserve_initrd(void)
> !ramdisk_image || !ramdisk_size)
> return; /* No initrd provided by bootloader */
>
> + ramdisk_end = PFN_UP(ramdisk_end) << PAGE_SHIFT;

Dito

> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index e71c5cb..727d6b0 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -332,6 +332,16 @@ int devmem_is_allowed(unsigned long pagenr)
> void free_init_pages(char *what, unsigned long begin, unsigned long end)
> {
> unsigned long addr = begin;
> + unsigned long addr_aligned, end_aligned;
> +
> + /* Make sure boundaries are page aligned */
> + addr_aligned = PFN_UP(addr) << PAGE_SHIFT;
> + end_aligned = PFN_DOWN(end) << PAGE_SHIFT;
> +
> + if (WARN(addr_aligned != addr || end_aligned != end, "free_init_pages: range [%#lx, %#lx] is not aligned\n", addr, end)) {
> + addr = addr_aligned;
> + end = end_aligned;
> + }

Maybe realign only for when it is not aligned? So to keep the fixup
out of line.

I suppose WARN_ON() is enough as it will print a stack trace which
should be enough clue of what went wrong.

> if (addr >= end)
> return;
> @@ -376,6 +386,18 @@ void free_initmem(void)
> #ifdef CONFIG_BLK_DEV_INITRD
> void free_initrd_mem(unsigned long start, unsigned long end)
> {
> - free_init_pages("initrd memory", start, end);
> + unsigned long end_aligned;
> +
> + /*
> + * end could be not aligned, and We can not align that,
> + * decompresser could be confused by aligned initrd_end
> + * We already reserve the end partial page before in
> + * - i386_start_kernel()
> + * - x86_64_start_kernel()
> + * - relocate_initrd()
> + * So here we can do PFN_UP() safely to get partial page to be freed
> + */
> + end_aligned = PFN_UP(end) << PAGE_SHIFT;

PAGE_ALIGN()

> + free_init_pages("initrd memory", start, end_aligned);
> }
> #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Weiner on 26 Mar 2010 20:10
On Fri, Mar 26, 2010 at 04:45:56PM -0700, Yinghai Lu wrote:
> On 03/26/2010 04:06 PM, Johannes Weiner wrote:
> > On Fri, Mar 26, 2010 at 03:21:32PM -0700, Yinghai Lu wrote:
> >> diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
> >> index adedeef..fe3d953 100644
> >> --- a/arch/x86/kernel/head32.c
> >> +++ b/arch/x86/kernel/head32.c
> >> @@ -47,6 +47,7 @@ void __init i386_start_kernel(void)
> >> u64 ramdisk_image = boot_params.hdr.ramdisk_image;
> >> u64 ramdisk_size = boot_params.hdr.ramdisk_size;
> >> u64 ramdisk_end = ramdisk_image + ramdisk_size;
> >> + ramdisk_end = PFN_UP(ramdisk_end) << PAGE_SHIFT;
> >
> > Why not PAGE_ALIGN()?
>
> looks like PFN_UP is more clear.

Why would you convert to a pfn just to go back to a physical address,
when all you wanted is align at the next page boundary? It looks
rather clumsy.

PAGE_ALIGN() is pretty wide spread and I think it's clear to most
people what it does.

> >> + size_aligned = ramdisk_end - ramdisk_image;
> >> +
> >> /* We need to move the initrd down into lowmem */
> >> - ramdisk_here = find_e820_area(0, end_of_lowmem, ramdisk_size,
> >> + ramdisk_here = find_e820_area(0, end_of_lowmem, size_aligned,
> >> PAGE_SIZE);
> >>
> >> if (ramdisk_here == -1ULL)
> >> panic("Cannot find place for new RAMDISK of size %lld\n",
> >> - ramdisk_size);
> >> + size_aligned);
> >>
> >> /* Note: this includes all the lowmem currently occupied by
> >> the initrd, we rely on that fact to keep the data intact. */
> >> - reserve_early(ramdisk_here, ramdisk_here + ramdisk_size,
> >> + reserve_early(ramdisk_here, ramdisk_here + size_aligned,
> >> "NEW RAMDISK");
> >> initrd_start = ramdisk_here + PAGE_OFFSET;
> >> initrd_end = initrd_start + ramdisk_size;
> >> printk(KERN_INFO "Allocated new RAMDISK: %08llx - %08llx\n",
> >> - ramdisk_here, ramdisk_here + ramdisk_size);
> >> + ramdisk_here, ramdisk_here + size_aligned);
> >>
> >> q = (char *)initrd_start;
> >>
> >> /* Copy any lowmem portion of the initrd */
> >> - if (ramdisk_image < end_of_lowmem) {
> >> - clen = end_of_lowmem - ramdisk_image;
> >> - p = (char *)__va(ramdisk_image);
> >> + image = ramdisk_image;
> >> + if (image < end_of_lowmem) {
> >> + clen = end_of_lowmem - image;
> >> + p = (char *)__va(image);
> >> memcpy(q, p, clen);
> >> q += clen;
> >> - ramdisk_image += clen;
> >> - ramdisk_size -= clen;
> >> + image += clen;
> >> + size_aligned -= clen;
> >> }
> >>
> >> /* Copy the highmem portion of the initrd */
> >> - while (ramdisk_size) {
> >> - slop = ramdisk_image & ~PAGE_MASK;
> >> - clen = ramdisk_size;
> >> + while (size_aligned) {
> >> + slop = image & ~PAGE_MASK;
> >> + clen = size_aligned;
> >> if (clen > MAX_MAP_CHUNK-slop)
> >> clen = MAX_MAP_CHUNK-slop;
> >> - mapaddr = ramdisk_image & PAGE_MASK;
> >> + mapaddr = image & PAGE_MASK;
> >> p = early_memremap(mapaddr, clen+slop);
> >> memcpy(q, p+slop, clen);
> >> early_iounmap(p, clen+slop);
> >> q += clen;
> >> - ramdisk_image += clen;
> >> - ramdisk_size -= clen;
> >> + image += clen;
> >> + size_aligned -= clen;
> >
> > Those appear to be a lot of spurious changes. We don't need to
> > copy the alignment padding as well, so it only matters that the
> > arguments to find_e820_area() and reserve_early() are aligned, no?
>
> we need to reserve that whole page, otherwise other user may use the same page.

I know that. And you would know that I know that, if you actually
read what I wrote.

> >> --- a/arch/x86/mm/init.c
> >> +++ b/arch/x86/mm/init.c
> >> @@ -332,6 +332,16 @@ int devmem_is_allowed(unsigned long pagenr)
> >> void free_init_pages(char *what, unsigned long begin, unsigned long end)
> >> {
> >> unsigned long addr = begin;
> >> + unsigned long addr_aligned, end_aligned;
> >> +
> >> + /* Make sure boundaries are page aligned */
> >> + addr_aligned = PFN_UP(addr) << PAGE_SHIFT;
> >> + end_aligned = PFN_DOWN(end) << PAGE_SHIFT;
> >> +
> >> + if (WARN(addr_aligned != addr || end_aligned != end, "free_init_pages: range [%#lx, %#lx] is not aligned\n", addr, end)) {
> >> + addr = addr_aligned;
> >> + end = end_aligned;
> >> + }
> >
> > Maybe realign only for when it is not aligned? So to keep the fixup
> > out of line.
> >
> > I suppose WARN_ON() is enough as it will print a stack trace which
> > should be enough clue of what went wrong.
>
> we need to PFN_DOWN the end, and don't free the partial page.
> otherwise could crash the system.

Dito.

if (WARN_ON(not_aligned)) {
align
}

is what I meant.

> So just print out the trace, and system still can be used. reporter to get dmesg if
> he don't have serial console.

/me goes arguing with M-x doctor, that thing actually acts on the
supplied input.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Weiner on 27 Mar 2010 20:10
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(a)redhat.com>
> Signed-off-by: Yinghai Lu <yinghai(a)kernel.org>
> Acked-by: Johannes Weiner <hannes(a)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/kernel/head32.c b/arch/x86/kernel/head32.c
index adedeef..ec7c672 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -46,7 +46,8 @@ void __init i386_start_kernel(void)
if (boot_params.hdr.type_of_loader && boot_params.hdr.ramdisk_image) {
u64 ramdisk_image = boot_params.hdr.ramdisk_image;
u64 ramdisk_size = boot_params.hdr.ramdisk_size;
- u64 ramdisk_end = ramdisk_image + ramdisk_size;
+ u64 ramdisk_end = PAGE_ALIGN(ramdisk_image + ramdisk_size);
+
reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
}
#endif
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index b5a9896..a26a8fd 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -105,7 +105,9 @@ void __init x86_64_start_reservations(char *real_mode_data)
if (boot_params.hdr.type_of_loader && boot_params.hdr.ramdisk_image) {
unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
unsigned long ramdisk_size = boot_params.hdr.ramdisk_size;
- unsigned long ramdisk_end = ramdisk_image + ramdisk_size;
+ unsigned long ramdisk_end = PAGE_ALIGN(ramdisk_image +
+ ramdisk_size);
+
reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
}
#endif
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index ca3f8fa..0594923 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -317,13 +317,14 @@ static void __init relocate_initrd(void)

u64 ramdisk_image = boot_params.hdr.ramdisk_image;
u64 ramdisk_size = boot_params.hdr.ramdisk_size;
+ u64 area_size = PAGE_ALIGN(ramdisk_size);
u64 end_of_lowmem = max_low_pfn_mapped << PAGE_SHIFT;
u64 ramdisk_here;
unsigned long slop, clen, mapaddr;
char *p, *q;

/* We need to move the initrd down into lowmem */
- ramdisk_here = find_e820_area(0, end_of_lowmem, ramdisk_size,
+ ramdisk_here = find_e820_area(0, end_of_lowmem, area_size,
PAGE_SIZE);

if (ramdisk_here == -1ULL)
@@ -332,7 +333,7 @@ static void __init relocate_initrd(void)

/* Note: this includes all the lowmem currently occupied by
the initrd, we rely on that fact to keep the data intact. */
- reserve_early(ramdisk_here, ramdisk_here + ramdisk_size,
+ reserve_early(ramdisk_here, ramdisk_here + area_size,
"NEW RAMDISK");
initrd_start = ramdisk_here + PAGE_OFFSET;
initrd_end = initrd_start + ramdisk_size;
@@ -378,7 +379,7 @@ static void __init reserve_initrd(void)
{
u64 ramdisk_image = boot_params.hdr.ramdisk_image;
u64 ramdisk_size = boot_params.hdr.ramdisk_size;
- u64 ramdisk_end = ramdisk_image + ramdisk_size;
+ u64 ramdisk_end = PAGE_ALIGN(ramdisk_image + ramdisk_size);
u64 end_of_lowmem = max_low_pfn_mapped << PAGE_SHIFT;

if (!boot_params.hdr.type_of_loader ||
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++;
}
@@ -376,6 +380,6 @@ void free_initmem(void)
#ifdef CONFIG_BLK_DEV_INITRD
void free_initrd_mem(unsigned long start, unsigned long end)
{
- free_init_pages("initrd memory", start, end);
+ free_init_pages("initrd memory", start, PAGE_ALIGN(end));
}
#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Yinghai Lu on 27 Mar 2010 21:00
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(a)redhat.com>
>> Signed-off-by: Yinghai Lu <yinghai(a)kernel.org>
>> Acked-by: Johannes Weiner <hannes(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Weiner on 27 Mar 2010 21:10
On Sat, Mar 27, 2010 at 05:50:06PM -0700, Yinghai Lu wrote:
> > 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

You are right! It should be enough to move the alignment fixup above the addr >= end
check and in that case? So we would get the warning and then simply return.

> will update that.

Thanks!

> YH

Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: Add MSG_WAITFORONE flag to recvmmsg
Next: [RFC] Intelligent power sharing driver