swiotlb: use the adjusted address for the highmem page lookup

swiotlb_bounce() reads the page frame number from the slot's recorded orig_addr, then advances orig_addr by tlb_offset to reach the address the caller asked about. The highmem branch mixes the two: the offset within the page comes from the adjusted address, the page from the value before it.

Once the adjustment crosses a page boundary the pair no longer describes one location, and the whole copy lands one page below the intended one for a positive tlb_offset, one above for a negative one. DMA_FROM_DEVICE writes the device data over the wrong page and leaves the intended one stale, DMA_TO_DEVICE feeds the device from a page the mapping may not cover. Partial syncs through dma_sync_single_range_for_*() are what make tlb_offset non-zero.

The branch test is picked the same way, so a slot recorded in lowmem can be adjusted into highmem and the lowmem path then hands a highmem address to phys_to_virt().

Take both from orig_addr once it is final and keep pfn in the branch that uses it. PhysHighMem() asks the question straight from the address, as dma-debug already does.

Fixes 5f89468e2f06 ("swiotlb: manipulate orig_addr when tlb_addr has offset")

Cc [email protected]

Signed-off-by Donggeun Yoo <[email protected]>

---
Reproduced under QEMU (qemu-system-arm -M virt, 2G) with a 32-bit ARM kernel built from multi_v7_defconfig plus CONFIG_ARM_LPAE=y and CONFIG_HIGHMEM=y, which brings in CONFIG_SWIOTLB, booted with swiotlb=force. A test module maps two highmem pages at page offset 3840, writes a pattern into the bounce buffer 500 bytes in and calls dma_sync_single_range_for_cpu() over that range, so that 3840 + 500 crosses into the second page:

before: swbug: pages pfn=700d4 highmem=1

          swbug: orig phys=700d4f00 (page off 3840)
          swbug: RESULT page0_off=244 page1_off=-1

after: swbug: RESULT page0_off=-1 page1_off=244

The pattern lands one page below its intended location without the change and in the right place with it.

 kernel/dma/swiotlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/dma/swiotlb.c +2 -3

index ded7016a46a7..aa2f1c4588b9 100644
@@ -1019,7 +1019,6 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size
 	int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
 	phys_addr_t orig_addr = mem->slots[index].orig_addr;
 	size_t alloc_size = mem->slots[index].alloc_size;
-	unsigned long pfn = PFN_DOWN(orig_addr);
 	unsigned char *vaddr = mem->vaddr + tlb_addr - mem->start;
 	int tlb_offset;
 

@@ -1052,7 +1051,8 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size
 		size = alloc_size;
 	}
 
