[PATCH v3 0/2] x86/ioremap: fix boundary calculation and boundary judgment issues for ioremap()

[PATCH v3 0/2] x86/ioremap: fix boundary calculation and boundary judgment issues for ioremap()

[Yaohui Wang]

ioremap_xxx() functions should fail if the memory address range contains
normal RAM. But due to some boundary calculation and boundary judgment
issues, the RAM check may be omitted for the very start or the very end
page in the memory range. As a consequence, ioremap_xxx() can be applied
to normal RAM pages by mistake. This raises the risk of misusing
ioremap_xxx() functions on normal RAM ranges, and may incur terrible
performance issues.

For example, suppose [phys_addr ~ phys_addr + PAGE_SIZE - 1] is a normal
RAM page. Calling ioremap(phys_addr, PAGE_SIZE-1) will succeed (but it
should not). This will set the cache flag of the phys_addr's directing
mapping pte to be PCD. What's worse, iounmap() won't revert this cache
flag in the directing mapping. So the pte in the directing mapping keeps
polluted until workarounds are applied (by invoking ioremap_cache() on
phys_addr) to fix the cache bit. If there is important data/code in the
polluted page, which is accessed frequently, then the performance of the
machine will drop terribly.

These two patches aim to address this issue. 

Yahui Wang (2):
  x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()
  kernel/resource: fix boundary judgment issues in find_next_iomem_res()
    and __walk_iomem_res_desc()

 arch/x86/mm/ioremap.c | 16 ++++++++--------
 kernel/resource.c     |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)


base-commit: 13311e74253fe64329390df80bed3f07314ddd61
-- 
2.25.1

[PATCH v3 1/2] x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()

[Yaohui Wang]

In __ioremap_check_ram(), the pfn wrapping calculation supposes res->start
to be page-aligned and res->end to be PAGE_SIZE - 1 aligned. But
res->start and res->end may not follow such alignment, which may make the
RAM checking be omitted for the very start page or the very end page of
the memory range. This can cause ioremap_xxx() to succeed on normal RAM by
mistake.

For example, suppose memory range [phys_addr ~ phys_addr + PAGE_SIZE - 1]
is a normal RAM page. ioremap(phys_addr, PAGE_SIZE - 1) will succeed
(but it should not) because the pfn wrapping prevents this page to be
checked whether it touches non-ioremappable resources.

The new pfn wrapping calculation makes sure the resulting pfn range covers
[res->start, res->end] completely.

