[PATCH] ARM: poison initmem when it is freed

[PATCH] ARM: poison initmem when it is freed

[Russell King - ARM Linux]

When the initmem is freed, we can no longer rely on its contents.  In
lightly loaded systems, this memory may persist for some time, making
it harder discover run-time issues (caused by the build warnings being
ignored.)

Poison the initmem at the point where it is freed to encourage run-time
problems when initmem is dereferenced as an aid to finding such problems.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mm/init.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 2c2cce9..46c39bd 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -19,6 +19,7 @@
 #include <linux/highmem.h>
 #include <linux/gfp.h>
 #include <linux/memblock.h>
+#include <linux/poison.h>
 #include <linux/sort.h>
 
 #include <asm/mach-types.h>
@@ -701,6 +702,8 @@ void free_initmem(void)
 				    "TCM link");
 #endif
 
+	memset(__init_begin, POISON_FREE_INITMEM, __init_end - __init_begin);
+
 	if (!machine_is_integrator() && !machine_is_cintegrator())
 		totalram_pages += free_area(__phys_to_pfn(__pa(__init_begin)),
 					    __phys_to_pfn(__pa(__init_end)),

[PATCH] ARM: poison initmem when it is freed

[Nicolas Pitre]

On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:

> When the initmem is freed, we can no longer rely on its contents.  In
> lightly loaded systems, this memory may persist for some time, making
> it harder discover run-time issues (caused by the build warnings being
> ignored.)
> 
> Poison the initmem at the point where it is freed to encourage run-time
> problems when initmem is dereferenced as an aid to finding such problems.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

The default poison doesn't appear to be a judicious choice for ARM.

include/linux/poison.h:#define POISON_FREE_INITMEM      0xcc

   0:   cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc

So if the gt condition is false this will execute nops until it falls 
out of the initmem section.  Would be nicer if a fault could be 
generated right at the accessed address which could be looked up.


Nicolas

[PATCH] ARM: poison initmem when it is freed

[Russell King - ARM Linux]

On Tue, Jul 05, 2011 at 03:17:33PM -0400, Nicolas Pitre wrote:
> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
> 
> > When the initmem is freed, we can no longer rely on its contents.  In
> > lightly loaded systems, this memory may persist for some time, making
> > it harder discover run-time issues (caused by the build warnings being
> > ignored.)
> > 
> > Poison the initmem at the point where it is freed to encourage run-time
> > problems when initmem is dereferenced as an aid to finding such problems.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> The default poison doesn't appear to be a judicious choice for ARM.
> 
> include/linux/poison.h:#define POISON_FREE_INITMEM      0xcc
> 
>    0:   cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
> 
> So if the gt condition is false this will execute nops until it falls 
> out of the initmem section.  Would be nicer if a fault could be 
> generated right at the accessed address which could be looked up.

Have you tried to find a byte-based poison value which would fault
yet still cause a pointer dereference?  You're limited to 0xeN on
ARM, of which there's almost nothing to chose from:

   0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
   4:   e1e1e1e1        mvn     lr, r1, ror #3
   8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
   c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
  10:   e4e4e4e4        strbt   lr, [r4], #1252
  14:   e5e5e5e5        strb    lr, [r5, #1509]!
  18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
  1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
  20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
  24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
  28:   eaeaeaea        b       0xffababd8
  2c:   ebebebeb        bl      0xffafafe0
  30:   ecececec        stcl    12, cr14, [ip], #944
  34:   edededed        stcl    13, cr14, [sp, #948]!
  38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
  3c:   efefefef        svc     0x00efefef

0xefefefef looks to be about the best alternative.

It then brings up whether POISON_FREE_INITMEM should be changed or not,
as 0xcc is the expected value for this at the moment.

[PATCH] ARM: poison initmem when it is freed

[Nicolas Pitre]

On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:

> On Tue, Jul 05, 2011 at 03:17:33PM -0400, Nicolas Pitre wrote:
> > On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
> > 
> > > When the initmem is freed, we can no longer rely on its contents.  In
> > > lightly loaded systems, this memory may persist for some time, making
> > > it harder discover run-time issues (caused by the build warnings being
> > > ignored.)
> > > 
> > > Poison the initmem at the point where it is freed to encourage run-time
> > > problems when initmem is dereferenced as an aid to finding such problems.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > 
> > The default poison doesn't appear to be a judicious choice for ARM.
> > 
> > include/linux/poison.h:#define POISON_FREE_INITMEM      0xcc
> > 
> >    0:   cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
> > 
> > So if the gt condition is false this will execute nops until it falls 
> > out of the initmem section.  Would be nicer if a fault could be 
> > generated right at the accessed address which could be looked up.
> 
> Have you tried to find a byte-based poison value which would fault
> yet still cause a pointer dereference?  You're limited to 0xeN on
> ARM, of which there's almost nothing to chose from:
> 
>    0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
>    4:   e1e1e1e1        mvn     lr, r1, ror #3
>    8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
>    c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
>   10:   e4e4e4e4        strbt   lr, [r4], #1252
>   14:   e5e5e5e5        strb    lr, [r5, #1509]!
>   18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
>   1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
>   20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
>   24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
>   28:   eaeaeaea        b       0xffababd8
>   2c:   ebebebeb        bl      0xffafafe0
>   30:   ecececec        stcl    12, cr14, [ip], #944
>   34:   edededed        stcl    13, cr14, [sp, #948]!
>   38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
>   3c:   efefefef        svc     0x00efefef
> 
> 0xefefefef looks to be about the best alternative.

Right.  Does it have to be a byte?  Having a word (or half-word if 
Thumb2) would be much more convenient.

> It then brings up whether POISON_FREE_INITMEM should be changed or not,
> as 0xcc is the expected value for this at the moment.

I would think that this should be a per architecture value to actually 
be useful.


Nicolas

[PATCH] ARM: poison initmem when it is freed

[Stephen Boyd]

On 07/05/2011 12:48 PM, Nicolas Pitre wrote:
> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
>
>> On Tue, Jul 05, 2011 at 03:17:33PM -0400, Nicolas Pitre wrote:
>>> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
>>>
>>>> When the initmem is freed, we can no longer rely on its contents.  In
>>>> lightly loaded systems, this memory may persist for some time, making
>>>> it harder discover run-time issues (caused by the build warnings being
>>>> ignored.)
>>>>
>>>> Poison the initmem at the point where it is freed to encourage run-time
>>>> problems when initmem is dereferenced as an aid to finding such problems.
>>>>
>>>> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>>> The default poison doesn't appear to be a judicious choice for ARM.
>>>
>>> include/linux/poison.h:#define POISON_FREE_INITMEM      0xcc
>>>
>>>    0:   cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
>>>
>>> So if the gt condition is false this will execute nops until it falls 
>>> out of the initmem section.  Would be nicer if a fault could be 
>>> generated right at the accessed address which could be looked up.
>> Have you tried to find a byte-based poison value which would fault
>> yet still cause a pointer dereference?  You're limited to 0xeN on
>> ARM, of which there's almost nothing to chose from:
>>
>>    0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
>>    4:   e1e1e1e1        mvn     lr, r1, ror #3
>>    8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
>>    c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
>>   10:   e4e4e4e4        strbt   lr, [r4], #1252
>>   14:   e5e5e5e5        strb    lr, [r5, #1509]!
>>   18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
>>   1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
>>   20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
>>   24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
>>   28:   eaeaeaea        b       0xffababd8
>>   2c:   ebebebeb        bl      0xffafafe0
>>   30:   ecececec        stcl    12, cr14, [ip], #944
>>   34:   edededed        stcl    13, cr14, [sp, #948]!
>>   38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
>>   3c:   efefefef        svc     0x00efefef
>>
>> 0xefefefef looks to be about the best alternative.
> Right.  Does it have to be a byte?  Having a word (or half-word if 
> Thumb2) would be much more convenient.
>
>> It then brings up whether POISON_FREE_INITMEM should be changed or not,
>> as 0xcc is the expected value for this at the moment.
> I would think that this should be a per architecture value to actually 
> be useful.
>

Didn't I already post this patch about 6 months ago?

https://lkml.org/lkml/2011/1/11/1

Here it is, the only downside I see is the memset isn't really efficient
as the assembler optimized one.

------8<------->8-------

Subject: [PATCH] arm: mm: Poison freed init memory

Poisoning __init marked memory can be useful when tracking down
obscure memory corruption bugs. Therefore, poison init memory
with 0xe7fddef0 to catch bugs earlier. The poison value is an
undefined instruction in ARM mode and branch to an undefined
instruction in Thumb mode.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/mm/init.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index cddd684..8b9d678 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -364,7 +364,8 @@ void __init bootmem_init(void)
 	max_pfn = max_high - PHYS_PFN_OFFSET;
 }
 
-static inline int free_area(unsigned long pfn, unsigned long end, char *s)
+static inline int free_area(unsigned long pfn, unsigned long end, char *s,
+		bool init_mem)
 {
 	unsigned int pages = 0, size = (end - pfn) << (PAGE_SHIFT - 10);
 
@@ -372,6 +373,14 @@ static inline int free_area(unsigned long pfn, unsigned long end, char *s)
 		struct page *page = pfn_to_page(pfn);
 		ClearPageReserved(page);
 		init_page_count(page);
+		if (init_mem) {
+			u32 *mem = __va(__pfn_to_phys(pfn));
+			u32 *end = (void *)mem + PAGE_SIZE;
+
+			do {
+				*mem++ = 0xe7fddef0;
+			} while (mem < end);
+		}
 		__free_page(page);
 		pages++;
 	}
@@ -478,7 +487,7 @@ static void __init free_highpages(void)
 				res_end = end;
 			if (res_start != start)
 				totalhigh_pages += free_area(start, res_start,
-							     NULL);
+							     NULL, false);
 			start = res_end;
 			if (start == end)
 				break;
@@ -486,7 +495,7 @@ static void __init free_highpages(void)
 
 		/* And now free anything which remains */
 		if (start < end)
-			totalhigh_pages += free_area(start, end, NULL);
+			totalhigh_pages += free_area(start, end, NULL, false);
 	}
 	totalram_pages += totalhigh_pages;
 #endif
@@ -518,7 +527,8 @@ void __init mem_init(void)
 #ifdef CONFIG_SA1111
 	/* now that our DMA memory is actually so designated, we can free it */
 	totalram_pages += free_area(PHYS_PFN_OFFSET,
-				    __phys_to_pfn(__pa(swapper_pg_dir)), NULL);
+				    __phys_to_pfn(__pa(swapper_pg_dir)), NULL,
+				    false);
 #endif
 
 	free_highpages();
@@ -650,13 +660,13 @@ void free_initmem(void)
 
 	totalram_pages += free_area(__phys_to_pfn(__pa(&__tcm_start)),
 				    __phys_to_pfn(__pa(&__tcm_end)),
-				    "TCM link");
+				    "TCM link", true);
 #endif
 
 	if (!machine_is_integrator() && !machine_is_cintegrator())
 		totalram_pages += free_area(__phys_to_pfn(__pa(__init_begin)),
 					    __phys_to_pfn(__pa(__init_end)),
-					    "init");
+					    "init", true);
 }
 
 #ifdef CONFIG_BLK_DEV_INITRD
