[PATCH] swiotlb: use the adjusted address for the highmem page lookup

[PATCH] swiotlb: use the adjusted address for the highmem page lookup

[Donggeun Yoo]

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: stable@vger.kernel.org
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
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(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index ded7016a46a7..aa2f1c4588b9 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -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

RE: [PATCH] swiotlb: use the adjusted address for the highmem page lookup

[Michael Kelley]

From: Donggeun Yoo <donggeunyoo.kernel@gmail.com> Sent: Saturday, September 5, 2026 1:42 AM
> 
> 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.

Yes -- this all makes sense. Current code is clearly using the wrong starting
pfn in the highmem branch, and your fix looks good.

> 
> 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().

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.

Michael

> 
> 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: stable@vger.kernel.org
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> 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(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index ded7016a46a7..aa2f1c4588b9 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -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
> 

Re: [PATCH] swiotlb: use the adjusted address for the highmem page lookup

[Donggeun Yoo]

On Sat, Sep 05, 2026 at 03:45:41PM +0000, Michael Kelley wrote:
> 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.

RE: [PATCH] swiotlb: use the adjusted address for the highmem page lookup

[Michael Kelley]

From: Donggeun Yoo <donggeunyoo.kernel@gmail.com> Sent: Saturday, September 5, 2026 11:55 AM
> 
> On Sat, Sep 05, 2026 at 03:45:41PM +0000, Michael Kelley wrote:
> > 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. 

Yes, that was exactly my question. I worded it poorly.

> 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.

So there's exactly one case, which is when a merged range crosses
the boundary between lowmem and highmem.  And the range in
that case always starts with lowmem and transitions to highmem.
There's not a case where lowmem and highmem are intermixed
in a range in arbitrary ways.  And there's no case that starts with
highmem and transitions to lowmem (though that would just work
with the current test).

> 
> 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.

OK, yes.  That makes sense.

> 
> 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.

Yep. I thought that would be the case. Using kmap_local_page()
is slower, but the case where the range starts with lowmem and
transition to highmem is a rarity. Please leave a comment in the
code about the reasoning behind the strange-looking test.

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

Works for me. Thanks!

Michael

RE: [PATCH] swiotlb: use the adjusted address for the highmem page lookup

[Michael Kelley]

From: Donggeun Yoo <donggeunyoo.kernel@gmail.com> Sent: Saturday, September 5, 2026 1:42 AM
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

> ---
> 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(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index ded7016a46a7..aa2f1c4588b9 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -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
>