| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <initrd-align-v7-reply@mdm.bga.com>
Date: Thu, 26 May 2011 06:18:46 -0500
From: Milton Miller <miltonm@....com>
To: Dave Carroll <dtcarroll@...il.com>
Cc: LPPC <linuxppc-dev@...ts.ozlabs.org>,
LKML <linux-kernel@...r.kernel.org>,
Dave Carroll <dcarroll@...ekcorp.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [PATCH v7] powerpc: Force page alignment for initrd reserved memory
On Wed, 25 May 2011 about 20:32:42 -0600, Dave Carroll wrote:
> When using 64K pages with a separate cpio rootfs, U-Boot will align
> the rootfs on a 4K page boundary. When the memory is reserved, and
> subsequent early memblock_alloc is called, it will allocate memory
> between the 64K page alignment and reserved memory. When the reserved
> memory is subsequently freed, it is done so by pages, causing the
> early memblock_alloc requests to be re-used, which in my case, caused
> the device-tree to be clobbered.
>
> This patch forces the reserved memory for initrd to be kernel page
> aligned, and will move the device tree if it overlaps with the range
> extension of initrd. This patch will also consolidate the identical
> function free_initrd_mem() from mm/init_32.c, init_64.c to mm/mem.c,
> and adds the same range extension when freeing initrd.
>
> Many thanks to Milton Miller for his input on this patch.
>
Thanks for the mailer update, it applied cleanly.
> Signed-off-by: Dave Carroll <dcarroll@...ekcorp.com>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
>
> ---
> * The previous patch [v6] was the wrong copy, sorry ...
>
> arch/powerpc/kernel/prom.c | 21 ++++++++++++++++++---
> arch/powerpc/mm/init_32.c | 15 ---------------
> arch/powerpc/mm/init_64.c | 13 -------------
> arch/powerpc/mm/mem.c | 19 +++++++++++++++++++
> 4 files changed, 37 insertions(+), 31 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 48aeb55..86966a0 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -81,12 +81,24 @@ static int __init early_parse_mem(char *p)
> return 0;
> }
> early_param("mem", early_parse_mem);
> +/**
> + * overlaps_initrd - check for overlap with page aligned extension of
> + * initrd.
> + */
1) Please place a blank line after the above early_param before
the comment block.
2) /** says it is kernel doc format, so you need to follow the rules.
The description should fit on one line (I might say: ... with the initrd,
page aligned.) and you have to document the parameters. (Or just make
it a simple comment block by removing the second *)
> +static inline int overlaps_initrd(unsigned long start, unsigned long size)
> +{
> + if (!initrd_start)
> + return 0;
>
> + return (start + size) > _ALIGN_DOWN(initrd_start, PAGE_SIZE) &&
> + start <= _ALIGN_UP(initrd_end, PAGE_SIZE);
> +}
> /**
need a blank line after the function before this comment block too.
> * move_device_tree - move tree to an unused area, if needed.
> *
> * The device tree may be allocated beyond our memory limit, or inside the
> - * crash kernel region for kdump. If so, move it out of the way.
> + * crash kernel region for kdump, or within the page aligned range of initrd.
> + * If so, move it out of the way.
of the initrd.
> */
> static void __init move_device_tree(void)
> {
> @@ -99,7 +111,8 @@ static void __init move_device_tree(void)
> size = be32_to_cpu(initial_boot_params->totalsize);
>
> if ((memory_limit && (start + size) > PHYSICAL_START + memory_limit) ||
> - overlaps_crashkernel(start, size)) {
> + overlaps_crashkernel(start, size) ||
> + overlaps_initrd(start, size)) {
> p = __va(memblock_alloc(size, PAGE_SIZE));
> memcpy(p, initial_boot_params, size);
> initial_boot_params = (struct boot_param_header *)p;
> @@ -555,7 +568,9 @@ static void __init early_reserve_mem(void)
> #ifdef CONFIG_BLK_DEV_INITRD
> /* then reserve the initrd, if any */
> if (initrd_start && (initrd_end > initrd_start))
> - memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
> + memblock_reserve(_ALIGN_DOWN(__pa(initrd_start), PAGE_SIZE),
> + _ALIGN_UP(initrd_end, PAGE_SIZE) -
> + _ALIGN_DOWN(initrd_start, PAGE_SIZE));
> #endif /* CONFIG_BLK_DEV_INITRD */
>
> #ifdef CONFIG_PPC32
> diff --git a/arch/powerpc/mm/init_32.c b/arch/powerpc/mm/init_32.c
> index d65b591..5de0f25 100644
> --- a/arch/powerpc/mm/init_32.c
> +++ b/arch/powerpc/mm/init_32.c
> @@ -223,21 +223,6 @@ void free_initmem(void)
> #undef FREESEC
> }
>
> -#ifdef CONFIG_BLK_DEV_INITRD
> -void free_initrd_mem(unsigned long start, unsigned long end)
> -{
> - if (start < end)
> - printk ("Freeing initrd memory: %ldk freed\n", (end - start) >> 10);
> - for (; start < end; start += PAGE_SIZE) {
> - ClearPageReserved(virt_to_page(start));
> - init_page_count(virt_to_page(start));
> - free_page(start);
> - totalram_pages++;
> - }
> -}
> -#endif
> -
> -
> #ifdef CONFIG_8xx /* No 8xx specific .c file to put that in ... */
> void setup_initial_memory_limit(phys_addr_t first_memblock_base,
> phys_addr_t first_memblock_size)
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index 6374b21..7591a97 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -99,19 +99,6 @@ void free_initmem(void)
> ((unsigned long)__init_end - (unsigned long)__init_begin) >> 10);
> }
>
> -#ifdef CONFIG_BLK_DEV_INITRD
> -void free_initrd_mem(unsigned long start, unsigned long end)
> -{
> - if (start < end)
> - printk ("Freeing initrd memory: %ldk freed\n", (end - start) >> 10);
> - for (; start < end; start += PAGE_SIZE) {
> - ClearPageReserved(virt_to_page(start));
> - init_page_count(virt_to_page(start));
> - free_page(start);
> - totalram_pages++;
> - }
> -}
> -#endif
>
> static void pgd_ctor(void *addr)
> {
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 57e545b..f60e44e 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -160,6 +160,25 @@ walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
> }
> EXPORT_SYMBOL_GPL(walk_system_ram_range);
>
> +#ifdef CONFIG_BLK_DEV_INITRD
> +void free_initrd_mem(unsigned long start, unsigned long end)
> +{
> + if (start >= end)
> + return;
> +
> + start = _ALIGN_DOWN(start, PAGE_SIZE);
> + end = _ALIGN_UP(end, PAGE_SIZE);
> + printk(KERN_INFO "Freeing initrd memory: %ldk freed\n",
> + (end - start) >> 10);
If you use pr_info then the above again fits on one line.
(this is what triggered my formatting and whitespace review).
Although I would then add a blank line before the loop for readability.
> + for (; start < end; start += PAGE_SIZE) {
> + ClearPageReserved(virt_to_page(start));
> + init_page_count(virt_to_page(start));
> + free_page(start);
> + totalram_pages++;
> + }
> +}
> +#endif
> +
Nit: Looking at the whole file I would put this after mem_init
because that puts it next to the other code that sets page counts,
clears PageReerved, and hands pages to page_alloc.
> /*
> * Initialize the bootmem system and give it all the memory we
> * have available. If we are using highmem, we only put the
> --
> 1.7.4
>
milton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists