bug in lmb_enforce_memory_limit()

bug in lmb_enforce_memory_limit()

[David Miller]

I just mentioned this to Ben H. on IRC and promised I would report it
here. :-)

The first loop over lmb.memory in this function interprets the
memory_limit as a raw size limit, and that's fine so far.

But the second loop over lmb.reserved interprets this value
instead as an "address limit."

I haven't cobbled together a fix myself, but probably the way to do
this is, when we're about break out of the first loop over lmb.memory,
walk through the now-trimmed memory blobs and trim those from
lmb.reserved, one by one.

This bug got introduced by:

   commit 2babf5c2ec2f2d5de3e38d20f7df7fd815fd10c9
   Author: Michael Ellerman <michael@ellerman.id.au>
   Date:   Wed May 17 18:00:46 2006 +1000

       [PATCH] powerpc: Unify mem= handling

back when LMB was still a powerpc local item. :-)

This led me to another bug which probably a lot of platforms are
effected by.

If you do this command line memory limiting, and the kernel was placed
by the boot loader into physical ram (say at the end of the available
physical memory) that gets trimmed out by the command line option, we
hang or crash right as we boot into userspace because freeing up
initmem ends up freeing invalid page structs.

I think, on sparc64, instead of adding all kinds of complicated logic
to free_initmem() I'm simply going to only poison the pages and
not free them at all if cmdline_memory_size has been set.

Re: bug in lmb_enforce_memory_limit()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

On Thu, 2008-08-14 at 01:20 -0700, David Miller wrote:
> I just mentioned this to Ben H. on IRC and promised I would report it
> here. :-)
> 
> The first loop over lmb.memory in this function interprets the
> memory_limit as a raw size limit, and that's fine so far.
> 
> But the second loop over lmb.reserved interprets this value
> instead as an "address limit."
>
> I haven't cobbled together a fix myself, but probably the way to do
> this is, when we're about break out of the first loop over lmb.memory,
> walk through the now-trimmed memory blobs and trim those from
> lmb.reserved, one by one.

Perhaps after the first loop we should set memory_limit to equal
lmb_end_of_DRAM(), then the second loop should work as it is.

I think that actually makes memory_limit (the variable) more useful, and
avoids more code like we have in numa_enforce_memory_limit(), which
doesn't use memory_limit exactly because it isn't the value we're
actually interested in (because of holes).


> This bug got introduced by:
> 
>    commit 2babf5c2ec2f2d5de3e38d20f7df7fd815fd10c9
>    Author: Michael Ellerman <michael@ellerman.id.au>
>    Date:   Wed May 17 18:00:46 2006 +1000
> 
>        [PATCH] powerpc: Unify mem= handling
> 
> back when LMB was still a powerpc local item. :-)

Guilty as charged. I have some tests for that code, but clearly not
enough - and it gets very little exercise otherwise.

> This led me to another bug which probably a lot of platforms are
> effected by.
> 
> If you do this command line memory limiting, and the kernel was placed
> by the boot loader into physical ram (say at the end of the available
> physical memory) that gets trimmed out by the command line option, we
> hang or crash right as we boot into userspace because freeing up
> initmem ends up freeing invalid page structs.
> 
> I think, on sparc64, instead of adding all kinds of complicated logic
> to free_initmem() I'm simply going to only poison the pages and
> not free them at all if cmdline_memory_size has been set.

Would it be that much extra logic to check that the address is less than
the limit? Especially if we changed memory_limit to incorporate holes.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: bug in lmb_enforce_memory_limit()

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Thu, 14 Aug 2008 21:26:53 +1000

> Perhaps after the first loop we should set memory_limit to equal
> lmb_end_of_DRAM(), then the second loop should work as it is.

Sounds great.  Mind if I push the following to Linus?

lmb: Fix reserved region handling in lmb_enforce_memory_limit().

The idea of the implementation of this fix is from Michael Ellerman.

This function has two loops, but they each interpret the memory_limit
value differently.  The first loop interprets it as a "size limit"
whereas the second loop interprets it as an "address limit".

Before the second loop runs, reset memory_limit to lmb_end_of_DRAM()
so that it all works out.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/lib/lmb.c b/lib/lmb.c
index 5d7b928..97e5470 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -462,6 +462,8 @@ void __init lmb_enforce_memory_limit(u64 memory_limit)
 	if (lmb.memory.region[0].size < lmb.rmo_size)
 		lmb.rmo_size = lmb.memory.region[0].size;
 
+	memory_limit = lmb_end_of_DRAM();
+
 	/* And truncate any reserves above the limit also. */
 	for (i = 0; i < lmb.reserved.cnt; i++) {
 		p = &lmb.reserved.region[i];

Re: bug in lmb_enforce_memory_limit()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Fri, 2008-08-15 at 15:25 -0700, David Miller wrote:
> From: Michael Ellerman <michael@ellerman.id.au>
> Date: Thu, 14 Aug 2008 21:26:53 +1000
> 
> > Perhaps after the first loop we should set memory_limit to equal
> > lmb_end_of_DRAM(), then the second loop should work as it is.
> 
> Sounds great.  Mind if I push the following to Linus?

Looks good to me.

I'll test it on Monday. I don't know if I have a system with memory
holes to test on, but I take it you do?

I notice some of our 32-bit code is using lmb_enforce_memory_limit() to
enforce an address limit, which is technically broken, but is probably
fine because it doesn't need to worry about holes.


> lmb: Fix reserved region handling in lmb_enforce_memory_limit().
> 
> The idea of the implementation of this fix is from Michael Ellerman.
> 
> This function has two loops, but they each interpret the memory_limit
> value differently.  The first loop interprets it as a "size limit"
> whereas the second loop interprets it as an "address limit".
> 
> Before the second loop runs, reset memory_limit to lmb_end_of_DRAM()
> so that it all works out.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

Acked-by: Michael Ellerman <michael@ellerman.id.au>

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: bug in lmb_enforce_memory_limit()

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Sat, 16 Aug 2008 10:46:22 +1000

> On Fri, 2008-08-15 at 15:25 -0700, David Miller wrote:
> > Sounds great.  Mind if I push the following to Linus?
> 
> Looks good to me.
> 
> I'll test it on Monday. I don't know if I have a system with memory
> holes to test on, but I take it you do?
> 
> I notice some of our 32-bit code is using lmb_enforce_memory_limit() to
> enforce an address limit, which is technically broken, but is probably
> fine because it doesn't need to worry about holes.

Sounds like fun :-)

> Acked-by: Michael Ellerman <michael@ellerman.id.au>

Thanks for reviewing Michael.

Re: bug in lmb_enforce_memory_limit()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Fri, 2008-08-15 at 19:57 -0700, David Miller wrote:
> From: Michael Ellerman <michael@ellerman.id.au>
> Date: Sat, 16 Aug 2008 10:46:22 +1000
> 
> > On Fri, 2008-08-15 at 15:25 -0700, David Miller wrote:
> > > Sounds great.  Mind if I push the following to Linus?
> > 
> > Looks good to me.
> > 
> > I'll test it on Monday. I don't know if I have a system with memory
> > holes to test on, but I take it you do?

Works for me.

Although I can't find a system with holes (the newer hypervisors make
things look contiguous I think), so I can't really test the interesting
case.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: bug in lmb_enforce_memory_limit()

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Mon, 18 Aug 2008 12:00:32 +1000

> Although I can't find a system with holes (the newer hypervisors make
> things look contiguous I think), so I can't really test the interesting
> case.

I have several so I'll make sure it works in such cases :)