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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 19-09-2026 02:54, Matthew Brost wrote:
> 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.

Sure.

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