[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[william.helsby at stfc.ac.uk]

A boot time system crash was noticed with a segmentation fault just after the initrd image had been used to initialise the ramdisk.
This occurred when the U-Boot loaded the ramdisk image from a FAT partition, but not when loaded by TFTPBOOT. This is not understood?
However the problem was caused by free_initrd_mem freeing and "poisoning" memory that had been allocted to init/main.c to store the saved_command_line
This patch reverses "ARM: 8167/1: extend the reserved memory for initrd to be page aligned" because it is safer to leave a partial head or tail page reserved (wasted) than to free a page which is partially still in use.
If this is not acceptable (particularly if wanting large contiguous physical areas for DMA) then a better solution is required.
This would extend the region reserved to page boundaries, if possible without overlapping other regions. My previous attempt to fix this coded this scheme, to grow the are reserved. 
However, this? again is not safe if in growing the area it then overlaps a region that is in use.
Note this path is against the 4.6 kernel, but as far as I can tell applies equally to 4.8.

Signed-off-by: William Helsby <wih73@xilinxsrv1.dl.ac.uk>
---
arch/arm/mm/init.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581a..ff3e9c3 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -770,12 +770,6 @@ static int keep_initrd;
void free_initrd_mem(unsigned long start, unsigned long end)
{
??????? if (!keep_initrd) {
-?????????????? if (start == initrd_start)
-?????????????????????? start = round_down(start, PAGE_SIZE);
-?????????????? if (end == initrd_end)
-?????????????????????? end = round_up(end, PAGE_SIZE);
-
-???? ??????????poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
??????????????? free_reserved_area((void *)start, (void *)end, -1, "initrd");
??????? }
}
--
1.8.3.1

Science and Technology Facilities Council
SciTech Daresbury
Keckwick Lane
Daresbury
Warrington WA4 4AD

Tel +44(0)1925 603250

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[Russell King - ARM Linux]

On Wed, Nov 09, 2016 at 04:35:37PM +0000, william.helsby at stfc.ac.uk wrote:
> A boot time system crash was noticed with a segmentation fault just after the initrd image had been used to initialise the ramdisk.
> This occurred when the U-Boot loaded the ramdisk image from a FAT partition, but not when loaded by TFTPBOOT. This is not understood?
> However the problem was caused by free_initrd_mem freeing and "poisoning" memory that had been allocted to init/main.c to store the saved_command_line
> This patch reverses "ARM: 8167/1: extend the reserved memory for initrd to be page aligned" because it is safer to leave a partial head or tail page reserved (wasted) than to free a page which is partially still in use.
> If this is not acceptable (particularly if wanting large contiguous physical areas for DMA) then a better solution is required.
> This would extend the region reserved to page boundaries, if possible without overlapping other regions. My previous attempt to fix this coded this scheme, to grow the are reserved. 
> However, this? again is not safe if in growing the area it then overlaps a region that is in use.
> Note this path is against the 4.6 kernel, but as far as I can tell applies equally to 4.8.

Please wrap commit messages at or before column 72, the exception is
for lines with a URL.

> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 370581a..ff3e9c3 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -770,12 +770,6 @@ static int keep_initrd;
> void free_initrd_mem(unsigned long start, unsigned long end)
> {
> ??????? if (!keep_initrd) {
> -?????????????? if (start == initrd_start)
> -?????????????????????? start = round_down(start, PAGE_SIZE);
> -?????????????? if (end == initrd_end)
> -?????????????????????? end = round_up(end, PAGE_SIZE);
> -
> -???? ??????????poison_init_mem((void *)start, PAGE_ALIGN(end) - start);

We're definitely not getting rid of the poisoning of the pages - the
poisoning there is to detect accesses to this memory which should not
be made.

The point of rounding up and down is to ensure that the partly-used
pages (which would have been previously reserved) are freed.

Probably a better fix is to round the start up/end down of the initrd
when reserving the memory region:

 arch/arm/mm/init.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581aeb871..ee8509e4329d 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -255,7 +255,11 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
 		phys_initrd_start = phys_initrd_size = 0;
 	}
 	if (phys_initrd_size) {
-		memblock_reserve(phys_initrd_start, phys_initrd_size);
+		phys_addr_t start, size;
+
+		start = round_down(phys_initrd_start, PAGE_SIZE);
+		end = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
+		memblock_reserve(start, end - start);
 
 		/* Now convert initrd to virtual addresses */
 		initrd_start = __phys_to_virt(phys_initrd_start);

and this should ensure that memblock_alloc() doesn't try to allocate
memory overlapping the pages containing the initrd.

Intentionally using pages overlapping the initrd is a recipe for
problems...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[william.helsby at stfc.ac.uk]

>From 45272bc4d3f27f2c316f5b607441d1337a5501ec Mon Sep 17 00:00:00 2001
From: William Helsby <william.helsby@stfc.ac.uk>
Date: Fri, 11 Nov 2016 15:04:04 +0000
Subject: [PATCH] arm: zynq::Fixing inconsistent reserve and free for initrd
 image
My first attempt at fixing this was very similar to your proposed patch,
though due me not understanding the need to switch outlook to plain text
it never got posted.
In the current case, with the initrd image starting page aligned, with space
above it, the patch worked.
However, on reflection it struck me that the placement of the ramdisk image
is done by the bootloader, so may not always be like this. 
This patch tries to round down the start and up the end, but checks that this 
does not cause an overlap. If it does overlap, it tries aligning just the end 
or start and if neither is possible falls back to not expanding the area 
reserved. 
The code then remembers the start and end of reserved area, with 
any successful expansions to page boundaries. 
If when ./init/initramfs.c calls free_initrd_mem(), the start or end match,
The respective extended values are used so complete pages are released.
Note that according to comments in ./init/initramfs.c, the crashkernel
region can overlap the initrd region, though I don't know anything about
this. 
The POISON_FREE_INITMEM value is used to cause free_reserved_area to
poison only the pages which are released.
Despite this code being careful not to overlap the kernel, there still may be 
an issue with it extending the RAMDISK image reserved are to overlap 
the device tree or something else I am unaware of.
Perhaps the call to early_init_fdt_reserve_self() should be moved 
to before the initrd code?
For the sake of freeing at most 2 partial 4k pages, I am not sure whether 
complexity and technical risk of this solution is justified - unless these 
pages are going to be in the middle of a DMA contiguous area.
Note that the arm64 code reverted to not extending the released area,
but enable clearing of the released areas at 
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/arch/arm64/mm/init.c?id=6b00f7efb5303418c231994c91fb8239f5ada260

Now
void free_initrd_mem(unsigned long start, unsigned long end)
{
	if (!keep_initrd)
		free_reserved_area((void *)start, (void *)end, 0, "initrd");
} 

Signed-off-by: William Helsby <william.helsby@stfc.ac.uk>
---
 arch/arm/mm/init.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581a..00a489d 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -49,6 +49,8 @@ unsigned long __init __clear_cr(unsigned long mask)

 static phys_addr_t phys_initrd_start __initdata = 0;
 static unsigned long phys_initrd_size __initdata = 0;
+static unsigned long initrd_reservation_start __initdata = 0;
+static unsigned long initrd_reservation_end __initdata = 0;

 static int __init early_initrd(char *p)
 {
@@ -255,11 +257,38 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
                phys_initrd_start = phys_initrd_size = 0;
        }
        if (phys_initrd_size) {
-               memblock_reserve(phys_initrd_start, phys_initrd_size);
+           /* try to round the initrd start and end down and up to page boundaries,
+              so when freed, whole pages can be released.
+              However the initrd image may be adjacent to something else, so check that this rounding is OK
+           */
+         /* First try rounding the start down and end up */
+           phys_addr_t phys_initrd_reservation_start = phys_initrd_start & PAGE_MASK;
+            unsigned long size_to_reserve = PAGE_ALIGN(phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+           if (!memblock_is_region_memory(phys_initrd_reservation_start, size_to_reserve) ||
+                 memblock_is_region_reserved(phys_initrd_reservation_start, size_to_reserve)) {
+             /* This either does fit or overlaps something else, so try just rounding end up */
+             phys_initrd_reservation_start = phys_initrd_start;
+             size_to_reserve = PAGE_ALIGN(phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+             if (!memblock_is_region_memory(phys_initrd_reservation_start, size_to_reserve) ||
+                 memblock_is_region_reserved(phys_initrd_reservation_start, size_to_reserve)) {
+               /* This either does not fit or overlaps something else, so try just rounding start down */
+               phys_initrd_reservation_start = phys_initrd_start & PAGE_MASK;
+               size_to_reserve = (phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+               if (!memblock_is_region_memory(phys_initrd_reservation_start, size_to_reserve) ||
+                   memblock_is_region_reserved(phys_initrd_reservation_start, size_to_reserve)) {
+                 /* This either does not fit or overlaps something else, so do not round at all */
+                 phys_initrd_reservation_start = phys_initrd_start;
+                 size_to_reserve = (phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+               }
+             }
+           }
+               memblock_reserve(phys_initrd_reservation_start, size_to_reserve);

                /* Now convert initrd to virtual addresses */
                initrd_start = __phys_to_virt(phys_initrd_start);
                initrd_end = initrd_start + phys_initrd_size;
+               initrd_reservation_start = __phys_to_virt(phys_initrd_reservation_start);
+               initrd_reservation_end = initrd_reservation_start + size_to_reserve;
        }
 #endif

@@ -771,12 +800,11 @@ void free_initrd_mem(unsigned long start, unsigned long end)
 {
        if (!keep_initrd) {
                if (start == initrd_start)
-                       start = round_down(start, PAGE_SIZE);
+                 start = initrd_reservation_start;
                if (end == initrd_end)
-                       end = round_up(end, PAGE_SIZE);
+                 end = initrd_reservation_end;

-               poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
-               free_reserved_area((void *)start, (void *)end, -1, "initrd");
+               free_reserved_area((void *)start, (void *)end, POISON_FREE_INITMEM, "initrd");
        }
 }

--
1.8.3.1

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at armlinux.org.uk] 
> Sent: 10 November 2016 17:47
>To: Helsby, William (STFC,DL,TECH)
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

>On Wed, Nov 09, 2016 at 04:35:37PM +0000, william.helsby at stfc.ac.uk wrote:
>> A boot time system crash was noticed with a segmentation fault just after the initrd image had been used to initialise the ramdisk.
>> This occurred when the U-Boot loaded the ramdisk image from a FAT partition, but not when loaded by TFTPBOOT. This is not understood?
>> However the problem was caused by free_initrd_mem freeing and 
>> "poisoning" memory that had been allocted to init/main.c to store the saved_command_line.
>> This patch reverses "ARM: 8167/1: extend the reserved memory for initrd to be page aligned" 
>> because it is safer to leave a partial head or tail page reserved (wasted) than to free a page which is partially still in use.
>> If this is not acceptable (particularly if wanting large contiguous physical areas for DMA) then a better solution is required.
>> This would extend the region reserved to page boundaries, if possible without overlapping other regions. 
>>My previous attempt to fix this coded this scheme, to grow the are reserved. 
>> However, this? again is not safe if in growing the area it then overlaps a region that is in use.
>> Note this patch is against the 4.6 kernel, but as far as I can tell applies equally to 4.8.
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index 
> 370581a..ff3e9c3 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -770,12 +770,6 @@ static int keep_initrd; void 
>> free_initrd_mem(unsigned long start, unsigned long end) {
>> ??????? if (!keep_initrd) {
>> -?????????????? if (start == initrd_start)
>> -?????????????????????? start = round_down(start, PAGE_SIZE);
>> -?????????????? if (end == initrd_end)
>> -?????????????????????? end = round_up(end, PAGE_SIZE);
>> -
>> -???? ??????????poison_init_mem((void *)start, PAGE_ALIGN(end) - 
>> start);

> We're definitely not getting rid of the poisoning of the pages - the poisoning there is to detect accesses to this memory which should not be made.

> The point of rounding up and down is to ensure that the partly-used pages (which would have been previously reserved) are freed.

> Probably a better fix is to round the start up/end down of the initrd when reserving the memory region:

>  arch/arm/mm/init.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index 370581aeb871..ee8509e4329d 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -255,7 +255,11 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
>  		phys_initrd_start = phys_initrd_size = 0;
>  	}
>  	if (phys_initrd_size) {
> -		memblock_reserve(phys_initrd_start, phys_initrd_size);
> +		phys_addr_t start, size;
> +
> +		start = round_down(phys_initrd_start, PAGE_SIZE);
> +		end = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
> +		memblock_reserve(start, end - start);
>  
>  		/* Now convert initrd to virtual addresses */
>  		initrd_start = __phys_to_virt(phys_initrd_start); 
>
> and this should ensure that memblock_alloc() doesn't try to allocate memory overlapping the pages containing the initrd.

> Intentionally using pages overlapping the initrd is a recipe for problems...

> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[william.helsby at stfc.ac.uk]

The option of moving the initrd code later (after 	early_init_fdt_reserve_self(); 	early_init_fdt_scan_reserved_mem() ) 
was tested.
This resulted in the initrd being disabled because early_init_fdt_scan_reserved_mem
Has already reserved the initrd area.
With memblock=debug
Machine model: Xspress3 Mini in PicoZed Developement Board
bootconsole [earlycon0] enabled
memblock_add: [0x00000000000000-0x0000000fffffff] flags 0x0 arm_add_
memory+0x14c/0x174
memblock_reserve: [0x00000000100000-0x00000000b71de7] flags 0x0 arm_
memblock_init+0x1c/0x284
memblock_reserve: [0x00000000004000-0x00000000007fff] flags 0x0 arm_
memblock_init+0x20/0x284
memblock_reserve: [0x00000000000000-0x00000000003fff] flags 0x0 arm_
memblock_init+0x30/0x284
memblock_reserve: [0x0000000fa5f000-0x0000000fa6207f] flags 0x0 arm_
memblock_init+0x38/0x284
memblock_reserve: [0x0000000fa5f000-0x0000000fa61fff] flags 0x0 early_
init_fdt_scan_reserved_mem+0x54/0x78
memblock_reserve: [0x0000000fa65000-0x0000000ffff46a] flags 0x0 early_
init_fdt_scan_reserved_mem+0x54/0x78
memblock_reserve: [0x0000000e800000-0x0000000f7fffff] flags 0x0 membl
ock_alloc_range_nid+0x3c/0x50
cma: Reserved 16 MiB at 0x0e800000
INITRD: 0x0fa65000+0x0059a46b overlaps in-use memory region - disabling initrd
MEMBLOCK configuration:

With the initrd code back to its normal place
memblock_add: [0x00000000000000-0x0000000fffffff] flags 0x0 arm_add_
memory+0x14c/0x174
memblock_reserve: [0x00000000100000-0x00000000b71de7] flags 0x0 arm_m
emblock_init+0x24/0x284
memblock_reserve: [0x0000000fa65000-0x0000000fffffff] flags 0x0 arm_me
mblock_init+0x1e4/0x284
memblock_reserve: [0x00000000004000-0x00000000007fff] flags 0x0 arm_me
mblock_init+0x210/0x284
memblock_reserve: [0x00000000000000-0x00000000003fff] flags 0x0 arm_me
mblock_init+0x220/0x284
memblock_reserve: [0x0000000fa5f000-0x0000000fa6207f] flags 0x0 arm_me
mblock_init+0x224/0x284
memblock_reserve: [0x0000000fa5f000-0x0000000fa61fff] flags 0x0 early_ini
t_fdt_scan_reserved_mem+0x54/0x78
memblock_reserve: [0x0000000fa65000-0x0000000ffff46a] flags 0x0 early_in
it_fdt_scan_reserved_mem+0x54/0x78
memblock_reserve: [0x0000000e800000-0x0000000f7fffff] flags 0x0 memblo
ck_alloc_range_nid+0x3c/0x50
cma: Reserved 16 MiB at 0x0e800000
MEMBLOCK configuration:
 memory size = 0x10000000 reserved size = 0x2017e68
 memory.cnt  = 0x1
 memory[0x0]    [0x00000000000000-0x0000000fffffff], 0x10000000 bytes f
lags: 0x0
 reserved.cnt  = 0x5
 reserved[0x0]  [0x00000000000000-0x00000000007fff], 0x8000 bytes fl
ags: 0x0
 reserved[0x1]  [0x00000000100000-0x00000000b71de7], 0xa71de8 bytes fl
ags: 0x0
 reserved[0x2]  [0x0000000e800000-0x0000000f7fffff], 0x1000000 bytes flags: 0x0
 reserved[0x3]  [0x0000000fa5f000-0x0000000fa6207f], 0x3080 bytes flag
s: 0x0
 reserved[0x4]  [0x0000000fa65000-0x0000000fffffff], 0x59b000 bytes fl
ags: 0x0
Memory policy: Data cache writealloc

So poison the memory all the way from the start to end I would now suggest:
--- ../linux-xlnx-4.6.0-orig/arch/arm/mm/init.c 2016-11-15 15:10:44.476001509 +0000
+++ arch/arm/mm/init.c  2016-11-15 14:05:19.640969244 +0000
@@ -49,6 +49,8 @@

 static phys_addr_t phys_initrd_start __initdata = 0;
 static unsigned long phys_initrd_size __initdata = 0;
+static unsigned long initrd_reservation_start __initdata = 0;
+static unsigned long initrd_reservation_end __initdata = 0;

 static int __init early_initrd(char *p)
 {
@@ -255,11 +257,38 @@
                phys_initrd_start = phys_initrd_size = 0;
        }
        if (phys_initrd_size) {
-               memblock_reserve(phys_initrd_start, phys_initrd_size);
+           /* try to round the initrd start and end down and up to page boundaries,
+              so when freed, whole pages can be released.
+              However the initrd image may be adjacent to something else, so check that this rounding is OK
+           */
+         /* First try rounding the start down and end up */
+           phys_addr_t phys_initrd_reservation_start = phys_initrd_start & PAGE_MASK;
+            unsigned long size_to_reserve = PAGE_ALIGN(phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+           if (!memblock_is_region_memory(phys_initrd_reservation_start, size_to_reserve) ||
+                 memblock_is_region_reserved(phys_initrd_reservation_start, size_to_reserve)) {
+             /* This either does fit or overlaps something else, so try just rounding end up */
+             phys_initrd_reservation_start = phys_initrd_start;
+             size_to_reserve = PAGE_ALIGN(phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+             if (!memblock_is_region_memory(phys_initrd_reservation_start, size_to_reserve) ||
+                 memblock_is_region_reserved(phys_initrd_reservation_start, size_to_reserve)) {
+               /* This either does not fit or overlaps something else, so try just rounding start down */
+               phys_initrd_reservation_start = phys_initrd_start & PAGE_MASK;
+               size_to_reserve = (phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+               if (!memblock_is_region_memory(phys_initrd_reservation_start, size_to_reserve) ||
+                   memblock_is_region_reserved(phys_initrd_reservation_start, size_to_reserve)) {
+                 /* This either does not fit or overlaps something else, so do not round at all */
+                 phys_initrd_reservation_start = phys_initrd_start;
+                 size_to_reserve = (phys_initrd_start+phys_initrd_size) - phys_initrd_reservation_start;
+               }
+             }
+           }
+               memblock_reserve(phys_initrd_reservation_start, size_to_reserve);

                /* Now convert initrd to virtual addresses */
                initrd_start = __phys_to_virt(phys_initrd_start);
                initrd_end = initrd_start + phys_initrd_size;
+               initrd_reservation_start = __phys_to_virt(phys_initrd_reservation_start);
+               initrd_reservation_end = initrd_reservation_start + size_to_reserve;
        }
 #endif

@@ -325,7 +354,12 @@
  */
 static inline void poison_init_mem(void *s, size_t count)
 {
-       u32 *p = (u32 *)s;
+       u32 *p;
+       unsigned long start = (unsigned long)s, start_align;
+       start_align = (start+3) & ~3L;   // Round up to 32 bit word boundary.
+       count -= (start_align-start);
+       p = (u32 *)start_align;
+       count &= ~3L;
        for (; count != 0; count -= 4)
                *p++ = 0xe7fddef0;
 }
@@ -771,11 +805,11 @@
 {
        if (!keep_initrd) {
                if (start == initrd_start)
-                       start = round_down(start, PAGE_SIZE);
+                 start = initrd_reservation_start;
                if (end == initrd_end)
-                       end = round_up(end, PAGE_SIZE);
+                 end = initrd_reservation_end;

-               poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
+               poison_init_mem((void *)start, end-start);
                free_reserved_area((void *)start, (void *)end, -1, "initrd");
        }
 }

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[Russell King - ARM Linux]

On Tue, Nov 15, 2016 at 03:45:55PM +0000, william.helsby at stfc.ac.uk wrote:
> The option of moving the initrd code later (after
> 	early_init_fdt_reserve_self();
> 	early_init_fdt_scan_reserved_mem() ) 
> was tested.

Moving stuff around tends to break...

I'd prefer to do it this way, as it's much more readable (and we prize
readability... see Documentation/CodingStyle.)

 arch/arm/mm/init.c | 71 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581aeb871..ffae20b25929 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -227,6 +227,48 @@ phys_addr_t __init arm_memblock_steal(phys_addr_t size, phys_addr_t align)
 	return phys;
 }
 
+static void __init arm_initrd_init(void)
+{
+#ifdef CONFIG_BLK_DEV_INITRD
+	unsigned long size;
+	phys_addr_t start;
+
+	/* FDT scan will populate initrd_start */
+	if (initrd_start && !phys_initrd_size) {
+		phys_initrd_start = __virt_to_phys(initrd_start);
+		phys_initrd_size = initrd_end - initrd_start;
+	}
+
+	initrd_start = initrd_end = 0;
+
+	if (!phys_initrd_size)
+		return;
+
+	start = phys_initrd_start & PAGE_MASK;
+	size = PAGE_ALIGN(phys_initrd_size + phys_initrd_start - start);
+
+	if (!memblock_is_region_memory(start, size)) {
+		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
+		       (u64)start, size);
+		phys_initrd_start = phys_initrd_size = 0;
+		return;
+	}
+
+	if (memblock_is_region_reserved(start, size)) {
+		pr_err("INITRD: 0x%08llx+0x%08lx overlaps in-use memory region - disabling initrd\n",
+		       (u64)start, size);
+		phys_initrd_start = phys_initrd_size = 0;
+		return;
+	}
+
+	memblock_reserve(start, size);
+
+	/* Now convert initrd to virtual addresses */
+	initrd_start = __phys_to_virt(phys_initrd_start);
+	initrd_end = initrd_start + phys_initrd_size;
+#endif
+}
+
 void __init arm_memblock_init(const struct machine_desc *mdesc)
 {
 	/* Register the kernel text, kernel data and initrd with memblock. */
@@ -235,34 +277,7 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
 #else
 	memblock_reserve(__pa(_stext), _end - _stext);
 #endif
-#ifdef CONFIG_BLK_DEV_INITRD
-	/* FDT scan will populate initrd_start */
-	if (initrd_start && !phys_initrd_size) {
-		phys_initrd_start = __virt_to_phys(initrd_start);
-		phys_initrd_size = initrd_end - initrd_start;
-	}
-	initrd_start = initrd_end = 0;
-	if (phys_initrd_size &&
-	    !memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
-		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
-		       (u64)phys_initrd_start, phys_initrd_size);
-		phys_initrd_start = phys_initrd_size = 0;
-	}
-	if (phys_initrd_size &&
-	    memblock_is_region_reserved(phys_initrd_start, phys_initrd_size)) {
-		pr_err("INITRD: 0x%08llx+0x%08lx overlaps in-use memory region - disabling initrd\n",
-		       (u64)phys_initrd_start, phys_initrd_size);
-		phys_initrd_start = phys_initrd_size = 0;
-	}
-	if (phys_initrd_size) {
-		memblock_reserve(phys_initrd_start, phys_initrd_size);
-
-		/* Now convert initrd to virtual addresses */
-		initrd_start = __phys_to_virt(phys_initrd_start);
-		initrd_end = initrd_start + phys_initrd_size;
-	}
-#endif
-
+	arm_initrd_init();
 	arm_mm_memblock_reserve();
 
 	/* reserve any platform specific memblock areas */

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[william.helsby at stfc.ac.uk]

My understanding of your proposed patch is that it rounds the area to be reserved for the initrd image to page
boundaries. It then checks that this rounding does not grow the initrd to overlap other areas.

If it does not overlap, all is well. The kernel will not kmalloc anything within the partial pages 
at the beginning and end of the initrd image. When the initrd image is freed, complete pages are 
released and there is no wasted memory.
However, if it does overlap other regions, it disables the initrd image and the kernel panics with no root file system.

My patch rounds the area to be reserved for the initrd image to page boundaries. 
It then checks that this rounding does not grow the initrd to overlap other areas.
If it does not overlap the system initialised the ramdisk from the initrd image and then frees the initrd area
with no wasted memory. All is good.
If it does overlap, my code tried to determine the cause of the overlap and fell back to, in the worst case,
just reserving the space actually occupied by the initrd image. In this case the kernel would start, 
initialise the ramdisk and try to free the initrd image. At worst 2 partial pages at the beginning and end of 
the initrd image would not be freed, wasting up to 8kBytes. 
To me this is a more acceptable fallback than deleting the root file system.

Also not that in the current code free_initrd_mem checks:
if (end == initrd_end)
                        end = round_up(end, PAGE_SIZE);
So only rounds up the end if the whole initrd image is being freed. 
If free_initrd_mem is called with end != initrd_end (because the crash kernel area overlaps), 
then the end is not rounded up. 
However:
poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
will still page align the end address and could potentially overwrite the crash kernel area.
I don't know anything about if and when the crash kernel area is used, but in principle this may be a bad thing.

William.

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at armlinux.org.uk] 
Sent: 16 November 2016 23:48
To: Helsby, William (STFC,DL,TECH)
Cc: linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

On Tue, Nov 15, 2016 at 03:45:55PM +0000, william.helsby at stfc.ac.uk wrote:
> The option of moving the initrd code later (after
> 	early_init_fdt_reserve_self();
> 	early_init_fdt_scan_reserved_mem() ) was tested.

Moving stuff around tends to break...

I'd prefer to do it this way, as it's much more readable (and we prize readability... see Documentation/CodingStyle.)

 arch/arm/mm/init.c | 71 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index 370581aeb871..ffae20b25929 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -227,6 +227,48 @@ phys_addr_t __init arm_memblock_steal(phys_addr_t size, phys_addr_t align)
 	return phys;
 }
 
+static void __init arm_initrd_init(void) { #ifdef CONFIG_BLK_DEV_INITRD
+	unsigned long size;
+	phys_addr_t start;
+
+	/* FDT scan will populate initrd_start */
+	if (initrd_start && !phys_initrd_size) {
+		phys_initrd_start = __virt_to_phys(initrd_start);
+		phys_initrd_size = initrd_end - initrd_start;
+	}
+
+	initrd_start = initrd_end = 0;
+
+	if (!phys_initrd_size)
+		return;
+
+	start = phys_initrd_start & PAGE_MASK;
+	size = PAGE_ALIGN(phys_initrd_size + phys_initrd_start - start);
+
+	if (!memblock_is_region_memory(start, size)) {
+		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
+		       (u64)start, size);
+		phys_initrd_start = phys_initrd_size = 0;
+		return;
+	}
+
+	if (memblock_is_region_reserved(start, size)) {
+		pr_err("INITRD: 0x%08llx+0x%08lx overlaps in-use memory region - disabling initrd\n",
+		       (u64)start, size);
+		phys_initrd_start = phys_initrd_size = 0;
+		return;
+	}
+
+	memblock_reserve(start, size);
+
+	/* Now convert initrd to virtual addresses */
+	initrd_start = __phys_to_virt(phys_initrd_start);
+	initrd_end = initrd_start + phys_initrd_size; #endif }
+
 void __init arm_memblock_init(const struct machine_desc *mdesc)  {
 	/* Register the kernel text, kernel data and initrd with memblock. */ @@ -235,34 +277,7 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)  #else
 	memblock_reserve(__pa(_stext), _end - _stext);  #endif -#ifdef CONFIG_BLK_DEV_INITRD
-	/* FDT scan will populate initrd_start */
-	if (initrd_start && !phys_initrd_size) {
-		phys_initrd_start = __virt_to_phys(initrd_start);
-		phys_initrd_size = initrd_end - initrd_start;
-	}
-	initrd_start = initrd_end = 0;
-	if (phys_initrd_size &&
-	    !memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
-		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
-		       (u64)phys_initrd_start, phys_initrd_size);
-		phys_initrd_start = phys_initrd_size = 0;
-	}
-	if (phys_initrd_size &&
-	    memblock_is_region_reserved(phys_initrd_start, phys_initrd_size)) {
-		pr_err("INITRD: 0x%08llx+0x%08lx overlaps in-use memory region - disabling initrd\n",
-		       (u64)phys_initrd_start, phys_initrd_size);
-		phys_initrd_start = phys_initrd_size = 0;
-	}
-	if (phys_initrd_size) {
-		memblock_reserve(phys_initrd_start, phys_initrd_size);
-
-		/* Now convert initrd to virtual addresses */
-		initrd_start = __phys_to_virt(phys_initrd_start);
-		initrd_end = initrd_start + phys_initrd_size;
-	}
-#endif
-
+	arm_initrd_init();
 	arm_mm_memblock_reserve();
 
 	/* reserve any platform specific memblock areas */

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.

[PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.

[Russell King - ARM Linux]

On Thu, Nov 17, 2016 at 10:41:19AM +0000, william.helsby at stfc.ac.uk wrote:
> My understanding of your proposed patch is that it rounds the area to be reserved for the initrd image to page
> boundaries. It then checks that this rounding does not grow the initrd to overlap other areas.
> 
> If it does not overlap, all is well. The kernel will not kmalloc anything within the partial pages 
> at the beginning and end of the initrd image. When the initrd image is freed, complete pages are 
> released and there is no wasted memory.
> However, if it does overlap other regions, it disables the initrd image and the kernel panics with no root file system.

As best I can do when replying to this... (due to the stupidly long lines)...

Yes, that's what will happen.  The kernel memory allocator deals with
things on a per-page basis, and that's not going to change.  If someone
wants to place objects on a non-page aligned basis into memory, then
they're intentionally making life hard for the kernel.

You talk about kmalloc() above, but kmalloc() is not really relevant
here - kmalloc() should be thought as a sub-page memory allocator (even
though it's capable of larger allocations.)  It sources its memory from
page_alloc(), which not surprisingly deals with page-sized objects, and
those are either "in-use" or "free".  There's no finer granularity there.

So, if you have a page partially in-use, as far as the kernel's normal
allocators are concerned, the whole page is in use.

Boot time is slightly different until the normal allocators are
initialised - we have memblock, which doesn't really know about stuff
like pages.  It only knows about ranges of addresses.  However, during
initialisation, when memblock hands over to the normal allocators, any
page that is partially reserved is treated as wholely reserved because
- as I say above - the normal allocators can't deal with that level of
information.

Rounding the initrd up/down when reserving it in memblock is the right
thing to do to ensure that when memory is requested from memblock,
memblock doesn't allocate the overlapping pages - because we want to
free that memory later.

So, I don't think your idea of trying to cover all the cases of part-used
pages is really worth it - and the boot loader has to understand that the
kernel operates on a page-basis, and expects objects to be aligned to the
size of a page.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.