Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Device memory mappings are leaked on the error path because `dpagemap` is not yet stored in `svm_pages`. - [High] Reading uninitialized memory (`svm_pages->dma_addr[0].dir`) when unwinding an IOVA allocation failure. - [High] System pages are incorrectly routed to `device_unmap` during unmapping of mixed ranges when IOVA is used. -- --- Patch [11]: [PATCH v2 11/33] drm/gpusvm: Add a DMA-mapping accounting callback --- commit 98db73f178129cc5ccc6277eaada197044dfbd3b Author: Thomas Hellström <[email protected]> drm/gpusvm: Add a DMA-mapping accounting callback This commit adds an optional @dma_map_account callback to struct drm_gpusvm_ops to track mapped pages. It is invoked during mapping and unmapping to keep symmetric accounting. It also relaxes drm_gpusvm_init() to allow restricted ops without full-SVM mode. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index fcfe635bc1956..512f52471419e 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c [ ... ] > @@ -1246,6 +1253,8 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm > *gpusvm, > else if (dpagemap && dpagemap->ops->device_unmap) > dpagemap->ops->device_unmap(dpagemap, > dev, addr); [Severity: High] This isn't a bug introduced by this patch, but can this leak device memory mappings on the error path? In drm_gpusvm_get_pages(), if an error occurs after successfully mapping one or more device pages, the code jumps to err_unmap and calls __drm_gpusvm_unmap_pages(). However, svm_pages->dpagemap is purposefully only updated after the mapping loop completes successfully. During the early error unwind, it remains NULL. Because of this, in __drm_gpusvm_unmap_pages(): struct drm_pagemap *dpagemap = svm_pages->dpagemap; The dpagemap evaluated here will be NULL, causing this else if condition to fail and completely bypassing the unmap callback for all successfully mapped device pages in that chunk. [Severity: High] This is also a pre-existing issue, but could this incorrectly unmap system pages as device pages in a mixed chunk? In drm_gpusvm_get_pages(), if a chunk has a mix of device and system pages (ctx->allow_mixed is true) and IOVA is used: When iterating over the pages in __drm_gpusvm_unmap_pages(), system pages will have addr->proto == DRM_INTERCONNECT_SYSTEM. However, if use_iova is true, the previous condition: if (!use_iova && addr->proto == DRM_INTERCONNECT_SYSTEM) dma_unmap_page(...) will evaluate to false. Execution then falls through to this else if branch. If a previous device page set dpagemap, it will inappropriately call dpagemap->ops->device_unmap() on the system page. > + if (gpusvm->ops && gpusvm->ops->dma_map_account) > + gpusvm->ops->dma_map_account(gpusvm, addr, -1); > i += 1 << addr->order; > } > [ ... ] > @@ -1654,6 +1663,10 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > (addr, DRM_INTERCONNECT_SYSTEM, order, > dma_dir); > } > + if (gpusvm->ops && gpusvm->ops->dma_map_account) > + gpusvm->ops->dma_map_account(gpusvm, > + &svm_pages->dma_addr[j], > + 1); [Severity: High] This is a pre-existing issue, but is it possible to read uninitialized memory here if the first page mapping fails? In drm_gpusvm_get_pages(), svm_pages->dma_addr is allocated via kvmalloc_objs() without zero-initialization. If an error occurs on the very first page (i = 0), such as an IOVA allocation failure, the code jumps to err_unmap with num_dma_mapped = 0, and __drm_gpusvm_unmap_pages() is called with npages = 0. Inside __drm_gpusvm_unmap_pages(), if dma_use_iova(&svm_pages->state) is true, it will unconditionally call: dma_iova_destroy(dev, &svm_pages->state, svm_pages->state_offset, svm_pages->dma_addr[0].dir, 0); Since npages = 0, svm_pages->dma_addr[0] was never initialized, leading to svm_pages->dma_addr[0].dir being read from uninitialized heap memory. > i += 1 << order; > num_dma_mapped = i; > flags.has_dma_mapping = true; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
