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

[sashiko-bot@kernel.org]

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.

> +
> +		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.

> +		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