mm: bootmem: Check pfn_valid() before accessing struct page

PATCH In review Matt Fleming 2 participants 3 messages May 27, 2014 – Jun 3, 2014 Memory management

Share & tools

Matt Fleming

author

2014-05-27 14:10 UTC #0 lore raw

PATCH 1/1 mm: bootmem: Check pfn_valid() before accessing struct page

We need to check that a pfn is valid before handing it to pfn_to_page() since on low memory systems with CONFIG_HIGHMEM=n it's possible that a pfn may not have a corresponding struct page.

This is in fact the case for one of Alan's machines where some of the EFI boot services pages live in highmem, and running a kernel without CONFIG_HIGHMEM enabled results in the following oops,

 BUG: unable to handle kernel paging request at f7f1f080
 IP: [<c17fba96>] __free_pages_bootmem+0x5a/0xb8
 *pdpt = 0000000001887001 *pde = 0000000001984067 *pte = 000000000 0000000
 Oops: 0002 [#1] SMP

[...]

 Call Trace:
  [<c17feacc>] free_bootmem_late+0x2d/0x3d
  [<c17f1013>] efi_free_boot_services+0x48/0x5b
  [<c17ddc12>] start_kernel+0x3ad/0x3cf
  [<c17dd654>] ? set_init_arg+0x49/0x49
  [<c17dd380>] i386_start_kernel+0x12e/0x131

Reported-by Alan Cox <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by Matt Fleming <[email protected]>

---
 mm/bootmem.c   | 3 +++
 mm/nobootmem.c | 3 +++
 2 files changed, 6 insertions(+)

mm/bootmem.c +3 -0

index 90bd3507b413..406e9cb1d58c 100644
@@ -164,6 +164,9 @@ void __init free_bootmem_late(unsigned long physaddr, unsigned long size)
 	end = PFN_DOWN(physaddr + size);
 
 	for (; cursor < end; cursor++) {
+		if (!pfn_valid(cursor))
+			continue;
+
 		__free_pages_bootmem(pfn_to_page(cursor), 0);
 		totalram_pages++;
 	}

mm/nobootmem.c +3 -0

index 04a9d94333a5..afad246688ce 100644
@@ -77,6 +77,9 @@ void __init free_bootmem_late(unsigned long addr, unsigned long size)
 	end = PFN_DOWN(addr + size);
 
 	for (; cursor < end; cursor++) {
+		if (!pfn_valid(cursor))
+			continue;
+
 		__free_pages_bootmem(pfn_to_page(cursor), 0);
 		totalram_pages++;
 	}

Signature · 1 line

1.9.0

Dave Hansen

2014-05-27 18:46 UTC #1 lore raw

Quoted 7 lines · Matt Fleming

We need to check that a pfn is valid before handing it to pfn_to_page() since on low memory systems with CONFIG_HIGHMEM=n it's possible that a pfn may not have a corresponding struct page.
This is in fact the case for one of Alan's machines where some of the EFI boot services pages live in highmem, and running a kernel without CONFIG_HIGHMEM enabled results in the following oops

...

Quoted 5 lines

mm/bootmem.c +0 -0

index 90bd3507b413..406e9cb1d58c 100644

mm/bootmem.c void __init free_bootmem_late(unsigned long physaddr, unsigned long size) · line 172

Quoted 9 lines

mm/bootmem.c +3 -0

@@ -164,6 +164,9 @@ void __init free_bootmem_late(unsigned long physaddr, unsigned long size)
 	end = PFN_DOWN(physaddr + size);
 
 	for (; cursor < end; cursor++) {
+		if (!pfn_valid(cursor))
+			continue;
+
 		__free_pages_bootmem(pfn_to_page(cursor), 0);
 		totalram_pages++;
 	}

I don't think this is quite right. pfn_valid() tells us whether we have a 'struct page' there or not. *BUT*, it does not tell us whether it is RAM that we can actually address and than can be freed in to the buddy allocator.

I think sparsemem is where this matters. Let's say mem= caused lowmem to end in the middle of a section (or that 896MB wasn't section-aligned). Then someone calls free_bootmem_late() on an area that is in the last section, but _above_ max_mapnr. It'll be pfn_valid(), we'll free it in to the buddy allocator, and we'll blam the first time we try to write to a bogus vaddr after a phys_to_virt().

At a higher level, I don't like the idea of the bootmem code papering over bugs when somebody calls in to it trying to _free_ stuff that's not memory (as far as the kernel is concerned).

I think the right thing to do is to call in to the e820 code and see if the range is E820_RAM before trying to bootmem-free it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Fleming, Matt

author

2014-06-03 15:17 UTC #2 lore raw

Quoted 6 lines · Dave Hansen

I don't think this is quite right. pfn_valid() tells us whether we have a 'struct page' there or not. *BUT*, it does not tell us whether it is RAM that we can actually address and than can be freed in to the buddy allocator.

Quoted 6 lines

I think sparsemem is where this matters. Let's say mem= caused lowmem to end in the middle of a section (or that 896MB wasn't section-aligned). Then someone calls free_bootmem_late() on an area that is in the last section, but _above_ max_mapnr. It'll be pfn_valid(), we'll free it in to the buddy allocator, and we'll blam the first time we try to write to a bogus vaddr after a phys_to_virt().

Ah, the sparsemem case wasn't something I'd considered. Thanks Dave.

Quoted 6 lines

At a higher level, I don't like the idea of the bootmem code papering over bugs when somebody calls in to it trying to _free_ stuff that's not memory (as far as the kernel is concerned).
I think the right thing to do is to call in to the e820 code and see if the range is E820_RAM before trying to bootmem-free it.

OK, this makes sense. I'll try that approach and see if it also fixes Alan's problem.