+Adrian On Tue, 23 Jun 2026 13:41:12 +0100 Akash Goel <[email protected]> wrote:
> On 6/23/26 13:09, Boris Brezillon wrote: > > On Tue, 23 Jun 2026 12:17:51 +0100 > > Akash Goel <[email protected]> wrote: > > > >> Hi Boris > >> > >> On 6/23/26 10:53, Boris Brezillon wrote: > >>> On Tue, 23 Jun 2026 10:24:13 +0100 > >>> Akash Goel <[email protected]> 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 <[email protected]> > >>>> --- > >>>> 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 <[email protected]> > > 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
