Re: [RFC 2/4] drm/gpusvm: Add devmem callback to get_pages

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Sep 16, 2026 at 11:36:38AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] drm/gpusvm: Type confusion and memory corruption on foreign device coherent pages
> - [Low] drm/gpusvm: Non-contiguous device memory runs merged when separated by system pages
> --
> 
> commit 26edb821373f785b22418001719e2435aed329fe
> Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> 
> drm/gpusvm: Add devmem callback to get_pages
> 
> This patch adds an optional drm_gpusvm_ctx.devmem_fn callback to get_pages.
> The callback is invoked for each contiguous run of pages backed by the same
> device-memory allocation when processing faulted pages under the notifier
> lock.
> 
> > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> > index b6c9d3a07dc81..6cec70d6f1859 100644
> > --- a/drivers/gpu/drm/drm_gpusvm.c
> > +++ b/drivers/gpu/drm/drm_gpusvm.c
> [ ... ]
> > +static void drm_gpusvm_walk_devmem(struct drm_gpusvm *gpusvm,
> > +				   unsigned long *pfns,
> > +				   unsigned long npages,
> > +				   const struct drm_gpusvm_ctx *ctx)
> > +{
> > +	struct drm_pagemap_devmem *last = NULL;
> > +	unsigned int order = 0;
> > +	unsigned long i;
> > +
> > +	lockdep_assert_held(&gpusvm->notifier_lock);
> > +
> > +	for (i = 0; i < npages; i += 1 << order) {
> > +		struct page *page = hmm_pfn_to_page(pfns[i]);
> > +		struct drm_pagemap_devmem *devmem;
> > +
> > +		order = drm_gpusvm_hmm_pfn_to_order(pfns[i], i, npages);
> > +
> > +		if (!is_device_private_page(page) &&
> > +		    !is_device_coherent_page(page))
> > +			continue;
> 
> [Severity: Low]
> Does this continue statement incorrectly skip resetting the 'last' tracking
> pointer for system pages?
> 
> If a virtual memory range contains a device memory run, followed by system
> memory, and then the same device memory run again, 'last' will retain its
> previous value during the system memory iteration. When the second run of
> device memory is reached, the subsequent 'devmem == last' check will
> evaluate to true, incorrectly skipping the callback for the second run and
> violating the API contract that non-contiguous allocations are reported more
> than once.
> 

I'm not sure what semantics we want here but per kernel doc last should
get reset to NULL on the continue. In practice likely doesn't matter
though but I guess let's adhere to the kernel doc.

> > +
> > +		devmem = drm_pagemap_page_to_devmem(page);
> 
> [Severity: High]
> Can this cause type confusion and memory corruption if the page is a foreign
> device coherent page?
> 
> HMM's hmm_range_fault() does not filter device coherent pages by
> dev_private_owner, so an unprivileged user mapping a device coherent page
> from a foreign driver (e.g., AMDGPU) will pass the
> is_device_coherent_page(page) check above.
> 
> The call to drm_pagemap_page_to_devmem(page) will then blindly cast the
> foreign driver's zone_device_data to a struct drm_pagemap_zdd * and
> dereference it, leading to invalid memory access.

This is probably right for coherent but likely out of scope for this
patch as all GPUSVM is broken here.

I tried to fix this here [1] but Thomas didn't like what I came up with.

Let's maybe throw this one on the backlog of known issues that should be
cleaned up.

Matt

[1] https://patchwork.freedesktop.org/patch/717215/?series=164587&rev=1

> 
> > +		if (devmem == last)
> > +			continue;
> > +
> > +		last = devmem;
> > +		ctx->devmem_fn(devmem);
> > +	}
> > +}
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260916112828.2528881-6-himal.prasad.ghimiray@intel.com?part=2