Skip to contentSkip to conversation

ARM: highmem: avoid clobbering non-page aligned memory reservations

PATCH In review Ard Biesheuvel 3 participants 8 messages Oct 29, 2020 – Oct 30, 2020

Subsystems (3)
Stable branch Arm (32-bit) Tensilica Xtensa port (xtensa)
lore
Share & tools
lore

Ard Biesheuvel

author
#0 lore raw

PATCH 1/1 ARM: highmem: avoid clobbering non-page aligned memory reservations

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +1 -1
index a391804c7ce3..d41781cb5496 100644
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
 	for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
 				&range_start, &range_end, NULL) {
 		unsigned long start = PHYS_PFN(range_start);
-		unsigned long end = PHYS_PFN(range_end);
+		unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));
 
 		/* Ignore complete lowmem entries */
 		if (end <= max_low)
Signature · 1 line
2.17.1
author
#1 lore raw
Quoted 19 lines · Ard Biesheuvel

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

- unsigned long start = PHYS_PFN(range_start);

+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Quoted 5 lines
arch/arm/mm/init.c +0 -1
                /* Ignore complete lowmem entries */
                if (end <= max_low)
--

2.17.1

#2 lore raw
Quoted 34 lines · Ard Biesheuvel
Quoted 19 lines · Ard Biesheuvel

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

(quoted diff) +1 -1
-               unsigned long start = PHYS_PFN(range_start);
+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);
Quoted 2 lines

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Is there a particular commit that you identified which could be used as
Fixes: tag to ease the back porting of such a change?

Signature · 1 line
Florian
author
#3 lore raw

(+ Mike)

Quoted 38 lines · Florian Fainelli
Quoted 34 lines · Ard Biesheuvel
Quoted 19 lines · Ard Biesheuvel

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

(quoted diff) +1 -1
-               unsigned long start = PHYS_PFN(range_start);
+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);
Quoted 5 lines
Quoted 2 lines

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Is there a particular commit that you identified which could be used as
Fixes: tag to ease the back porting of such a change?

Ah hold on. This appears to be a very recent regression, in cddb5ddf2b76debdb8cad1728ad0a9321383d933, added in v5.10-rc1.

The old code was

unsigned long start = memblock_region_memory_base_pfn(mem);

which uses PFN_UP() to round up, whereas the new code rounds down.

Looks like this is broken on a lot of platforms.

Mike?

#4 lore raw

Hi Ard,

Quoted 52 lines · Ard Biesheuvel

(+ Mike)

Quoted 38 lines · Florian Fainelli
Quoted 34 lines · Ard Biesheuvel
Quoted 19 lines · Ard Biesheuvel

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

(quoted diff) +1 -1
-               unsigned long start = PHYS_PFN(range_start);
+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);
Quoted 5 lines
Quoted 2 lines

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Is there a particular commit that you identified which could be used as
Fixes: tag to ease the back porting of such a change?

Ah hold on. This appears to be a very recent regression, in cddb5ddf2b76debdb8cad1728ad0a9321383d933, added in v5.10-rc1.

The old code was

Quoted 7 lines

unsigned long start = memblock_region_memory_base_pfn(mem);

which uses PFN_UP() to round up, whereas the new code rounds down.

Looks like this is broken on a lot of platforms.

Mike?

I've reviewed again the whole series and it seems that only highmem initialization on arm and xtensa (that copied this code from arm) have this problem. I might have missed something again, though.

So, to restore the original behaviour I think the fix should be

	for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
				&range_start, &range_end, NULL) {
		unsigned long start = PHYS_UP(range_start);
		unsigned long end = PHYS_DOWN(range_end);
Signature · 7 lines
Sincerely yours,
Mike.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#5 lore raw
Quoted 68 lines · Mike Rapoport

Hi Ard,

Quoted 52 lines · Ard Biesheuvel

(+ Mike)

Quoted 38 lines · Florian Fainelli
Quoted 34 lines · Ard Biesheuvel
Quoted 19 lines · Ard Biesheuvel

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

(quoted diff) +1 -1
-               unsigned long start = PHYS_PFN(range_start);
+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);
Quoted 5 lines
Quoted 2 lines

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Is there a particular commit that you identified which could be used as
Fixes: tag to ease the back porting of such a change?

Ah hold on. This appears to be a very recent regression, in cddb5ddf2b76debdb8cad1728ad0a9321383d933, added in v5.10-rc1.

The old code was

Quoted 7 lines

unsigned long start = memblock_region_memory_base_pfn(mem);

which uses PFN_UP() to round up, whereas the new code rounds down.

Looks like this is broken on a lot of platforms.

Mike?

