per_cpu_ptr_to_phys() failure on UV x86_64

phys_addr_t per_cpu_ptr_to_phys(void *addr)
{

mm/percpu.c +3 -0

+	if ((unsigned long)addr < VMALLOC_START ||
+	    (unsigned long)addr >= VMALLOC_END)
+		return __pa(addr);

(scratching head...) So, that means it's given an address for which !pcpu_addr_in_first_chunk() but outside of vmalloc area. Strange. I'll find out what's going on.

Quoted 3 lines · Tejun Heo

(scratching head...) So, that means it's given an address for which !pcpu_addr_in_first_chunk() but outside of vmalloc area. Strange. I'll find out what's going on.

Does the following patch work? The original patch assumed that @addr would be the address of the base cpu which isn't true. I only compile tested the patch so it might be broken (sorry, I gotta go somewhere now) but this should be the right direction.

mm/percpu.c +17 -8

index 46485e1..8956155 100644
@@ -978,14 +978,23 @@ bool is_kernel_percpu_address(unsigned long addr)
  */
 phys_addr_t per_cpu_ptr_to_phys(void *addr)
 {
-	if (pcpu_addr_in_first_chunk(addr)) {
-		if ((unsigned long)addr < VMALLOC_START ||
-		    (unsigned long)addr >= VMALLOC_END)
-			return __pa(addr);
-		else
-			return page_to_phys(vmalloc_to_page(addr));
-	} else
-		return page_to_phys(pcpu_addr_to_page(addr));
+	void __percpu *base = __addr_to_pcpu_ptr(pcpu_base_addr);
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		void *start = per_cpu_ptr(base, cpu);
+
+		if (addr >= start && addr < start + pcpu_unit_size) {
+			/* in the first chunk */
+			if ((unsigned long)addr < VMALLOC_START ||
+			    (unsigned long)addr >= VMALLOC_END)
+				return __pa(addr);
+			else
+				return page_to_phys(vmalloc_to_page(addr));
+		}
+	}
+	/* in one of the other chunks */
+	return page_to_phys(pcpu_addr_to_page(addr));
 }
 
 static inline size_t pcpu_calc_fc_sizes(size_t static_size,

Signature · 2 lines

-- 
tejun

per_cpu_ptr_to_phys() determines whether the passed in @addr belongs to the first_chunk or not by just matching the address against the address range of the base unit (unit0, used by cpu0). When an adress from another cpu was passed in, it will always determine that the address doesn't belong to the first chunk even when it does. This makes the function return a bogus physical address which may lead to crash.

This problem was discovered by Cliff Wickman while investigating a crash during kdump on a SGI UV system.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Cliff Wickman <[email protected]>

---
Can you please verify this one? I added a small optimization so that it doesn't suck too bad on large machines and it works fine here but it would be great to have your Tested-by:.

Thanks.

 mm/percpu.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

mm/percpu.c +28 -3

index 46485e1..6470e77 100644
@@ -229,8 +229,8 @@ static int __maybe_unused pcpu_page_idx(unsigned int cpu, int page_idx)
 	return pcpu_unit_map[cpu] * pcpu_unit_pages + page_idx;
 }
 
-static unsigned long __maybe_unused pcpu_chunk_addr(struct pcpu_chunk *chunk,
-						unsigned int cpu, int page_idx)
+static unsigned long pcpu_chunk_addr(struct pcpu_chunk *chunk,
+				     unsigned int cpu, int page_idx)
 {
 	return (unsigned long)chunk->base_addr + pcpu_unit_offsets[cpu] +
 		(page_idx << PAGE_SHIFT);

@@ -978,7 +978,32 @@ bool is_kernel_percpu_address(unsigned long addr)
  */
 phys_addr_t per_cpu_ptr_to_phys(void *addr)
 {
-	if (pcpu_addr_in_first_chunk(addr)) {
+	void __percpu *base = __addr_to_pcpu_ptr(pcpu_base_addr);
+	bool in_first_chunk = false;
+	unsigned long first_start, first_end;
+	unsigned int cpu;
+
+	/*
+	 * The following test on first_start/end isn't strictly
+	 * necessary but will speed up lookups of addresses which
+	 * aren't in the first chunk.
+	 */
+	first_start = pcpu_chunk_addr(pcpu_first_chunk, pcpu_first_unit_cpu, 0);
+	first_end = pcpu_chunk_addr(pcpu_first_chunk, pcpu_last_unit_cpu,
+				    pcpu_unit_pages);
+	if ((unsigned long)addr >= first_start &&
+	    (unsigned long)addr < first_end) {
+		for_each_possible_cpu(cpu) {
+			void *start = per_cpu_ptr(base, cpu);
+
+			if (addr >= start && addr < start + pcpu_unit_size) {
+				in_first_chunk = true;
+				break;
+			}
+		}
+	}
+
+	if (in_first_chunk) {
 		if ((unsigned long)addr < VMALLOC_START ||
 		    (unsigned long)addr >= VMALLOC_END)
 			return __pa(addr);

Signature · 1 line

1.6.4.2

Quoted 11 lines · Tejun Heo

per_cpu_ptr_to_phys() determines whether the passed in @addr belongs to the first_chunk or not by just matching the address against the address range of the base unit (unit0, used by cpu0). When an adress from another cpu was passed in, it will always determine that the address doesn't belong to the first chunk even when it does. This makes the function return a bogus physical address which may lead to crash.

This problem was discovered by Cliff Wickman while investigating a crash during kdump on a SGI UV system.

Quoted 6 lines

Signed-off-by Tejun Heo <[email protected]>

Reported-by Cliff Wickman <[email protected]>

---
Can you please verify this one? I added a small optimization so that it doesn't suck too bad on large machines and it works fine here but it would be great to have your Tested-by:.

Yep. Works fine on 32p UV.

Tested-by Cliff Wickman <[email protected]>