[PATCH] mm: fix pfn calculation mistake in __ioremap_check_ram

[PATCH] mm: fix pfn calculation mistake in __ioremap_check_ram

[Yaohui Wang]

According to the source code in function
arch/x86/mm/ioremap.c:__ioremap_caller, after __ioremap_check_mem, if the
mem range is IORES_MAP_SYSTEM_RAM, then __ioremap_caller should fail. But
because of the pfn calculation problem, __ioremap_caller can success
on IORES_MAP_SYSTEM_RAM region when the @size parameter is less than
PAGE_SIZE. This may cause misuse of the ioremap function and raise the
risk of performance issues. For example, ioremap(phys, PAGE_SIZE-1) may
cause the direct memory mapping of @phys to be uncached, and iounmap won't
revert this change. This patch fixes this issue.

In arch/x86/mm/ioremap.c:__ioremap_check_ram, start_pfn should wrap down
the res->start address, and end_pfn should wrap up the res->end address.
This makes the check more strict and should be more reasonable.

Signed-off-by: Ben Luo <luoben@linux.alibaba.com>
Signed-off-by: Yahui Wang <yaohuiwang@linux.alibaba.com>
---
 arch/x86/mm/ioremap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 9e5ccc56f..79adf0d2d 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -74,8 +74,8 @@ static unsigned int __ioremap_check_ram(struct resource *res)
 	if ((res->flags & IORESOURCE_SYSTEM_RAM) != IORESOURCE_SYSTEM_RAM)
 		return 0;
 
-	start_pfn = (res->start + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	stop_pfn = (res->end + 1) >> PAGE_SHIFT;
+	start_pfn = res->start >> PAGE_SHIFT;
+	stop_pfn = (res->end + PAGE_SIZE) >> PAGE_SHIFT;
 	if (stop_pfn > start_pfn) {
 		for (i = 0; i < (stop_pfn - start_pfn); ++i)
 			if (pfn_valid(start_pfn + i) &&
-- 
2.25.1

Re: [PATCH] mm: fix pfn calculation mistake in __ioremap_check_ram

[Dave Hansen]

On 6/7/21 2:19 AM, Yaohui Wang wrote:
> According to the source code in function
> arch/x86/mm/ioremap.c:__ioremap_caller, after __ioremap_check_mem, if the
> mem range is IORES_MAP_SYSTEM_RAM, then __ioremap_caller should fail. But
> because of the pfn calculation problem, __ioremap_caller can success
> on IORES_MAP_SYSTEM_RAM region when the @size parameter is less than
> PAGE_SIZE. This may cause misuse of the ioremap function and raise the
> risk of performance issues. For example, ioremap(phys, PAGE_SIZE-1) may
> cause the direct memory mapping of @phys to be uncached, and iounmap won't
> revert this change. This patch fixes this issue.
> 
> In arch/x86/mm/ioremap.c:__ioremap_check_ram, start_pfn should wrap down
> the res->start address, and end_pfn should wrap up the res->end address.
> This makes the check more strict and should be more reasonable.

Was this found by inspection, or was there a real-world bug which this
patch addresses?

Re: [PATCH] mm: fix pfn calculation mistake in __ioremap_check_ram

[Yaohui Wang]

On 2021/6/7 21:55, Dave Hansen wrote:
> On 6/7/21 2:19 AM, Yaohui Wang wrote:
>> According to the source code in function
>> arch/x86/mm/ioremap.c:__ioremap_caller, after __ioremap_check_mem, if the
>> mem range is IORES_MAP_SYSTEM_RAM, then __ioremap_caller should fail. But
>> because of the pfn calculation problem, __ioremap_caller can success
>> on IORES_MAP_SYSTEM_RAM region when the @size parameter is less than
>> PAGE_SIZE. This may cause misuse of the ioremap function and raise the
>> risk of performance issues. For example, ioremap(phys, PAGE_SIZE-1) may
>> cause the direct memory mapping of @phys to be uncached, and iounmap won't
>> revert this change. This patch fixes this issue.
>>
>> In arch/x86/mm/ioremap.c:__ioremap_check_ram, start_pfn should wrap down
>> the res->start address, and end_pfn should wrap up the res->end address.
>> This makes the check more strict and should be more reasonable.
> 
> Was this found by inspection, or was there a real-world bug which this
> patch addresses?
>

I did a performance test for linux kernel in many aspects. One of my 
scripts is to test the performance influence of ioremap. I found that 
applying ioremap on normal RAM may cause terrible performance issues.

To avoid memory cache behavior aliasing, ioremap will call 
memtype_kernel_map_sync to sync the cache attribute in the directing 
mapping, which causes:

1. If the phys addr is in a huge page in the directing mapping, then 
ioremap will split the huge page into 4K pages.
2. It will set the PCD bit in the directing mapping pte.

Both the above behaviors will downgrade the performance of the machine, 
especially when there is important code/data which is accessed 
frequently. What's worse, iounmap won't clear the PCD bit in the 
directing mapping pte, and I need to call ioremap_cache to clear the PCD 
bit. All these should be avoided.

Another thing also confuses me:

 From __ioremap_caller, we can see that __ioremap_caller don't allow us 
to remap normal RAM. In my understanding, direct mapping only maps 
normal RAM. So if the remap behavior is not allowed on normal RAM, it 
should be unnecessary to call memtype_kernel_map_sync. Is this right?