I've reviewed again the whole series and it seems that only highmem initialization on arm and xtensa (that copied this code from arm) have this problem. I might have missed something again, though.

Quoted 8 lines

So, to restore the original behaviour I think the fix should be

        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_UP(range_start);
                unsigned long end = PHYS_DOWN(range_end);

PHYS_UP and PHYS_DOWN don't exist.

Could you please send a patch that fixes this everywhere where it's broken?

#6 lore raw
Quoted 71 lines · Ard Biesheuvel
Quoted 61 lines · Mike Rapoport

Hi Ard,

Quoted 46 lines
Quoted 35 lines · Ard Biesheuvel
Quoted 34 lines · Ard Biesheuvel
Quoted 19 lines

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

(quoted diff) +1 -1
-               unsigned long start = PHYS_PFN(range_start);
+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);
Quoted 5 lines
Quoted 2 lines

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Is there a particular commit that you identified which could be used as
Fixes: tag to ease the back porting of such a change?

Ah hold on. This appears to be a very recent regression, in cddb5ddf2b76debdb8cad1728ad0a9321383d933, added in v5.10-rc1.

The old code was

Quoted 7 lines

unsigned long start = memblock_region_memory_base_pfn(mem);

which uses PFN_UP() to round up, whereas the new code rounds down.

Looks like this is broken on a lot of platforms.

Mike?

I've reviewed again the whole series and it seems that only highmem initialization on arm and xtensa (that copied this code from arm) have this problem. I might have missed something again, though.

Quoted 8 lines

So, to restore the original behaviour I think the fix should be

        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_UP(range_start);
                unsigned long end = PHYS_DOWN(range_end);
Quoted 3 lines

PHYS_UP and PHYS_DOWN don't exist.

Could you please send a patch that fixes this everywhere where it's broken?

Argh, this should have been PFN_{UP,DOWN}.
With the patch below qemu-system-arm boots for me. Does it fix your setup as well?

I kept your authorship as you did the heavy lifting here :)

With acks from ARM and xtensa maintainers I can take it via memblock tree.

From 5399699b9f8de405819c59c3feddecaac0ed1399 Mon Sep 17 00:00:00 2001
From: Ard Biesheuvel <[email protected]>
Date: Fri, 30 Oct 2020 22:53:02 +0200
Subject: [PATCH] ARM, xtensa: highmem: avoid clobbering non-page aligned
memory reservations

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system.

Until commit cddb5ddf2b76 ("arm, xtensa: simplify initialization of high memory pages") it rounded beginning of each region upwards and end of each region downwards.

However, after that commit free_highmem() rounds the beginning and end of each region downwards, we and may end up freeing a page that is memblock_reserve()d, resulting in memory corruption.

Restore the original rounding of the region boundaries to avoid freeing reserved pages.

Fixes cddb5ddf2b76 ("arm, xtensa: simplify initialization of high memory pages")
Signed-off-by Ard Biesheuvel <[email protected]>
Co-developed-by Mike Rapoport <[email protected]>
Signed-off-by Mike Rapoport <[email protected]>
---
 arch/arm/mm/init.c    | 4 ++--
 arch/xtensa/mm/init.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm/mm/init.c +2 -2
