Why the region area don't decrease 1 in function sanity_check_meminfo?

Commit Message

Jonathan Austin Aug. 10, 2012, 3:14 p.m. UTC

On 24/07/12 11:54, ??? wrote:

> I don't known why the memory region have not decrease 1, i think it
> should be decreased(like this , (__va(bank->start + bank->size
> -1))because in sparse memory system,  this address bank->start +
> bank->size is belong to another bank, so this method is not correctly.

I think you're right - we should have the '- 1' in there, although I 
don't think there currently exist any situations where this will cause
us grief. Here's a patch for correctness anyway...

> Please indicate whether my understanding is correct.
 

As far as I can see in all existing situations, the worst effect of this
will be truncating by a single byte a bank that doesn't actually need
truncating.

As you said in your following email, we would get trouble with this if
using SPARSEMEM in a situation where the physical to virtual address
conversion is not monotonic increasing.

Jonny

---8<---
From: Jonathan Austin <[e-mail]>
Date: Thu, 9 Aug 2012 15:59:16 +0100
Subject: [PATCH] arm: mm: Fix vmalloc overlap check for !HIGHMEM

With !HIGHMEM, sanity_check_meminfo checks for banks that completely or
partially overlap the vmalloc region. The check for partial overlap checks
__va(bank->start + bank->size) > vmalloc_min, but the last address of the
bank is (bank->start + bank->size -1).

This doesn't cause serious issues for existing platforms, except to
truncate by a single byte a bank that doesn't actually need truncating if
it happens to finish at vmalloc_min -1.

However, theoretically, if using using SPARSEMEM in a situation where the
physical to virtual address conversion is not monotonic increasing, the
incorrect test could result in a bank not being truncated when it should be.

Reported-by: ??? (Steve) <[e-mail]>
Signed-off-by: Jonathan Austin <[e-mail]>
---
 arch/arm/mm/mmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Russell King - ARM Linux Aug. 10, 2012, 3:37 p.m. UTC | #1

On Fri, Aug 10, 2012 at 04:14:05PM +0100, Jonathan Austin wrote:
> From: Jonathan Austin <[e-mail]>
> Date: Thu, 9 Aug 2012 15:59:16 +0100
> Subject: [PATCH] arm: mm: Fix vmalloc overlap check for !HIGHMEM
> 
> With !HIGHMEM, sanity_check_meminfo checks for banks that completely or
> partially overlap the vmalloc region. The check for partial overlap checks
> __va(bank->start + bank->size) > vmalloc_min, but the last address of the
> bank is (bank->start + bank->size -1).

Erm.

Let's say you have a bank at 0x80000000, which maps to 0xc0000000 virtual.
This is 512MB in size (so it's last byte address is 0x9fffffff).  That
places it at at 0xdfffffff.  Now, let's say vmalloc_min is 0xe0000000.

"bank->start + bank->size" would be 0xa0000000, right ?

So, "__va(bank->start + bank->size)" would be 0xe0000000.

And "0xe0000000 > vmalloc_min" would be false.

> However, theoretically, if using using SPARSEMEM in a situation where the
> physical to virtual address conversion is not monotonic increasing, the
> incorrect test could result in a bank not being truncated when it should be.

Right, so what you're actually talking about is a non-linear translation
by __va() and friends.  In that case, what you actually need is:

	(__va(bank->start + bank->size - 1) + 1) > vmalloc_min
or
	__va(bank->start + bank->size - 1) >= vmalloc_min

and not

	__va(bank->start + bank->size - 1) > vmalloc_min

Jonathan Austin Aug. 10, 2012, 5:43 p.m. UTC | #2

Hi Russell,

On 10/08/12 16:37, Russell King - ARM Linux wrote:

>> With !HIGHMEM, sanity_check_meminfo checks for banks that completely or
>> partially overlap the vmalloc region. The check for partial overlap checks
>> __va(bank->start + bank->size) > vmalloc_min, but the last address of the
>> bank is (bank->start + bank->size -1).

> 
> Erm.
> 
> Let's say you have a bank at 0x80000000, which maps to 0xc0000000 virtual.
> This is 512MB in size (so it's last byte address is 0x9fffffff).  That
> places it at at 0xdfffffff.  Now, let's say vmalloc_min is 0xe0000000.
> 
> "bank->start + bank->size" would be 0xa0000000, right ?
> 
> So, "__va(bank->start + bank->size)" would be 0xe0000000.


Yep, except in our (hypothetical?) case below...
>

> And "0xe0000000 > vmalloc_min" would be false.
> 


Yup... My commit message is wrong. Sorry. The linear case is fine...

>> However, theoretically, if using using SPARSEMEM in a situation where the
>> physical to virtual address conversion is not monotonic increasing, the
>> incorrect test could result in a bank not being truncated when it should be.
> 
> Right, so what you're actually talking about is a non-linear translation
> by __va() and friends.  In that case, what you actually need is:
> 
> 	(__va(bank->start + bank->size - 1) + 1) > vmalloc_min
> or
> 	__va(bank->start + bank->size - 1) >= vmalloc_min
> 
> and not
> 
> 	__va(bank->start + bank->size - 1) > vmalloc_min
> 


Agreed. I prefer the second form, and there seems to be a precedent for
">=" further up in sanity_check_meminfo, too.

If you think it is worth fixing for this case, I'll make the fixes,
ensure the commit message is !wrong and submit this to the patch-system.

Jonny

Patch

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 4c2d045..29f7084 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -961,8 +961,8 @@  void __init sanity_check_meminfo(void)
                 * Check whether this memory bank would partially overlap
                 * the vmalloc area.
                 */
-               if (__va(bank->start + bank->size) > vmalloc_min ||
-                   __va(bank->start + bank->size) < __va(bank->start)) {
+               if (__va(bank->start + bank->size - 1) > vmalloc_min ||
+                   __va(bank->start + bank->size - 1) < __va(bank->start)) {
                        unsigned long newsize = vmalloc_min - __va(bank->start);
                        printk(KERN_NOTICE "Truncating RAM at %.8llx-%.8llx "
                               "to -%.8llx (vmalloc region overlap).\n",