Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] xe_device_assert_dma_pages_zero() is called too early in xe_device_destroy(), leading to spurious kernel assertion failures. - [High] Uncapped `addr->order` in SVM and userptr DMA accounting leads to out-of-bounds array writes. - [Low] Too many arguments to `drm_gpusvm_init` in `xe_svm.h` causes a compilation error when `CONFIG_DRM_GPUSVM` is enabled but `CONFIG_DRM_XE_GPUSVM` is disabled. Pre-existing issues: - [Critical] Asynchronous execution of `vm_destroy_work_func` can cause a Use-After-Free of the `xe` device structure. -- --- Patch [12]: [PATCH v2 12/33] drm/xe: Add debugfs stats for DMA-mapped pages per order --- commit 808c14d37a18acfee1cc7559a4568116c9d63f8b Author: Thomas Hellström <[email protected]> Subject: drm/xe: Add debugfs stats for DMA-mapped pages per order This commit adds per-page-order DMA mapping counters for system memory accessible by the GPU. The stats cover both TTM buffer objects and SVM/userptr ranges, exposing them through debugfs. > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > index ad7f3e61d4579..0c4ee874a56d7 100644 > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c [ ... ] > @@ -475,8 +499,10 @@ static void xe_device_destroy(struct drm_device *dev, > void *dummy) > { > struct xe_device *xe = to_xe_device(dev); > > + xe_device_assert_dma_pages_zero(xe); > + > xe_bo_dev_fini(&xe->bo_device); [Severity: High] Does this assertion trigger prematurely if the asynchronous BO destruction worker has pending buffers to free? In xe_device_destroy(), xe_device_assert_dma_pages_zero() is called before xe_bo_dev_fini() flushes the async_free worker. If there are delayed destructions, wouldn't the DMA mappings still be active, causing the counters to be non-zero and triggering a kernel warning? > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c > index b228a737cfd6d..2f639fc0cac4e 100644 > --- a/drivers/gpu/drm/xe/xe_svm.c > +++ b/drivers/gpu/drm/xe/xe_svm.c [ ... ] > +static void xe_svm_dma_map_account(struct drm_gpusvm *gpusvm, > + const struct drm_pagemap_addr *addr, > + int sign) > +{ > + struct xe_device *xe = gpusvm_to_vm(gpusvm)->xe; > + unsigned int order = addr->order; > + > + if (addr->proto != DRM_INTERCONNECT_SYSTEM) > + return; > + > + atomic_long_add((long)(1UL << order) * sign, > + &xe->mem.dma_mapped_pages_svm[order]); [Severity: High] Can this cause an out-of-bounds write if addr->order exceeds the array bounds of dma_mapped_pages_svm? In xe_svm_dma_map_account(), addr->order is used directly as an array index. If an unprivileged user maps a large huge page (such as a 1GB page with an order of 18) and faults it into the GPU, wouldn't this exceed the NR_PAGE_ORDERS limit and corrupt adjacent kernel memory? > +} > +#endif > diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h > index a921556d34662..edd668c415512 100644 > --- a/drivers/gpu/drm/xe/xe_svm.h > +++ b/drivers/gpu/drm/xe/xe_svm.h [ ... ] > @@ -236,7 +236,8 @@ int xe_svm_init(struct xe_vm *vm) > { > #if IS_ENABLED(CONFIG_DRM_GPUSVM) > return drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)", > - NULL, 0, 0, 0, NULL, NULL, 0); > + NULL, 0, 0, 0, NULL, NULL, > + xe_userptr_gpusvm_ops_get(), 0); [Severity: Low] Does this function call pass too many arguments to drm_gpusvm_init()? In the xe_svm_init() inline stub, the xe_userptr_gpusvm_ops_get() argument was added without removing one of the NULL placeholders. Since drm_gpusvm_init() expects 9 arguments and this now provides 10, won't this cause a compilation error when CONFIG_DRM_GPUSVM is enabled but CONFIG_DRM_XE_GPUSVM is disabled? > #else > return 0; > #endif > diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c > index 8b2d461ea0b28..79c2a81001f2e 100644 > --- a/drivers/gpu/drm/xe/xe_userptr.c > +++ b/drivers/gpu/drm/xe/xe_userptr.c [ ... ] > +static void xe_userptr_dma_map_account(struct drm_gpusvm *gpusvm, > + const struct drm_pagemap_addr *addr, > + int sign) > +{ > + struct xe_vm *vm = container_of(gpusvm, struct xe_vm, svm.gpusvm); > + unsigned int order = addr->order; > + > + if (addr->proto != DRM_INTERCONNECT_SYSTEM) > + return; > + > + atomic_long_add((long)(1UL << order) * sign, > + &vm->xe->mem.dma_mapped_pages_svm[order]); [Severity: High] Similarly to xe_svm_dma_map_account(), can this cause an out-of-bounds write if addr->order is larger than NR_PAGE_ORDERS? Using addr->order directly as an array index without capping it could allow unprivileged user-space applications mapping large huge pages to corrupt memory when invoking xe_userptr_dma_map_account(). > +} While reviewing the VM lifecycle code related to this patch, I noticed a separate pre-existing issue in drivers/gpu/drm/xe/xe_vm.c. [Severity: Critical] This is a pre-existing issue, but does queueing the destroy work asynchronously lead to a use-after-free if the last reference to the DRM device is dropped before the worker executes? In xe_vm_free(), the destruction work is queued to system_dfl_wq: queue_work(system_dfl_wq, &vm->destroy_work); If this drops the last device reference, the xe device could be freed via RCU before vm_destroy_work_func() runs. Since vm_destroy_work_func() dereferences the xe pointer (for example, when calling xe_pm_runtime_put()), wouldn't this result in a use-after-free? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
