Re: [PATCH] drm/panthor: Fix NPD issue on partial unmap of an evicted BO

[Boris Brezillon <boris.brezillon@collabora.com>]

+Adrian

On Tue, 23 Jun 2026 13:41:12 +0100
Akash Goel <akash.goel@arm.com> wrote:

> On 6/23/26 13:09, Boris Brezillon wrote:
> > On Tue, 23 Jun 2026 12:17:51 +0100
> > Akash Goel <akash.goel@arm.com> wrote:
> >   
> >> Hi Boris
> >>
> >> On 6/23/26 10:53, Boris Brezillon wrote:  
> >>> On Tue, 23 Jun 2026 10:24:13 +0100
> >>> Akash Goel <akash.goel@arm.com> wrote:
> >>>      
> >>>> This commit fixes the NULL pointer dereference issue that would have
> >>>> happened on the split of GPU mapping due to partial unmap of an evicted
> >>>> BO. There is a logic to handle the partial unmap of huge pages when the
> >>>> GPU mapping is split. That logic was not being completely skipped for
> >>>> the VMA of an evicted BO and that resulted in a NPD possibility for the
> >>>> 'bo->backing.pages' pointer, which is set to NULL when pages of a
> >>>> BO are released on eviction.
> >>>>     
> >>  
> >>>>
> >>>> Fixes: 8e7460eac786 ("drm/panthor: Support partial unmaps of huge pages")
> >>>> Signed-off-by: Akash Goel <akash.goel@arm.com>
> >>>> ---
> >>>>    drivers/gpu/drm/panthor/panthor_mmu.c | 26 +++++++++++++-------------
> >>>>    1 file changed, 13 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> >>>> index 31cc57029c12..285e7b9bc100 100644
> >>>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> >>>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> >>>> @@ -2358,20 +2358,20 @@ static int panthor_gpuva_sm_step_remap(struct drm_gpuva_op *op,
> >>>>    	 */
> >>>>    	panthor_fix_sparse_map_offset(op->remap.next, unmap_vma->flags);
> >>>>    
> >>>> -	/*
> >>>> -	 * ARM IOMMU page table management code disallows partial unmaps of huge pages,
> >>>> -	 * so when a partial unmap is requested, we must first unmap the entire huge
> >>>> -	 * page and then remap the difference between the huge page minus the requested
> >>>> -	 * unmap region. Calculating the right start address and range for the expanded
> >>>> -	 * unmap operation is the responsibility of the following function.
> >>>> -	 */
> >>>> -	unmap_hugepage_align(&op->remap, &unmap_start, &unmap_range);
> >>>> -
> >>>> -	/* If the range changed, we might have to lock a wider region to guarantee
> >>>> -	 * atomicity. panthor_vm_lock_region() bails out early if the new region
> >>>> -	 * is already part of the locked region, so no need to do this check here.
> >>>> -	 */
> >>>>    	if (!unmap_vma->evicted) {
> >>>> +		/*
> >>>> +		* ARM IOMMU page table management code disallows partial unmaps of huge pages,
> >>>> +		* so when a partial unmap is requested, we must first unmap the entire huge
> >>>> +		* page and then remap the difference between the huge page minus the requested
> >>>> +		* unmap region. Calculating the right start address and range for the expanded
> >>>> +		* unmap operation is the responsibility of the following function.
> >>>> +		*/
> >>>> +		unmap_hugepage_align(&op->remap, &unmap_start, &unmap_range);
> >>>> +
> >>>> +		/* If the range changed, we might have to lock a wider region to guarantee
> >>>> +		 * atomicity. panthor_vm_lock_region() bails out early if the new region
> >>>> +		 * is already part of the locked region, so no need to do this check here.
> >>>> +		 */
> >>>>    		panthor_vm_lock_region(vm, unmap_start, unmap_range);
> >>>>    		panthor_vm_unmap_pages(vm, unmap_start, unmap_range);
> >>>>    	}  
> >>>
> >>>
> >>> I think we want something like that instead, so we can keep the
> >>> 2M alignment for sparse mappings which go recently introduced.
> >>>      
> >>
> >> Thanks for the suggestion. But sorry I didn't get it.
> >>
> >> I see that the patching of 'op->remap.next->gem.offset' would still be
> >> done with my change.
> >>
> >> 	panthor_fix_sparse_map_offset(op->remap.next, unmap_vma->flags);
> >>
> >> 	if (!unmap_vma->evicted) {
> >> 		unmap_hugepage_align(&op->remap, &unmap_start,
> >>
> >> IIUC, the 2M alignment is done to avoid a potential partial unmap of 2M
> >> page. But if the VMA is in evicted state then already the unmap would
> >> have happened for the whole virtual range covered by the VMA.  
> > 
> > Nah, you're correct, the patching of the drm_gpuva is independent of the
> > adjusted unmap range, so we should be good even if we don't adjust this
> > range for evicted sparse mappings. Sorry for the noise.
> >   
> 
> No worries. Thanks for confirming.
> 
> Since I had a closer look at the code, sorry I have another doubt.
> 
> Do we really need the call to 'panthor_fix_sparse_map_offset()' in the 
> following code block ?. The 'op->remap.next->gem.offset' would already 
> have been patched before.

That's probably not needed, indeed. Adrian to confirm.

> 
> 
> 	if (op->remap.next) {
> 		u64 addr = op->remap.next->va.addr;
> 		u64 size = unmap_start + unmap_range - op->remap.next->va.addr;
> 
> 		if (!unmap_vma->evicted && size > 0) {
> 			struct drm_gpuva_op_map map_op = {
> 				.va.addr = addr,
> 				.va.range = size,
> 				.gem.obj = op->remap.next->gem.obj,
> 				.gem.offset = op->remap.next->gem.offset,
> 			};
> 			panthor_fix_sparse_map_offset(&map_op, unmap_vma->flags);
> 
> 			ret = panthor_vm_exec_map_op(vm, unmap_vma->flags, &map_op);
> 
> 
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Sorry I realized that indentation needs to be fixed in my patch.
> 
> Will send a v2 and ad your r-b tag.

Sounds good.

Thanks,

Boris