index d57112a276f5..c23dbf8bebee 100644
@@ -354,8 +354,8 @@ static void __init free_highpages(void)
 	/* set highmem page free */
 	for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
 				&range_start, &range_end, NULL) {
-		unsigned long start = PHYS_PFN(range_start);
-		unsigned long end = PHYS_PFN(range_end);
+		unsigned long start = PFN_UP(range_start);
+		unsigned long end = PFN_DOWN(range_end);
 
 		/* Ignore complete lowmem entries */
 		if (end <= max_low)
arch/xtensa/mm/init.c +2 -2
index c6fc83efee0c..8731b7ad9308 100644
@@ -89,8 +89,8 @@ static void __init free_highpages(void)
 	/* set highmem page free */
 	for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
 				&range_start, &range_end, NULL) {
-		unsigned long start = PHYS_PFN(range_start);
-		unsigned long end = PHYS_PFN(range_end);
+		unsigned long start = PFN_UP(range_start);
+		unsigned long end = PFN_DOWN(range_end);
 
 		/* Ignore complete lowmem entries */
 		if (end <= max_low)
Signature · 6 lines
2.28.0


-- 
Sincerely yours,
Mike.
author
#7 lore raw
Quoted 73 lines · Mike Rapoport
Quoted 71 lines · Ard Biesheuvel
Quoted 69 lines · Mike Rapoport

Hi Ard,

Quoted 46 lines
Quoted 35 lines · Ard Biesheuvel
Quoted 34 lines · Ard Biesheuvel
Quoted 19 lines

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system. However, as it rounds the end of each region downwards, we may end up freeing a page that is memblock_reserve()d, resulting in memory corruption. So align the end of the range to the next page instead.

Cc <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/init.c +0 -0
index a391804c7ce3..d41781cb5496 100644

arch/arm/mm/init.c static void __init free_highpages(void) · line 358

Quoted 6 lines
arch/arm/mm/init.c +1 -1
@@ -354,7 +354,7 @@ static void __init free_highpages(void)
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long end = PHYS_PFN(PAGE_ALIGN(range_end));

Apologies, this should be

(quoted diff) +1 -1
-               unsigned long start = PHYS_PFN(range_start);
+               unsigned long start = PHYS_PFN(PAGE_ALIGN(range_start));
                unsigned long end = PHYS_PFN(range_end);
Quoted 5 lines
Quoted 2 lines

Strangely enough, the wrong version above also fixed the issue I was seeing, but it is start that needs rounding up, not end.

Is there a particular commit that you identified which could be used as
Fixes: tag to ease the back porting of such a change?

Ah hold on. This appears to be a very recent regression, in cddb5ddf2b76debdb8cad1728ad0a9321383d933, added in v5.10-rc1.

The old code was

Quoted 7 lines

unsigned long start = memblock_region_memory_base_pfn(mem);

which uses PFN_UP() to round up, whereas the new code rounds down.

Looks like this is broken on a lot of platforms.

Mike?

I've reviewed again the whole series and it seems that only highmem initialization on arm and xtensa (that copied this code from arm) have this problem. I might have missed something again, though.

So, to restore the original behaviour I think the fix should be

        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
                unsigned long start = PHYS_UP(range_start);
                unsigned long end = PHYS_DOWN(range_end);
Quoted 8 lines
Quoted 3 lines

PHYS_UP and PHYS_DOWN don't exist.

Could you please send a patch that fixes this everywhere where it's broken?

Argh, this should have been PFN_{UP,DOWN}.
With the patch below qemu-system-arm boots for me. Does it fix your setup as well?

Yeah that looks fine.

Quoted 5 lines

I kept your authorship as you did the heavy lifting here :)

With acks from ARM and xtensa maintainers I can take it via memblock tree.

That would be up to Russell (on cc) and whoever the xtensa maintainers are, who are not on CC.

Would you mind chasing them?

Quoted 10 lines

From 5399699b9f8de405819c59c3feddecaac0ed1399 Mon Sep 17 00:00:00 2001
From: Ard Biesheuvel <[email protected]>
Date: Fri, 30 Oct 2020 22:53:02 +0200
Subject: [PATCH] ARM, xtensa: highmem: avoid clobbering non-page aligned
memory reservations

free_highpages() iterates over the free memblock regions in high memory, and marks each page as available for the memory management system.

Quoted 6 lines

Until commit cddb5ddf2b76 ("arm, xtensa: simplify initialization of high memory pages") it rounded beginning of each region upwards and end of each region downwards.

However, after that commit free_highmem() rounds the beginning and end of each region downwards, we and may end up freeing a page that is

we may end up

Quoted 51 lines

memblock_reserve()d, resulting in memory corruption.

Restore the original rounding of the region boundaries to avoid freeing reserved pages.

Fixes cddb5ddf2b76 ("arm, xtensa: simplify initialization of high memory pages")
Signed-off-by Ard Biesheuvel <[email protected]>
Co-developed-by Mike Rapoport <[email protected]>
Signed-off-by Mike Rapoport <[email protected]>
---
 arch/arm/mm/init.c    | 4 ++--
 arch/xtensa/mm/init.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm/mm/init.c +2 -2
index d57112a276f5..c23dbf8bebee 100644
@@ -354,8 +354,8 @@ static void __init free_highpages(void)
        /* set highmem page free */
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
-               unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long start = PFN_UP(range_start);
+               unsigned long end = PFN_DOWN(range_end);
 
                /* Ignore complete lowmem entries */
                if (end <= max_low)
arch/xtensa/mm/init.c +2 -3
index c6fc83efee0c..8731b7ad9308 100644
@@ -89,8 +89,8 @@ static void __init free_highpages(void)
        /* set highmem page free */
        for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
                                &range_start, &range_end, NULL) {
-               unsigned long start = PHYS_PFN(range_start);
-               unsigned long end = PHYS_PFN(range_end);
+               unsigned long start = PFN_UP(range_start);
+               unsigned long end = PFN_DOWN(range_end);
 
                /* Ignore complete lowmem entries */
                if (end <= max_low)
--

2.28.0

--
Sincerely yours,
Mike.