Fixes: 0e4c12b45aa8 (x86/mm, resource: Use PAGE_KERNEL protection for ioremap of memory pages)
Signed-off-by: Yahui Wang <yaohuiwang@linux.alibaba.com>
Signed-off-by: Ben Luo <luoben@linux.alibaba.com>
---
 arch/x86/mm/ioremap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 60ade7dd71bd..609a8bd6f680 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -68,19 +68,19 @@ int ioremap_change_attr(unsigned long vaddr, unsigned long size,
 /* Does the range (or a subset of) contain normal RAM? */
 static unsigned int __ioremap_check_ram(struct resource *res)
 {
-	unsigned long start_pfn, stop_pfn;
+	unsigned long start_pfn, stop_pfn, npages;
 	unsigned long i;
 
 	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;
-	if (stop_pfn > start_pfn) {
-		for (i = 0; i < (stop_pfn - start_pfn); ++i)
-			if (pfn_valid(start_pfn + i) &&
-			    !PageReserved(pfn_to_page(start_pfn + i)))
-				return IORES_MAP_SYSTEM_RAM;
+	start_pfn = PFN_DOWN(res->start);
+	stop_pfn = PFN_DOWN(res->end);
+	npages = stop_pfn - start_pfn + 1;
+	for (i = 0; i < npages; ++i) {
+		if (pfn_valid(start_pfn + i) &&
+		    !PageReserved(pfn_to_page(start_pfn + i)))
+			return IORES_MAP_SYSTEM_RAM;
 	}
 
 	return 0;
-- 
2.25.1

Re: [PATCH v3 1/2] x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()

[Dave Hansen]

On 6/21/21 5:34 AM, Yaohui Wang wrote:
> For example, suppose memory range [phys_addr ~ phys_addr + PAGE_SIZE - 1]
> is a normal RAM page. ioremap(phys_addr, PAGE_SIZE - 1) will succeed
> (but it should not) because the pfn wrapping prevents this page to be
> checked whether it touches non-ioremappable resources.

I would have expected such a scenario to get caught by this check:

	if ((res->flags & IORESOURCE_SYSTEM_RAM) != IORESOURCE_SYSTEM_RAM)
		return false;

Was this issue found by inspection, or is it causing an actual problem
in practice?

Also, it would be really nice to include the original authors when you
send Fixes: for patches.  BTW, scripts/get_maintainer.pl would have done
this for you.  I've added Tom and Brijesh.  Please cc them in the future.

Re: [PATCH v3 1/2] x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()

[Yaohui Wang]

On 2021/7/1 22:41, Dave Hansen wrote:
> Was this issue found by inspection, or is it causing an actual problem
> in practice?

This issus truly caused terrible perforamnce downgrade in the practice.
When developing an out of tree module in our testing environment,
invoking ioremap() on normal RAM causes apparent CLI lag. The Unixbench
score also decreases a lot (5x slowdown in the worst case).

Debugging such performance issue is extremely difficult, especially when
the code of the faulty module itself is already very complex. I tested
the system in many aspects before finally located this problem.

> Also, it would be really nice to include the original authors when you
> send Fixes: for patches.  BTW, scripts/get_maintainer.pl would have done
> this for you.  I've added Tom and Brijesh.  Please cc them in the future.

I'll pay attention to this in the future. Thank you for your time and
patience!

Re: [PATCH v3 1/2] x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()

[Dave Hansen]

On 7/2/21 3:05 AM, Yaohui Wang wrote:
> On 2021/7/1 22:41, Dave Hansen wrote:
>> Was this issue found by inspection, or is it causing an actual problem
>> in practice?
> 
> This issus truly caused terrible perforamnce downgrade in the practice.
> When developing an out of tree module in our testing environment,
> invoking ioremap() on normal RAM causes apparent CLI lag. The Unixbench
> score also decreases a lot (5x slowdown in the worst case).
> 
> Debugging such performance issue is extremely difficult, especially when
> the code of the faulty module itself is already very complex. I tested
> the system in many aspects before finally located this problem.

Do you know why this check:

> 	if ((res->flags & IORESOURCE_SYSTEM_RAM) != IORESOURCE_SYSTEM_RAM)
> 		return false;

did not catch your out-of-tree driver's errant ioremap()?

Re: [PATCH v3 1/2] x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()

[Yaohui Wang]

On 2021/7/2 22:49, Dave Hansen wrote:
> Do you know why this check:
> 
>> 	if ((res->flags & IORESOURCE_SYSTEM_RAM) != IORESOURCE_SYSTEM_RAM)
>> 		return false;
> 
> did not catch your out-of-tree driver's errant ioremap()?
>
If ioremap() is invoked on an area that contains normal memory, then:

	(res->flags & IORESOURCE_SYSTEM_RAM) == IORESOURCE_SYSTEM_RAM)

is true, so the original check is false. The code following the check
will continue to scan whether this area contains any page that is not
PageReserved (i.e. that is truly normal RAM).

Your idea should be:

	if ((res->flags & IORESOURCE_SYSTEM_RAM) == IORESOURCE_SYSTEM_RAM)
	return IORES_MAP_SYSTEM_RAM;

But this check is too strict as IORESOURCE_SYSTEM_RAM area may contain
PageReserved pages, and PageReserved pages should be ioremap-able. So
the checking logic of the original __ioremap_check_ram() function is
reasonable.

[PATCH v3 2/2] kernel/resource: fix boundary judgment issues in find_next_iomem_res() and __walk_iomem_res_desc()

[Yaohui Wang]

Memory resources are described with the start address and the inclusive
end address, which means for a resource with 1 byte length the start
address is the same as the end address.

find_next_iomem_res() and __walk_iomem_res_desc() ignore resources with
1 byte length, which prevents that ioremap_xxx(phys_addr, 1) is checked
whether it touches non-ioremappable resources.

Fixes: 010a93bf97c7 (resource: Fix find_next_iomem_res() iteration issue)
Fixes: b69c2e20f6e4 (resource: Clean it up a bit)
Signed-off-by: Yahui Wang <yaohuiwang@linux.alibaba.com>
Signed-off-by: Ben Luo <luoben@linux.alibaba.com>
---
 kernel/resource.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index ca9f5198a01f..31e371babfad 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -344,7 +344,7 @@ static int find_next_iomem_res(resource_size_t start, resource_size_t end,
 	if (!res)
 		return -EINVAL;
 
-	if (start >= end)
+	if (start > end)
 		return -EINVAL;
 
 	read_lock(&resource_lock);
@@ -392,7 +392,7 @@ static int __walk_iomem_res_desc(resource_size_t start, resource_size_t end,
 	struct resource res;
 	int ret = -EINVAL;
 
-	while (start < end &&
+	while (start <= end &&
 	       !find_next_iomem_res(start, end, flags, desc, &res)) {
 		ret = (*func)(&res, arg);
 		if (ret)
-- 
2.25.1

Re: [PATCH v3 2/2] kernel/resource: fix boundary judgment issues in find_next_iomem_res() and __walk_iomem_res_desc()

[Dave Hansen]

On 6/21/21 5:34 AM, Yaohui Wang wrote:
> Memory resources are described with the start address and the inclusive
> end address, which means for a resource with 1 byte length the start
> address is the same as the end address.

Is it just me or does this little "feature" of the resource code
continue to bite us over and over?

It might be some time for some unit tests for kernel/resource.c.

Re: [PATCH v3 0/2] x86/ioremap: fix boundary calculation and boundary judgment issues for ioremap()

[Yaohui Wang]

Hi, maintainers,

It's been 10 days since I sent the patches at Jun 21st. Would you please
help to review them? Wish to hear your feedbacks, Thanks!


Yaohui

On 2021/6/21 20:34, Yaohui Wang wrote:
> ioremap_xxx() functions should fail if the memory address range contains
> normal RAM. But due to some boundary calculation and boundary judgment
> issues, the RAM check may be omitted for the very start or the very end
> page in the memory range. As a consequence, ioremap_xxx() can be applied
> to normal RAM pages by mistake. This raises the risk of misusing
> ioremap_xxx() functions on normal RAM ranges, and may incur terrible
> performance issues.
> 
> For example, suppose [phys_addr ~ phys_addr + PAGE_SIZE - 1] is a normal
> RAM page. Calling ioremap(phys_addr, PAGE_SIZE-1) will succeed (but it
> should not). This will set the cache flag of the phys_addr's directing
> mapping pte to be PCD. What's worse, iounmap() won't revert this cache
> flag in the directing mapping. So the pte in the directing mapping keeps
> polluted until workarounds are applied (by invoking ioremap_cache() on
> phys_addr) to fix the cache bit. If there is important data/code in the
> polluted page, which is accessed frequently, then the performance of the
> machine will drop terribly.
> 
> These two patches aim to address this issue.
> 
> Yahui Wang (2):
>    x86/ioremap: fix the pfn calculation mistake in __ioremap_check_ram()
>    kernel/resource: fix boundary judgment issues in find_next_iomem_res()
>      and __walk_iomem_res_desc()
> 
>   arch/x86/mm/ioremap.c | 16 ++++++++--------
>   kernel/resource.c     |  4 ++--
>   2 files changed, 10 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 13311e74253fe64329390df80bed3f07314ddd61
>