* RE: [PATCH] swiotlb: use the adjusted address for the highmem page lookup
2026-09-05 8:42 ` [PATCH] swiotlb: use the adjusted address for the highmem page lookup Donggeun Yoo
@ 2026-09-05 15:45 ` Michael Kelley
2026-09-05 18:54 ` Donggeun Yoo
2026-09-05 20:08 ` Michael Kelley
2026-09-09 9:30 ` Marek Szyprowski
2 siblings, 1 reply; 6+ messages in thread
From: Michael Kelley @ 2026-09-05 15:45 UTC (permalink / raw)
To: Donggeun Yoo, Marek Szyprowski
Cc: Robin Murphy, Konrad Rzeszutek Wilk, Chanho Park, Bumyong Lee,
iommu@lists.linux.dev, linux-kernel@vger.kernel.org,
stable@vger.kernel.org
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] swiotlb: use the adjusted address for the highmem page lookup
2026-09-05 15:45 ` Michael Kelley
@ 2026-09-05 18:54 ` Donggeun Yoo
2026-09-05 20:07 ` Michael Kelley
0 siblings, 1 reply; 6+ messages in thread
From: Donggeun Yoo @ 2026-09-05 18:54 UTC (permalink / raw)
To: Michael Kelley
Cc: Marek Szyprowski, Robin Murphy, Konrad Rzeszutek Wilk,
Chanho Park, Bumyong Lee, iommu, linux-kernel, stable,
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] swiotlb: use the adjusted address for the highmem page lookup
2026-09-05 18:54 ` Donggeun Yoo
@ 2026-09-05 20:07 ` Michael Kelley
0 siblings, 0 replies; 6+ messages in thread
From: Michael Kelley @ 2026-09-05 20:07 UTC (permalink / raw)
To: Donggeun Yoo, Michael Kelley
Cc: Marek Szyprowski, Robin Murphy, Konrad Rzeszutek Wilk,
Chanho Park, Bumyong Lee, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] swiotlb: use the adjusted address for the highmem page lookup
2026-09-05 8:42 ` [PATCH] swiotlb: use the adjusted address for the highmem page lookup Donggeun Yoo
2026-09-05 15:45 ` Michael Kelley
@ 2026-09-05 20:08 ` Michael Kelley
2026-09-09 9:30 ` Marek Szyprowski
2 siblings, 0 replies; 6+ messages in thread
From: Michael Kelley @ 2026-09-05 20:08 UTC (permalink / raw)
To: Donggeun Yoo, Marek Szyprowski
Cc: Robin Murphy, Konrad Rzeszutek Wilk, Chanho Park, Bumyong Lee,
iommu@lists.linux.dev, linux-kernel@vger.kernel.org,
stable@vger.kernel.org
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] swiotlb: use the adjusted address for the highmem page lookup
2026-09-05 8:42 ` [PATCH] swiotlb: use the adjusted address for the highmem page lookup Donggeun Yoo
2026-09-05 15:45 ` Michael Kelley
2026-09-05 20:08 ` Michael Kelley
@ 2026-09-09 9:30 ` Marek Szyprowski
2 siblings, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2026-09-09 9:30 UTC (permalink / raw)
To: Donggeun Yoo
Cc: Robin Murphy, Konrad Rzeszutek Wilk, Bumyong Lee, iommu,
linux-kernel, stable
On 05.09.2026 10:42, Donggeun Yoo wrote:
> 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>
Applied to dma-mapping-fixes, thanks!
> ---
> 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;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 6+ messages in thread