Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Reading uninitialized memory `svm_pages->dma_addr[0].dir` in the error path of `__drm_gpusvm_unmap_pages()` when IOVA allocation fails. - [High] System memory pages are incorrectly routed to `device_unmap()` during unmap of mixed ranges when `use_iova` is true. -- commit 755aa251270882d2dbbc0da1e00b00a5aafa5b38 Author: Honglei Huang <[email protected]> drm/gpusvm: embed struct drm_device into drm_gpusvm_pages Since now the drm_gpusvm_pages instance is inherently bound to one specific drm_device, make that ownership explicit by giving drm_gpusvm_pages its own drm_device handle, and drive all DMA through it instead of through the gpusvm. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index df900553f21a4..155e3c2e703d9 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c > @@ -1135,11 +1136,16 @@ static void __drm_gpusvm_unmap_pages(struct > drm_gpusvm *gpusvm, > unsigned long npages) > { > struct drm_pagemap *dpagemap = svm_pages->dpagemap; > - struct device *dev = gpusvm->drm->dev; > + struct device *dev; > unsigned long i, j; > > lockdep_assert_held(&gpusvm->notifier_lock); > > + if (!svm_pages->drm) > + return; > + > + dev = svm_pages->drm->dev; > + [Severity: High] This is a pre-existing issue, but does __drm_gpusvm_unmap_pages() incorrectly route system memory pages to device_unmap() when use_iova is true? In a mixed range where ctx->allow_mixed is true, containing both system and device pages, dpagemap is non-NULL. During unmap, the loop checks: 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, addr->dir); } If use_iova is true, the first condition evaluates to false for system pages, causing execution to fall through to the else if branch. This passes system DMA addresses to the device-specific unmap callback, which could lead to invalid memory accesses or state corruption. [ ... ] > @@ -1555,11 +1564,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > } > > if (!i) > - dma_iova_try_alloc(gpusvm->drm->dev, state, > + dma_iova_try_alloc(svm_pages->drm->dev, state, > 0, npages * PAGE_SIZE); > > if (dma_use_iova(state)) { > - err = dma_iova_link(gpusvm->drm->dev, state, > + err = dma_iova_link(svm_pages->drm->dev, state, > hmm_pfn_to_phys(pfns[i]), > svm_pages->state_offset, > PAGE_SIZE << order, [Severity: High] This is a pre-existing issue, but does this error path read uninitialized memory if dma_iova_link() fails on the first iteration where i == 0? Earlier in drm_gpusvm_get_pages(), svm_pages->dma_addr is allocated using kvmalloc_objs() which does not zero-initialize the memory. If the link fails on the first iteration, the code jumps to err_unmap before svm_pages->dma_addr[0] is initialized. The err_unmap path calls __drm_gpusvm_unmap_pages() with num_dma_mapped set to 0. Because dma_use_iova() returns true, __drm_gpusvm_unmap_pages() unconditionally calls dma_iova_destroy() and evaluates svm_pages->dma_addr[0].dir to pass as the direction argument: if (use_iova) dma_iova_destroy(dev, &svm_pages->state, svm_pages->dma_addr[0].dir, 0); This evaluates uninitialized memory, which will trigger KMSAN warnings and pass a garbage DMA direction to the DMA subsystem. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
