Re: [PATCH v9 4/5] drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Device memory mapping leak on error path due to late dpagemap assignment.
- [High] System pages are incorrectly passed to device_unmap during mixed mapping cleanup when IOVA is used.
- [Medium] Direct read of `range->base.flags.migrate_devmem` in `xe_svm_alloc_vram` without `READ_ONCE` can cause a data race.
--

commit e379316e89e810abd61f491527f08c65a115928f
Author: Honglei Huang <honghuan@amd.com>
Subject: drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range

This commit separates physical pages and MM VA ranges abstractly by removing
the embedded pages object from the range structure. It drops range-level page
wrappers in favor of direct driver usage of drm_gpusvm_pages helpers. The
documentation is also updated to reflect the new 1:1 and N:1 driver layouts.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index b0e9a2db10803..e0fd0b2fcc5ee 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c

[ ... ]

> @@ -1638,29 +1695,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_get_pages);

[Severity: High]
This is a pre-existing issue, but does drm_gpusvm_get_pages() permanently
leak device mappings if an error occurs?

When mapping a batch of device pages, if a failure occurs:

drm_gpusvm_get_pages()
    err = -EFAULT;
    goto err_unmap;

The code jumps to err_unmap to rollback, and __drm_gpusvm_unmap_pages()
checks svm_pages->dpagemap to determine if it should invoke device_unmap.
However, svm_pages->dpagemap is only assigned at the very end of the
success path:

    drm_pagemap_put(svm_pages->dpagemap);
    svm_pages->dpagemap = dpagemap;

Because it is still NULL during the error rollback, will the mapped device
pages be skipped and leaked?

[ ... ]

> @@ -1691,29 +1725,6 @@ void drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_unmap_pages);

[Severity: High]
This is a pre-existing issue, but are system pages incorrectly passed to
device_unmap() during mixed mapping cleanup when IOVA is used?

In __drm_gpusvm_unmap_pages(), the cleanup loop checks system pages:

    if (!use_iova && addr->proto == DRM_INTERCONNECT_SYSTEM)
        dma_unmap_page(...);
    else if (dpagemap && dpagemap->ops->device_unmap)
        dpagemap->ops->device_unmap(dpagemap, dev, addr);

If use_iova is true, the first condition evaluates to false for system
pages, causing them to fall through to the else-if branch. Since dpagemap
is non-NULL in a mixed mapping, will the driver-specific device_unmap()
callback be incorrectly executed on the system page?

> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index 3f38283111cca..2862104aa1b20 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h

[ ... ]

> @@ -189,7 +188,6 @@ struct drm_gpusvm_range {
>  	struct kref refcount;
>  	struct interval_tree_node itree;
>  	struct list_head entry;
> -	struct drm_gpusvm_pages pages;
>  	struct drm_gpusvm_range_flags flags;
>  };

[Severity: Medium]
This is a pre-existing issue, but can reading flags.migrate_devmem without
READ_ONCE() cause a data race?

In the xe driver's xe_svm_alloc_vram() function, there is a direct read of
the bitfield:

    xe_assert(range_to_vm(&range->base)->xe,
              range->base.flags.migrate_devmem);

The unmapped and partial_unmap flags within the range->base.flags union
are concurrently updated using WRITE_ONCE() under notifier_lock during MMU
notifier execution (e.g., drm_gpusvm_range_set_unmapped).

Since xe_assert executes outside the notifier_lock, this compiles to a
non-atomic load of the underlying word. Could a concurrent WRITE_ONCE() to
the same word cause a data race or false assertion trigger here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630083307.381909-1-honghuan@amd.com?part=4