[PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Christian A. Ehrhardt]

When checking a different patch[1] sashiko AI pointed out that
panthor needs the same fix[2]:

In the ->huge_fault handler do not install a PMD huge page
mapping if the huge page exceeds the boundaries of the VMA.

[1] https://lore.kernel.org/lkml/20260622215718.1532689-1-lk@c--e.de/
[2] https://sashiko.dev/#/patchset/20260622215718.1532689-1-lk%40c--e.de

Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Fixes: 68cbf96b1e9b ("drm/panthor: Part ways with drm_gem_shmem_object")
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 drivers/gpu/drm/panthor/panthor_gem.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

NOTE:
The panthor version is only compile tested because I don't
have the hardware. However, the code is identical to that
fixed in [1] and I have a reproducer for that.

No need for for stable backports. The code is new in 7.1.

diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index a1e2eb1ca7bb..54535bae2b0c 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -802,9 +802,13 @@ static vm_fault_t insert_page(struct vm_fault *vmf, unsigned int order, struct p
 	} else if (order == PMD_ORDER) {
 		unsigned long pfn = page_to_pfn(page);
 		unsigned long paddr = pfn << PAGE_SHIFT;
+		struct vm_area_struct *vma = vmf->vma;
+		unsigned long start = ALIGN_DOWN(vmf->address, PMD_SIZE);
+		unsigned long end = start + PMD_SIZE;
+		bool in_range = vma->vm_start <= start && end <= vma->vm_end;
 		bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
 
-		if (aligned &&
+		if (aligned && in_range &&
 		    folio_test_pmd_mappable(page_folio(page))) {
 			pfn &= PMD_MASK >> PAGE_SHIFT;
 			return vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);
-- 
2.43.0

Re: [PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Matthew Wilcox]

On Tue, Jun 23, 2026 at 08:19:42PM +0200, Christian A. Ehrhardt wrote:
> The panthor version is only compile tested because I don't
> have the hardware. However, the code is identical to that
> fixed in [1] and I have a reproducer for that.
> 
> No need for for stable backports. The code is new in 7.1.

What documentation did you need to see to persuade you to use
map_pages instead of writing a huge_fault handler?

(yes, you're all Doing It Wrong, please stop, but help us help you)

Re: [PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Boris Brezillon]

Hi Matthew,

On Tue, 23 Jun 2026 19:33:42 +0100
Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Jun 23, 2026 at 08:19:42PM +0200, Christian A. Ehrhardt wrote:
> > The panthor version is only compile tested because I don't
> > have the hardware. However, the code is identical to that
> > fixed in [1] and I have a reproducer for that.
> > 
> > No need for for stable backports. The code is new in 7.1.  
> 
> What documentation did you need to see to persuade you to use
> map_pages instead of writing a huge_fault handler?
> 
> (yes, you're all Doing It Wrong, please stop, but help us help you)

We tried [1], but couldn't figure out how to make it work with the
current locking in the gem_shmem logic, so we decided to postpone the
map_page() addition. We plan to get back to it and implement map_page(),
but my understanding is that we still need the fault()+huge_fault() as a
fallback for when we can't acquire the locks we need to service the
fault. Oh, and BTW, we had just the fault() implementation in the
version that was merged, no huge_fault(), but this fault()
implementation was mapping PMDs which led to another issue [2] that
forced us to add this huge_fault() implem.

I'm all for improving that, but if I'm completely honest, there are
times where our MM-related questions are left unanswered ([3] is one
example), and it gets hard to figure out how to do things properly
without proper guidance from the MM experts. I thought [2] was one of
these, but apparently the MM list/maintainers were not Cc-ed on that
one, oops.

Just to be clear, I'm not blaming anyone here (I know how busy subsystem
maintainers are in general), I'm just trying to explain how we ended up
with these new huge_fault() implementations (one being a fork of the
original implementation) that you were not expecting, but also wanted to
point out that we've been pretty transparent about where we were heading
during the submission process, and we didn't get a clear "no, stop
there!" back then.

Anyway, I'll try to get back to this map_pages() implementation, and
see how we can handle the fallback when locks can't be acquired in that
path (I assume this would mean no PMD mapping in the fallback path if
we get rid of huge_fault()).

Regards,

Boris

[1]https://patchew.org/linux/20251015153018.43735-1-loic.molinari@collabora.com/20251015153018.43735-4-loic.molinari@collabora.com/
[2]https://lore.kernel.org/all/20260319015224.46896-1-pedrodemargomes@gmail.com/
[3]https://lore.kernel.org/all/20260319155028.291f3157@fedora/

Re: [PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Boris Brezillon]

On Tue, 23 Jun 2026 22:03:41 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Anyway, I'll try to get back to this map_pages() implementation, and
> see how we can handle the fallback when locks can't be acquired in that
> path (I assume this would mean no PMD mapping in the fallback path if
> we get rid of huge_fault()).

More on that. I had a brief look at map_pages()/fault_around(), and it
seems it's only meant to populate PTE entries, not PMDs. Part of the
perf boost we were looking for when adding huge_fault() was limiting
the number of faults when adjacent pages are touched, but the other
part was limiting the page table depth to optimize page-table walks, so
if we're implementing map_pages() without huge_fault(), we basically
lose the second part of the optimization. I'd need to check how much
perf we leave on the table with that.

Re: [PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Boris Brezillon]

On Tue, 23 Jun 2026 22:44:21 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue, 23 Jun 2026 22:03:41 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > Anyway, I'll try to get back to this map_pages() implementation, and
> > see how we can handle the fallback when locks can't be acquired in that
> > path (I assume this would mean no PMD mapping in the fallback path if
> > we get rid of huge_fault()).  
> 
> More on that. I had a brief look at map_pages()/fault_around(), and it
> seems it's only meant to populate PTE entries, not PMDs.

I was wrong, filemap_map_pages() clearly attempts a huge_page map if
the conditions are met (proper alignment of VMA, folio is huge, ...).

Now, I have several questions around how to implement map_pages()
properly in our case.

Ideally we would use a generic helper like filemap_map_pages(), which
basically all implementer of .map_pages() are using, because the loop,
checks and locking is far from trivial there. Unfortunately we can't
really use that one because
   a. in our case, the vm_file attached to the vma is a pseudo file that
      doesn't really back the data. Things are redirected internally to
      another file object that's backed by shmem, meaning the
      vmf->vm_pgoff and vmf->vma info are unusable as-is
   b. we use PFN insertion instead of page insertion since [1]

I'm sure we can hand-roll our own map_pages() implementation, but
again, we would need custom versions of the vmf_insert_pfn() to have an
optimal version that can batch multiple PTE updates (just like
filemap_map_folio_range() does). The pte/pmd locking also looks a bit
different too in map_pages(). These are all rather tricky details that
are hard to sort out when you don't know MM internals and the various
pitfalls around page table updates. Long story short, if you really want
us to move away from .huge_fault(), we're gonna need a bit of
hand-holding, because otherwise we're shooting in the dark.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v7.1&id=8b93d1d7dbd578fd296e70008b29c0f62d09d7cb

Re: [PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Boris Brezillon]

On Tue, 23 Jun 2026 20:19:42 +0200
"Christian A. Ehrhardt" <lk@c--e.de> wrote:

> When checking a different patch[1] sashiko AI pointed out that
> panthor needs the same fix[2]:
> 
> In the ->huge_fault handler do not install a PMD huge page
> mapping if the huge page exceeds the boundaries of the VMA.
> 
> [1] https://lore.kernel.org/lkml/20260622215718.1532689-1-lk@c--e.de/
> [2] https://sashiko.dev/#/patchset/20260622215718.1532689-1-lk%40c--e.de
> 
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Fixes: 68cbf96b1e9b ("drm/panthor: Part ways with drm_gem_shmem_object")
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

I know the discussion is ongoing to decide what we should do about
these huge_fault() handlers, but I think it's worth getting this fix in
in the meantime.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panthor/panthor_gem.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> NOTE:
> The panthor version is only compile tested because I don't
> have the hardware. However, the code is identical to that
> fixed in [1] and I have a reproducer for that.
> 
> No need for for stable backports. The code is new in 7.1.
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index a1e2eb1ca7bb..54535bae2b0c 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -802,9 +802,13 @@ static vm_fault_t insert_page(struct vm_fault *vmf, unsigned int order, struct p
>  	} else if (order == PMD_ORDER) {
>  		unsigned long pfn = page_to_pfn(page);
>  		unsigned long paddr = pfn << PAGE_SHIFT;
> +		struct vm_area_struct *vma = vmf->vma;
> +		unsigned long start = ALIGN_DOWN(vmf->address, PMD_SIZE);
> +		unsigned long end = start + PMD_SIZE;
> +		bool in_range = vma->vm_start <= start && end <= vma->vm_end;
>  		bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
>  
> -		if (aligned &&
> +		if (aligned && in_range &&
>  		    folio_test_pmd_mappable(page_folio(page))) {
>  			pfn &= PMD_MASK >> PAGE_SHIFT;
>  			return vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);

Re: [PATCH] drm/panthor: Check VMA boundaries for PMD mappings

[Boris Brezillon]

On Wed, 24 Jun 2026 12:26:36 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue, 23 Jun 2026 20:19:42 +0200
> "Christian A. Ehrhardt" <lk@c--e.de> wrote:
> 
> > When checking a different patch[1] sashiko AI pointed out that
> > panthor needs the same fix[2]:
> > 
> > In the ->huge_fault handler do not install a PMD huge page
> > mapping if the huge page exceeds the boundaries of the VMA.
> > 
> > [1] https://lore.kernel.org/lkml/20260622215718.1532689-1-lk@c--e.de/
> > [2] https://sashiko.dev/#/patchset/20260622215718.1532689-1-lk%40c--e.de
> > 
> > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > Cc: Steven Price <steven.price@arm.com>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Fixes: 68cbf96b1e9b ("drm/panthor: Part ways with drm_gem_shmem_object")
> > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>  
> 
> I know the discussion is ongoing to decide what we should do about
> these huge_fault() handlers, but I think it's worth getting this fix in
> in the meantime.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

BTW, I saw shashiko complain about our page_offset calculation which
should look something like

	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);

to be immune to the vma->vm_{start,pgoff} adjustments done when the
VMA is split.

> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_gem.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > NOTE:
> > The panthor version is only compile tested because I don't
> > have the hardware. However, the code is identical to that
> > fixed in [1] and I have a reproducer for that.
> > 
> > No need for for stable backports. The code is new in 7.1.
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > index a1e2eb1ca7bb..54535bae2b0c 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > @@ -802,9 +802,13 @@ static vm_fault_t insert_page(struct vm_fault *vmf, unsigned int order, struct p
> >  	} else if (order == PMD_ORDER) {
> >  		unsigned long pfn = page_to_pfn(page);
> >  		unsigned long paddr = pfn << PAGE_SHIFT;
> > +		struct vm_area_struct *vma = vmf->vma;
> > +		unsigned long start = ALIGN_DOWN(vmf->address, PMD_SIZE);
> > +		unsigned long end = start + PMD_SIZE;
> > +		bool in_range = vma->vm_start <= start && end <= vma->vm_end;
> >  		bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
> >  
> > -		if (aligned &&
> > +		if (aligned && in_range &&
> >  		    folio_test_pmd_mappable(page_folio(page))) {
> >  			pfn &= PMD_MASK >> PAGE_SHIFT;
> >  			return vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);  
>