@@ -668,7 +678,7 @@ void free_initrd_mem(unsigned long start, unsigned long end)
 	if (!keep_initrd)
 		totalram_pages += free_area(__phys_to_pfn(__pa(start)),
 					    __phys_to_pfn(__pa(end)),
-					    "initrd");
+					    "initrd", true);
 }
 
 static int __init keepinitrd_setup(char *__unused)
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: poison initmem when it is freed

[Tixy]

On Tue, 2011-07-05 at 15:48 -0400, Nicolas Pitre wrote:
> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
> > Have you tried to find a byte-based poison value which would fault
> > yet still cause a pointer dereference?  You're limited to 0xeN on
> > ARM, of which there's almost nothing to chose from:
> > 
> >    0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
> >    4:   e1e1e1e1        mvn     lr, r1, ror #3
> >    8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
> >    c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
> >   10:   e4e4e4e4        strbt   lr, [r4], #1252
> >   14:   e5e5e5e5        strb    lr, [r5, #1509]!
> >   18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
> >   1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
> >   20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
> >   24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
> >   28:   eaeaeaea        b       0xffababd8
> >   2c:   ebebebeb        bl      0xffafafe0
> >   30:   ecececec        stcl    12, cr14, [ip], #944
> >   34:   edededed        stcl    13, cr14, [sp, #948]!
> >   38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
> >   3c:   efefefef        svc     0x00efefef
> > 
> > 0xefefefef looks to be about the best alternative.
> 
> Right.  Does it have to be a byte?  Having a word (or half-word if 
> Thumb2) would be much more convenient.

For Thumb, 0xde?? is Permanently UNDEFINED, so we could have 0xdede for
a single byte pattern or an even more descriptive 0xdead if we don't
have that restriction.

-- 
Tixy

[PATCH] ARM: poison initmem when it is freed

[Russell King - ARM Linux]

On Tue, Jul 05, 2011 at 04:34:39PM -0700, Stephen Boyd wrote:
> On 07/05/2011 12:48 PM, Nicolas Pitre wrote:
> > On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
> >
> >> On Tue, Jul 05, 2011 at 03:17:33PM -0400, Nicolas Pitre wrote:
> >>> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
> >>>
> >>>> When the initmem is freed, we can no longer rely on its contents.  In
> >>>> lightly loaded systems, this memory may persist for some time, making
> >>>> it harder discover run-time issues (caused by the build warnings being
> >>>> ignored.)
> >>>>
> >>>> Poison the initmem at the point where it is freed to encourage run-time
> >>>> problems when initmem is dereferenced as an aid to finding such problems.
> >>>>
> >>>> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> >>> The default poison doesn't appear to be a judicious choice for ARM.
> >>>
> >>> include/linux/poison.h:#define POISON_FREE_INITMEM      0xcc
> >>>
> >>>    0:   cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
> >>>
> >>> So if the gt condition is false this will execute nops until it falls 
> >>> out of the initmem section.  Would be nicer if a fault could be 
> >>> generated right at the accessed address which could be looked up.
> >> Have you tried to find a byte-based poison value which would fault
> >> yet still cause a pointer dereference?  You're limited to 0xeN on
> >> ARM, of which there's almost nothing to chose from:
> >>
> >>    0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
> >>    4:   e1e1e1e1        mvn     lr, r1, ror #3
> >>    8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
> >>    c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
> >>   10:   e4e4e4e4        strbt   lr, [r4], #1252
> >>   14:   e5e5e5e5        strb    lr, [r5, #1509]!
> >>   18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
> >>   1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
> >>   20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
> >>   24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
> >>   28:   eaeaeaea        b       0xffababd8
> >>   2c:   ebebebeb        bl      0xffafafe0
> >>   30:   ecececec        stcl    12, cr14, [ip], #944
> >>   34:   edededed        stcl    13, cr14, [sp, #948]!
> >>   38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
> >>   3c:   efefefef        svc     0x00efefef
> >>
> >> 0xefefefef looks to be about the best alternative.
> > Right.  Does it have to be a byte?  Having a word (or half-word if 
> > Thumb2) would be much more convenient.
> >
> >> It then brings up whether POISON_FREE_INITMEM should be changed or not,
> >> as 0xcc is the expected value for this at the moment.
> > I would think that this should be a per architecture value to actually 
> > be useful.
> >
> 
> Didn't I already post this patch about 6 months ago?
> 
> https://lkml.org/lkml/2011/1/11/1
> 
> Here it is, the only downside I see is the memset isn't really efficient
> as the assembler optimized one.

Ok, let's do it your way...

But, do we need to do it page by page?  Can we not have a function which
does the poisioning, and we just pass the __init_begin/__init_end and tcm
start/end stuff to?

[PATCH] ARM: poison initmem when it is freed

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 10:08:20AM +0100, Tixy wrote:
> On Tue, 2011-07-05 at 15:48 -0400, Nicolas Pitre wrote:
> > On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
> > > Have you tried to find a byte-based poison value which would fault
> > > yet still cause a pointer dereference?  You're limited to 0xeN on
> > > ARM, of which there's almost nothing to chose from:
> > > 
> > >    0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
> > >    4:   e1e1e1e1        mvn     lr, r1, ror #3
> > >    8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
> > >    c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
> > >   10:   e4e4e4e4        strbt   lr, [r4], #1252
> > >   14:   e5e5e5e5        strb    lr, [r5, #1509]!
> > >   18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
> > >   1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
> > >   20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
> > >   24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
> > >   28:   eaeaeaea        b       0xffababd8
> > >   2c:   ebebebeb        bl      0xffafafe0
> > >   30:   ecececec        stcl    12, cr14, [ip], #944
> > >   34:   edededed        stcl    13, cr14, [sp, #948]!
> > >   38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
> > >   3c:   efefefef        svc     0x00efefef
> > > 
> > > 0xefefefef looks to be about the best alternative.
> > 
> > Right.  Does it have to be a byte?  Having a word (or half-word if 
> > Thumb2) would be much more convenient.
> 
> For Thumb, 0xde?? is Permanently UNDEFINED, so we could have 0xdede for
> a single byte pattern or an even more descriptive 0xdead if we don't
> have that restriction.

Works for Thumb but not ARM.  For ARM it needs to be 0xeN.

Stephen Boyd's patch results in a 32-bit value which will fault as
an instruction in both ARM and Thumb modes, so that sounds like the
best solution.

[PATCH] ARM: poison initmem when it is freed

[Stephen Boyd]

On 07/06/2011 01:34 PM, Russell King - ARM Linux wrote:
> On Tue, Jul 05, 2011 at 04:34:39PM -0700, Stephen Boyd wrote:
>> On 07/05/2011 12:48 PM, Nicolas Pitre wrote:
>>> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
>>>
>>>> On Tue, Jul 05, 2011 at 03:17:33PM -0400, Nicolas Pitre wrote:
>>>>> On Tue, 5 Jul 2011, Russell King - ARM Linux wrote:
>>>>>
>>>>>> When the initmem is freed, we can no longer rely on its contents.  In
>>>>>> lightly loaded systems, this memory may persist for some time, making
>>>>>> it harder discover run-time issues (caused by the build warnings being
>>>>>> ignored.)
>>>>>>
>>>>>> Poison the initmem at the point where it is freed to encourage run-time
>>>>>> problems when initmem is dereferenced as an aid to finding such problems.
>>>>>>
>>>>>> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>>>>> The default poison doesn't appear to be a judicious choice for ARM.
>>>>>
>>>>> include/linux/poison.h:#define POISON_FREE_INITMEM      0xcc
>>>>>
>>>>>    0:   cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
>>>>>
>>>>> So if the gt condition is false this will execute nops until it falls 
>>>>> out of the initmem section.  Would be nicer if a fault could be 
>>>>> generated right at the accessed address which could be looked up.
>>>> Have you tried to find a byte-based poison value which would fault
>>>> yet still cause a pointer dereference?  You're limited to 0xeN on
>>>> ARM, of which there's almost nothing to chose from:
>>>>
>>>>    0:   e0e0e0e0        rsc     lr, r0, r0, ror #1
>>>>    4:   e1e1e1e1        mvn     lr, r1, ror #3
>>>>    8:   e2e2e2e2        rsc     lr, r2, #536870926      ; 0x2000000e
>>>>    c:   e3e3e3e3        mvn     lr, #-1946157053        ; 0x8c000003
>>>>   10:   e4e4e4e4        strbt   lr, [r4], #1252
>>>>   14:   e5e5e5e5        strb    lr, [r5, #1509]!
>>>>   18:   e6e6e6e6        strbt   lr, [r6], r6, ror #13
>>>>   1c:   e7e7e7e7        strb    lr, [r7, r7, ror #15]!
>>>>   20:   e8e8e8e8        stmia   r8!, {r3, r5, r6, r7, fp, sp, lr, pc}^
>>>>   24:   e9e9e9e9        stmib   r9!, {r0, r3, r5, r6, r7, r8, fp, sp, lr, pc}^
>>>>   28:   eaeaeaea        b       0xffababd8
>>>>   2c:   ebebebeb        bl      0xffafafe0
>>>>   30:   ecececec        stcl    12, cr14, [ip], #944
>>>>   34:   edededed        stcl    13, cr14, [sp, #948]!
>>>>   38:   eeeeeeee        cdp     14, 14, cr14, cr14, cr14, {7}
>>>>   3c:   efefefef        svc     0x00efefef
>>>>
>>>> 0xefefefef looks to be about the best alternative.
>>> Right.  Does it have to be a byte?  Having a word (or half-word if 
>>> Thumb2) would be much more convenient.
>>>
>>>> It then brings up whether POISON_FREE_INITMEM should be changed or not,
>>>> as 0xcc is the expected value for this at the moment.
>>> I would think that this should be a per architecture value to actually 
>>> be useful.
>>>
>>
>> Didn't I already post this patch about 6 months ago?
>>
>> https://lkml.org/lkml/2011/1/11/1
>>
>> Here it is, the only downside I see is the memset isn't really efficient
>> as the assembler optimized one.
>
> Ok, let's do it your way...
>
> But, do we need to do it page by page?  Can we not have a function which
> does the poisioning, and we just pass the __init_begin/__init_end and tcm
> start/end stuff to?

Should it include the initrd too? At least x86 poisons that memory but I
don't know who would be using that incorrectly.

How about a free_init_area() function which calls free_area() after
poisoning the memory?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: poison initmem when it is freed

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 01:55:54PM -0700, Stephen Boyd wrote:
> Should it include the initrd too? At least x86 poisons that memory but I
> don't know who would be using that incorrectly.

It could do - I don't see any harm in not doing so.  The only issue
is that people may want to disable this stuff if they're after
squeezing every last ms out of the boot time.

> How about a free_init_area() function which calls free_area() after
> poisoning the memory?

I need to go back and look at the Integrator etc situation with regard
to reorganizing the vmlinux layout - it may be that the omission of
freeing .init memory can now be removed (it was there to stop the
memory being used as the first K of memory wasn't DMA-able.)

Assuming it has to stay though, we still should arrange for the initrd
memory to be poisoned even if it isn't freed.

[PATCH] ARM: poison initmem when it is freed

[Tim Bird]

On 07/06/2011 02:01 PM, Russell King - ARM Linux wrote:
> On Wed, Jul 06, 2011 at 01:55:54PM -0700, Stephen Boyd wrote:
>> Should it include the initrd too? At least x86 poisons that memory but I
>> don't know who would be using that incorrectly.
> 
> It could do - I don't see any harm in not doing so.  The only issue
> is that people may want to disable this stuff if they're after
> squeezing every last ms out of the boot time.

I'd appreciate that.  I think the ability to poison is nice.
But if it takes any time at all it would be nice to be
able to disable it to avoid an increase in boot time.
 -- Tim

=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================

[PATCHv2] arm: mm: Poison freed init memory

[Stephen Boyd]

Poisoning __init marked memory can be useful when tracking down
obscure memory corruption bugs. Therefore, poison init memory
with 0xe7fddef0 to catch bugs earlier. The poison value is an
undefined instruction in ARM mode and branch to an undefined
instruction in Thumb mode.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

On 7/6/2011 2:01 PM, Russell King - ARM Linux wrote:
> On Wed, Jul 06, 2011 at 01:55:54PM -0700, Stephen Boyd wrote:
>> Should it include the initrd too? At least x86 poisons that memory but I
>> don't know who would be using that incorrectly.
>
> It could do - I don't see any harm in not doing so.  The only issue
> is that people may want to disable this stuff if they're after
> squeezing every last ms out of the boot time.

I haven't done this. I hope a follow up patch will suffice.

>
>> How about a free_init_area() function which calls free_area() after
>> poisoning the memory?
>
> I need to go back and look at the Integrator etc situation with regard
> to reorganizing the vmlinux layout - it may be that the omission of
> freeing .init memory can now be removed (it was there to stop the
> memory being used as the first K of memory wasn't DMA-able.)
>
> Assuming it has to stay though, we still should arrange for the initrd
> memory to be poisoned even if it isn't freed.

Is this is patch what you're saying? I would have liked to do a
free_init_area() wrapper, but until the Integrator situation can be
sorted it doesn't look worthwhile.

 arch/arm/mm/init.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index c19571c..d6360b1 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -422,6 +422,17 @@ static inline int free_area(unsigned long pfn, unsigned long end, char *s)
 	return pages;
 }
 
+/*
+ * Poison init memory with an undefined instruction (ARM) or a branch to an
+ * undefined instruction (Thumb).
+ */
+static inline void poison_init_mem(void *s, size_t count)
+{
+	u32 *p = (u32 *)s;
+	while ((count = count - 4))
+		*p++ = 0xe7fddef0;
+}
+
 static inline void
 free_memmap(unsigned long start_pfn, unsigned long end_pfn)
 {
@@ -704,11 +715,13 @@ void free_initmem(void)
 #ifdef CONFIG_HAVE_TCM
 	extern char __tcm_start, __tcm_end;
 
+	poison_init_mem(&__tcm_start, &__tcm_end - &__tcm_start);
 	totalram_pages += free_area(__phys_to_pfn(__pa(&__tcm_start)),
 				    __phys_to_pfn(__pa(&__tcm_end)),
 				    "TCM link");
 #endif
 
+	poison_init_mem(__init_begin, __init_end - __init_begin);
 	if (!machine_is_integrator() && !machine_is_cintegrator())
 		totalram_pages += free_area(__phys_to_pfn(__pa(__init_begin)),
 					    __phys_to_pfn(__pa(__init_end)),
@@ -721,10 +734,12 @@ static int keep_initrd;
 
 void free_initrd_mem(unsigned long start, unsigned long end)
 {
-	if (!keep_initrd)
+	if (!keep_initrd) {
+		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
 		totalram_pages += free_area(__phys_to_pfn(__pa(start)),
 					    __phys_to_pfn(__pa(end)),
 					    "initrd");
+	}
 }
 
 static int __init keepinitrd_setup(char *__unused)
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCHv2] arm: mm: Poison freed init memory

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 09:47:27AM -0700, Stephen Boyd wrote:
> Poisoning __init marked memory can be useful when tracking down
> obscure memory corruption bugs. Therefore, poison init memory
> with 0xe7fddef0 to catch bugs earlier. The poison value is an
> undefined instruction in ARM mode and branch to an undefined
> instruction in Thumb mode.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> 
> On 7/6/2011 2:01 PM, Russell King - ARM Linux wrote:
> > On Wed, Jul 06, 2011 at 01:55:54PM -0700, Stephen Boyd wrote:
> >> Should it include the initrd too? At least x86 poisons that memory but I
> >> don't know who would be using that incorrectly.
> >
> > It could do - I don't see any harm in not doing so.  The only issue
> > is that people may want to disable this stuff if they're after
> > squeezing every last ms out of the boot time.
> 
> I haven't done this. I hope a follow up patch will suffice.
> 
> >
> >> How about a free_init_area() function which calls free_area() after
> >> poisoning the memory?
> >
> > I need to go back and look at the Integrator etc situation with regard
> > to reorganizing the vmlinux layout - it may be that the omission of
> > freeing .init memory can now be removed (it was there to stop the
> > memory being used as the first K of memory wasn't DMA-able.)
> >
> > Assuming it has to stay though, we still should arrange for the initrd
> > memory to be poisoned even if it isn't freed.
> 
> Is this is patch what you're saying? I would have liked to do a
> free_init_area() wrapper, but until the Integrator situation can be
> sorted it doesn't look worthwhile.

Yes, thanks.  This looks fine for the time being.  Have you been able
to test it?  If yes, then please put it in the patch system and I'll
see about giving it a test too.

[PATCHv2] arm: mm: Poison freed init memory

[Nicolas Pitre]

On Thu, 7 Jul 2011, Stephen Boyd wrote:

> Poisoning __init marked memory can be useful when tracking down
> obscure memory corruption bugs. Therefore, poison init memory
> with 0xe7fddef0 to catch bugs earlier. The poison value is an
> undefined instruction in ARM mode and branch to an undefined
> instruction in Thumb mode.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>



> ---
> 
> On 7/6/2011 2:01 PM, Russell King - ARM Linux wrote:
> > On Wed, Jul 06, 2011 at 01:55:54PM -0700, Stephen Boyd wrote:
> >> Should it include the initrd too? At least x86 poisons that memory but I
> >> don't know who would be using that incorrectly.
> >
> > It could do - I don't see any harm in not doing so.  The only issue
> > is that people may want to disable this stuff if they're after
> > squeezing every last ms out of the boot time.
> 
> I haven't done this. I hope a follow up patch will suffice.
> 
> >
> >> How about a free_init_area() function which calls free_area() after
> >> poisoning the memory?
> >
> > I need to go back and look at the Integrator etc situation with regard
> > to reorganizing the vmlinux layout - it may be that the omission of
> > freeing .init memory can now be removed (it was there to stop the
> > memory being used as the first K of memory wasn't DMA-able.)
> >
> > Assuming it has to stay though, we still should arrange for the initrd
> > memory to be poisoned even if it isn't freed.
> 
> Is this is patch what you're saying? I would have liked to do a
> free_init_area() wrapper, but until the Integrator situation can be
> sorted it doesn't look worthwhile.
> 
>  arch/arm/mm/init.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index c19571c..d6360b1 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -422,6 +422,17 @@ static inline int free_area(unsigned long pfn, unsigned long end, char *s)
>  	return pages;
>  }
>  
> +/*
> + * Poison init memory with an undefined instruction (ARM) or a branch to an
> + * undefined instruction (Thumb).
> + */
> +static inline void poison_init_mem(void *s, size_t count)
> +{
> +	u32 *p = (u32 *)s;
> +	while ((count = count - 4))
> +		*p++ = 0xe7fddef0;
> +}
> +
>  static inline void
>  free_memmap(unsigned long start_pfn, unsigned long end_pfn)
>  {
> @@ -704,11 +715,13 @@ void free_initmem(void)
>  #ifdef CONFIG_HAVE_TCM
>  	extern char __tcm_start, __tcm_end;
>  
> +	poison_init_mem(&__tcm_start, &__tcm_end - &__tcm_start);
>  	totalram_pages += free_area(__phys_to_pfn(__pa(&__tcm_start)),
>  				    __phys_to_pfn(__pa(&__tcm_end)),
>  				    "TCM link");
>  #endif
>  
> +	poison_init_mem(__init_begin, __init_end - __init_begin);
>  	if (!machine_is_integrator() && !machine_is_cintegrator())
>  		totalram_pages += free_area(__phys_to_pfn(__pa(__init_begin)),
>  					    __phys_to_pfn(__pa(__init_end)),
> @@ -721,10 +734,12 @@ static int keep_initrd;
>  
>  void free_initrd_mem(unsigned long start, unsigned long end)
>  {
> -	if (!keep_initrd)
> +	if (!keep_initrd) {
> +		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
>  		totalram_pages += free_area(__phys_to_pfn(__pa(start)),
>  					    __phys_to_pfn(__pa(end)),
>  					    "initrd");
> +	}
>  }
>  
>  static int __init keepinitrd_setup(char *__unused)
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCHv2] arm: mm: Poison freed init memory

[Stephen Boyd]

On 07/07/2011 10:36 AM, Russell King - ARM Linux wrote:
> On Thu, Jul 07, 2011 at 09:47:27AM -0700, Stephen Boyd wrote:
>> Poisoning __init marked memory can be useful when tracking down
>> obscure memory corruption bugs. Therefore, poison init memory
>> with 0xe7fddef0 to catch bugs earlier. The poison value is an
>> undefined instruction in ARM mode and branch to an undefined
>> instruction in Thumb mode.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>
>> On 7/6/2011 2:01 PM, Russell King - ARM Linux wrote:
>>> On Wed, Jul 06, 2011 at 01:55:54PM -0700, Stephen Boyd wrote:
>>>> Should it include the initrd too? At least x86 poisons that memory but I
>>>> don't know who would be using that incorrectly.
>>> It could do - I don't see any harm in not doing so.  The only issue
>>> is that people may want to disable this stuff if they're after
>>> squeezing every last ms out of the boot time.
>> I haven't done this. I hope a follow up patch will suffice.
>>
>>>> How about a free_init_area() function which calls free_area() after
>>>> poisoning the memory?
>>> I need to go back and look at the Integrator etc situation with regard
>>> to reorganizing the vmlinux layout - it may be that the omission of
>>> freeing .init memory can now be removed (it was there to stop the
>>> memory being used as the first K of memory wasn't DMA-able.)
>>>
>>> Assuming it has to stay though, we still should arrange for the initrd
>>> memory to be poisoned even if it isn't freed.
>> Is this is patch what you're saying? I would have liked to do a
>> free_init_area() wrapper, but until the Integrator situation can be
>> sorted it doesn't look worthwhile.
> Yes, thanks.  This looks fine for the time being.  Have you been able
> to test it?  If yes, then please put it in the patch system and I'll
> see about giving it a test too.

Yes it's been tested (which is why there is a PAGE_ALIGN on initrd).

6996/1

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.