-	if (PageHighMem(pfn_to_page(pfn))) {
+	if (PhysHighMem(orig_addr)) {
+		unsigned long pfn = PFN_DOWN(orig_addr);
 		unsigned int offset = orig_addr & ~PAGE_MASK;
 		struct page *page;
 		unsigned int sz = 0;
--

2.53.0

I don't understand this paragraph, but that may be because I'm not that familiar with highmem. Are all the slots making up a particular swiotlb mapping either highmem or lowmem? If a mixture is possible, then a partial sync could start somewhere in a lowmem page and cross over into a highmem page, which would break.

Quoted 5 lines

I don't understand this paragraph, but that may be because I'm not that familiar with highmem. Are all the slots making up a particular swiotlb mapping either highmem or lowmem? If a mixture is possible, then a partial sync could start somewhere in a lowmem page and cross over into a highmem page, which would break.

The slots are always lowmem: the pool comes from memblock_alloc_low() and swiotlb holds one kernel address for it in mem->vaddr. PageHighMem() here asks about the pages behind orig_addr, so the question is whether one mapping's original buffer can span both.

It can. ZONE_NORMAL ends at
max_low_pfn and ZONE_HIGHMEM starts there, and while the page allocator will not hand out a run across that line, pages_are_mergeable() and bvec_try_merge_page() merge on physical adjacency alone.

Your case is real, and this patch does not fix it. On a 32-bit ARM guest (multi_v7_defconfig plus ARM_LPAE and HIGHMEM, 2G, swiotlb=force) with lowmem ending at pfn 0x70000 and high_memory at f0000000:

orig=6ffffe00 len=1024 last=700001ff
mainline PageHighMem(pfn_to_page(PFN_DOWN(orig))) = 0
this patch PhysHighMem(orig) = 0
phys_to_virt(last) = f00001ff, past high_memory

dma_map_page(pfn 0x6ffff, off 3584, len 1024, TO_DEVICE)

  Unable to handle kernel paging request at virtual address f0000000
  Internal error: Oops: 206 [#1] SMP ARM
  PC is at mmiocpy+0x4c/0x334
   dma_map_page_attrs from ...

The same oops with and without this patch, and no partial sync is needed for it: the bounce at map time does it.

is_highmem() is monotonic in the pfn, since ZONE_HIGHMEM and a ZONE_MOVABLE carved out of it are the highest zones, so testing the last byte alone covers your case and this one, at the cost of the single test already there. On top of this patch:

	-	if (PhysHighMem(orig_addr)) {
	+	if (PhysHighMem(orig_addr + size - 1)) {

The loop copies through kmap_local_page(), which is fine for a lowmem page, so entering it for a range that only ends in highmem is correct. That guest survives the map above with it.

The second one predates 5f89468e2f06, so I will send it separately.

swiotlb_bounce() reads the page frame number from the slot's recorded orig_addr, then advances orig_addr by tlb_offset to reach the address the caller asked about. The highmem branch mixes the two: the offset within the page comes from the adjusted address, the page from the value before it.

Once the adjustment crosses a page boundary the pair no longer describes one location, and the whole copy lands one page below the intended one for a positive tlb_offset, one above for a negative one. DMA_FROM_DEVICE writes the device data over the wrong page and leaves the intended one stale, DMA_TO_DEVICE feeds the device from a page the mapping may not cover. Partial syncs through dma_sync_single_range_for_*() are what make tlb_offset non-zero.

The branch test is picked the same way, so a slot recorded in lowmem can be adjusted into highmem and the lowmem path then hands a highmem address to phys_to_virt().

Take both from orig_addr once it is final and keep pfn in the branch that uses it. PhysHighMem() asks the question straight from the address, as dma-debug already does.

Fixes 5f89468e2f06 ("swiotlb: manipulate orig_addr when tlb_addr has offset")

Cc [email protected]

Signed-off-by Donggeun Yoo <[email protected]>

---
Reproduced under QEMU (qemu-system-arm -M virt, 2G) with a 32-bit ARM kernel built from multi_v7_defconfig plus CONFIG_ARM_LPAE=y and CONFIG_HIGHMEM=y, which brings in CONFIG_SWIOTLB, booted with swiotlb=force. A test module maps two highmem pages at page offset 3840, writes a pattern into the bounce buffer 500 bytes in and calls dma_sync_single_range_for_cpu() over that range, so that 3840 + 500 crosses into the second page:

before: swbug: pages pfn=700d4 highmem=1

          swbug: orig phys=700d4f00 (page off 3840)
          swbug: RESULT page0_off=244 page1_off=-1

after: swbug: RESULT page0_off=-1 page1_off=244

The pattern lands one page below its intended location without the change and in the right place with it.

 kernel/dma/swiotlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/dma/swiotlb.c +2 -3

index ded7016a46a7..aa2f1c4588b9 100644
@@ -1019,7 +1019,6 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size
 	int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
 	phys_addr_t orig_addr = mem->slots[index].orig_addr;
 	size_t alloc_size = mem->slots[index].alloc_size;
-	unsigned long pfn = PFN_DOWN(orig_addr);
 	unsigned char *vaddr = mem->vaddr + tlb_addr - mem->start;
 	int tlb_offset;
 

@@ -1052,7 +1051,8 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size
 		size = alloc_size;
 	}
 
-	if (PageHighMem(pfn_to_page(pfn))) {
+	if (PhysHighMem(orig_addr)) {
+		unsigned long pfn = PFN_DOWN(orig_addr);
 		unsigned int offset = orig_addr & ~PAGE_MASK;
 		struct page *page;
 		unsigned int sz = 0;
--

2.53.0

swiotlb_bounce() reads the page frame number from the slot's recorded orig_addr, then advances orig_addr by tlb_offset to reach the address the caller asked about. The highmem branch mixes the two: the offset within the page comes from the adjusted address, the page from the value before it.

Once the adjustment crosses a page boundary the pair no longer describes one location, and the whole copy lands one page below the intended one for a positive tlb_offset, one above for a negative one. DMA_FROM_DEVICE writes the device data over the wrong page and leaves the intended one stale, DMA_TO_DEVICE feeds the device from a page the mapping may not cover. Partial syncs through dma_sync_single_range_for_*() are what make tlb_offset non-zero.

The branch test is picked the same way, so a slot recorded in lowmem can be adjusted into highmem and the lowmem path then hands a highmem address to phys_to_virt().

Take both from orig_addr once it is final and keep pfn in the branch that uses it. PhysHighMem() asks the question straight from the address, as dma-debug already does.

Fixes 5f89468e2f06 ("swiotlb: manipulate orig_addr when tlb_addr has offset")

Cc [email protected]

Signed-off-by Donggeun Yoo <[email protected]>

---
Reproduced under QEMU (qemu-system-arm -M virt, 2G) with a 32-bit ARM kernel built from multi_v7_defconfig plus CONFIG_ARM_LPAE=y and CONFIG_HIGHMEM=y, which brings in CONFIG_SWIOTLB, booted with swiotlb=force. A test module maps two highmem pages at page offset 3840, writes a pattern into the bounce buffer 500 bytes in and calls dma_sync_single_range_for_cpu() over that range, so that 3840 + 500 crosses into the second page:

before: swbug: pages pfn=700d4 highmem=1

          swbug: orig phys=700d4f00 (page off 3840)
          swbug: RESULT page0_off=244 page1_off=-1

after: swbug: RESULT page0_off=-1 page1_off=244

The pattern lands one page below its intended location without the change and in the right place with it.

 kernel/dma/swiotlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/dma/swiotlb.c +0 -1

index ded7016a46a7..aa2f1c4588b9 100644
@@ -1019,7 +1019,6 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size
 	int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
 	phys_addr_t orig_addr = mem->slots[index].orig_addr;
 	size_t alloc_size = mem->slots[index].alloc_size;
-	unsigned long pfn = PFN_DOWN(orig_addr);
 	unsigned char *vaddr = mem->vaddr + tlb_addr - mem->start;
 	int tlb_offset;