Re: [PATCH] drm/gpusvm,pagemap: Do not assume DRM pagemap owns device pages

[Francois Dugast <francois.dugast@intel.com>]

On Fri, Apr 10, 2026 at 01:02:00PM -0700, Matthew Brost wrote:
> On Fri, Apr 10, 2026 at 02:46:54PM +0200, Francois Dugast wrote:
> > On Wed, Apr 08, 2026 at 06:55:12PM -0700, Matthew Brost wrote:
> > > Update drm_pagemap_page_zone_device_data() to derive the pgmap ops from
> > > the page and compare them against the DRM pagemap ops. If the ops do not
> > > match, return NULL.
> > > 
> > > Also harden two risky call sites by checking for NULL after
> > > hmm_range_fault() or migrate_vma_setup() when migrating to device
> > > memory, as it is possible to encounter device pages that are not owned
> > > by DRM pagemap.
> > 
> > Shouldn't we also harden other calls to drm_pagemap_page_zone_device_data() in
> > 
> >     drm_pagemap_migrate_map_device_private_pages()
> >     drm_pagemap_migrate_unmap_pages()
> 
> We sanitize prior to this in drm_pagemap_migrate_to_devmem or are
> operating on pages handed back via populate_devmem_pfn.
> 
> >     drm_pagemap_migrate_populate_ram_pfn()
> 
> Operating on page handed back via populate_devmem_pfn. Also wouldn't
> NULL ptr reference.
> 
> >     __drm_pagemap_migrate_to_ram()
> >     drm_pagemap_folio_free()
> >     drm_pagemap_migrate_to_ram()
> 
> These are in the vops path of pagemap we check against.
> 
> >     drm_pagemap_page_to_dpagemap()
> 
> We sanitize prior to this in drm_gpusvm_get_pages.
> 
> Thus all the above sites I figure a warn is enough as it would indicate
> a fairly serious bug in drm gpusvm/pagemap/calling driver which this
> code completely controls.
> 
> The case where I do sanitize - after collection via hmm_range_fault,
> migrate_vma_setup, I think it could be possible an outside driver has
> moved pages to private (very unlikely) and this driver also tries to
> move, so abort rather NULL ptr dereference.

Indeed, thanks for pointing it out and for the explanation.

Reviewed-by: Francois Dugast <francois.dugast@intel.com>

> 
> So basically mitagated the 2 risky places with sanitization. Ofc we
> could check this everywhere...
> 
> Matt
> 
> > 
> > Francois
> > 
> > > 
> > > Suggested-by: sashiko.dev
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_gpusvm.c  |  5 +++++
> > >  drivers/gpu/drm/drm_pagemap.c | 14 ++++++++++----
> > >  include/drm/drm_pagemap.h     |  5 ++++-
> > >  3 files changed, 19 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> > > index 365a9c0b522a..b3cccd047a21 100644
> > > --- a/drivers/gpu/drm/drm_gpusvm.c
> > > +++ b/drivers/gpu/drm/drm_gpusvm.c
> > > @@ -1506,6 +1506,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
> > >  			struct drm_pagemap_zdd *__zdd =
> > >  				drm_pagemap_page_zone_device_data(page);
> > >  
> > > +			if (!__zdd) {
> > > +				err = -EINVAL;
> > > +				goto err_unmap;
> > > +			}
> > > +
> > >  			if (!ctx->allow_mixed &&
> > >  			    zdd != __zdd && i > 0) {
> > >  				err = -EOPNOTSUPP;
> > > diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> > > index d82ea7ccb8da..95c951c5b569 100644
> > > --- a/drivers/gpu/drm/drm_pagemap.c
> > > +++ b/drivers/gpu/drm/drm_pagemap.c
> > > @@ -753,10 +753,16 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,
> > >  				own_pages++;
> > >  				goto next;
> > >  			}
> > > -			cur.dpagemap = src_zdd->dpagemap;
> > > -			cur.ops = src_zdd->devmem_allocation->ops;
> > > -			cur.device = cur.dpagemap->drm->dev;
> > > -			pages[i] = src_page;
> > > +			if (src_zdd) {
> > > +				cur.dpagemap = src_zdd->dpagemap;
> > > +				cur.ops = src_zdd->devmem_allocation->ops;
> > > +				cur.device = cur.dpagemap->drm->dev;
> > > +				pages[i] = src_page;
> > > +			} else {
> > > +				npages = i;
> > > +				err = -EINVAL;
> > > +				goto err_finalize;
> > > +			}
> > >  		}
> > >  		if (!pages[i]) {
> > >  			cur.dpagemap = NULL;
> > > diff --git a/include/drm/drm_pagemap.h b/include/drm/drm_pagemap.h
> > > index 95eb4b66b057..9b7c50932db5 100644
> > > --- a/include/drm/drm_pagemap.h
> > > +++ b/include/drm/drm_pagemap.h
> > > @@ -367,12 +367,15 @@ int drm_pagemap_reinit(struct drm_pagemap *dpagemap);
> > >   * drm_pagemap_page_zone_device_data() - Page to zone_device_data
> > >   * @page: Pointer to the page
> > >   *
> > > - * Return: Page's zone_device_data
> > > + * Return: Page's zone_device_data if owned by DRM pagemap, NULL otherwise
> > >   */
> > >  static inline struct drm_pagemap_zdd *drm_pagemap_page_zone_device_data(struct page *page)
> > >  {
> > >  	struct folio *folio = page_folio(page);
> > >  
> > > +	if (WARN_ON_ONCE(page_pgmap(page)->ops != drm_pagemap_pagemap_ops_get()))
> > > +		return NULL;
> > > +
> > >  	return folio_zone_device_data(folio);
> > >  }
> > >  
> > > -- 
> > > 2.34.1
> > >