Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Uninitialized memory read of `svm_pages->dma_addr[0].dir` passed to `dma_iova_destroy()` on the error path. - [High] System memory pages are erroneously unmapped using the device-specific unmap callback in mixed SVM ranges. - [High] Device memory DMA mappings are leaked on the error path because `svm_pages->dpagemap` is assigned too late. -- --- Patch [11]: [PATCH v3 11/33] drm/gpusvm: Add a DMA-mapping accounting callback --- commit c12d7531ab5aab8f1172b95a5e7473303dec85e1 Author: Thomas Hellström <[email protected]> drm/gpusvm: Add a DMA-mapping accounting callback The patch adds a dma_map_account callback to drm_gpusvm_ops to allow drivers to symmetrically account for DMA-mapped pages during map and unmap operations. > 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 does the error path in drm_gpusvm_get_pages() trigger an uninitialized memory read here? If the first page's mapping fails (e.g., dma_iova_link() fails), the code jumps to the err_unmap label with num_dma_mapped = 0. Because dma_iova_try_alloc() succeeded, use_iova evaluates to true in __drm_gpusvm_unmap_pages(). This unconditionally executes dma_iova_destroy() just before this loop, reading svm_pages->dma_addr[0].dir by value: if (use_iova) dma_iova_destroy(dev, &svm_pages->state, svm_pages->state_offset, svm_pages->dma_addr[0].dir, 0); Since svm_pages->dma_addr was allocated using kvmalloc_objs without zero-initialization, this passes an uninitialized enum dma_data_direction into a kernel DMA API, which can trigger KMSAN panics or BUG_ON checks in DMA debugging routines. [Severity: High] This is a pre-existing issue, but does this logic misroute system memory pages to the device unmap callback? When ctx->allow_mixed is true, an SVM range can contain both system and device pages. In this scenario, use_iova evaluates to true, and svm_pages->dpagemap is initialized to the device's pagemap. During __drm_gpusvm_unmap_pages(), the loop iterates over all pages. For a system page, addr->proto == DRM_INTERCONNECT_SYSTEM is true, but !use_iova evaluates to false. This causes the execution to fall through to the else if condition without validating the page type: else if (dpagemap && dpagemap->ops->device_unmap) dpagemap->ops->device_unmap(dpagemap, dev, addr); This incorrectly executes the device's unmap callback on a system memory page (containing an IOVA address), which seems likely to cause memory corruption or a crash. [Severity: High] This is also a pre-existing issue, but are device memory DMA mappings leaked on the error path because svm_pages->dpagemap is assigned too late? If an error occurs during drm_gpusvm_get_pages() after successfully mapping one or more device private pages, the code jumps to the err_unmap cleanup path. However, svm_pages->dpagemap is only updated at the very end of the success path: if (pagemap) { flags.has_devmem_pages = true; drm_pagemap_get(dpagemap); drm_pagemap_put(svm_pages->dpagemap); svm_pages->dpagemap = dpagemap; } Consequently, inside __drm_gpusvm_unmap_pages(), the local dpagemap variable is NULL, causing the condition dpagemap && dpagemap->ops->device_unmap to evaluate to false. The device unmap callback is skipped, leaking the device memory mappings. The newly added dma_map_account callback will un-account these leaked pages, causing internal accounting to irreversibly drift from the hardware state. > + if (gpusvm->ops && gpusvm->ops->dma_map_account) > + gpusvm->ops->dma_map_account(gpusvm, addr, -1); > i += 1 << addr->order; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
