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

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

[湛振波]

In this function, have these codes:

/*
		 * 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)) {
			unsigned long newsize = vmalloc_min - __va(bank->start);
			printk(KERN_NOTICE "Truncating RAM at %.8llx-%.8llx "
			       "to -%.8llx (vmalloc region overlap).\n",
			       (unsigned long long)bank->start,
			       (unsigned long long)bank->start + bank->size - 1,
			       (unsigned long long)bank->start + newsize - 1);
			bank->size = newsize;
		}

Dear all,


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.

Please indicate whether my understanding is correct.

Thanks.


Steve

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

[湛振波]

Dear all,

If system has defined CONFIG_SPARSEMEM, that va() and pa() have been
redefiend, like this,

 "
 #define __phys_to_virt(phys)						\
 	((phys) >= 0x80000000 ?	(phys) - 0x80000000 + PAGE_OFFSET2 :	\
 	 (phys) >= 0x20000000 ?	(phys) - 0x20000000 + PAGE_OFFSET1 :	\
 	 (phys) + PAGE_OFFSET)

 #define __virt_to_phys(virt)						\
 	 ((virt) >= PAGE_OFFSET2 ? (virt) - PAGE_OFFSET2 + 0x80000000 :	\
 	  (virt) >= PAGE_OFFSET1 ? (virt) - PAGE_OFFSET1 + 0x20000000 :	\
 	  (virt) - PAGE_OFFSET)

"
But this line "__va(bank->start + bank->size) < __va(bank->start)"
always assume that the next bank's vaddr is bigger than it.
Yes, we can defined these two MACROS carefully to avoid this problem.
How do you think it. Can you shed some light on what the correct
solution might be?

steve

>
>> Date: Tue, 24 Jul 2012 18:54:30 +0800
>> From: ??? <zhanzhenbo@gmail.com>
>> To: will.deacon at arm.com, catalin.marinas at arm.com
>> Cc: linux-arm-kernel at lists.infradead.org
>> Subject: Why the region area don't decrease 1 in function
>> 	sanity_check_meminfo?
>> Message-ID:
>> 	<CAKvkGKeqzAiqRAj+H+E-eoN9SOOMP-aMhUJYmUb0UK4ei=jouA@mail.gmail.com>
>> Content-Type: text/plain; charset=ISO-8859-1
>>
>> In this function, have these codes:
>>
>> /*
>> 		 * 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)) {
>> 			unsigned long newsize = vmalloc_min - __va(bank->start);
>> 			printk(KERN_NOTICE "Truncating RAM at %.8llx-%.8llx "
>> 			       "to -%.8llx (vmalloc region overlap).\n",
>> 			       (unsigned long long)bank->start,
>> 			       (unsigned long long)bank->start + bank->size - 1,
>> 			       (unsigned long long)bank->start + newsize - 1);
>> 			bank->size = newsize;
>> 		}
>>
>> Dear all,
>>
>>
>> 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.
>>
>> Please indicate whether my understanding is correct.
>>
>> Thanks.
>>
>>
>> Steve
>>
>>
>>
>> ------------------------------
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
>> End of linux-arm-kernel Digest, Vol 36, Issue 687
>> *************************************************
>>
>
>
> --
> Steve Zhan
>


-- 
Steve Zhan

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

[Jonathan Austin]

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 <jonathan.austin@arm.com>
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) <zhanzhenbo@gmail.com>
Signed-off-by: Jonathan Austin <jonathan.austin@arm.com>
---
 arch/arm/mm/mmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

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",

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

[Russell King - ARM Linux]

On Fri, Aug 10, 2012 at 04:14:05PM +0100, Jonathan Austin wrote:
> From: Jonathan Austin <jonathan.austin@arm.com>
> 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

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

[Jonathan Austin]

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