Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Device memory DMA mappings are leaked if an error occurs during the mapping loop in `drm_gpusvm_get_pages()`. - [High] An uninitialized memory read occurs in `__drm_gpusvm_unmap_pages()` if IOVA allocation succeeds but linking fails on the very first page. -- commit 4fe6a3e14e9c6c05c83b0d432b653dc56d06006d Author: Honglei Huang <[email protected]> drm/gpusvm: embed struct drm_device into drm_gpusvm_pages Embeds the drm_device into drm_gpusvm_pages so that the pages object explicitly owns the drm handle. It routes DMA operations through svm_pages->drm instead of gpusvm->drm. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index abdfdaaf5e23c..604554e40f62d 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c [ ... ] > @@ -1532,10 +1541,10 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > } > svm_pages->dma_addr[j] = > dpagemap->ops->device_map(dpagemap, > - gpusvm->drm->dev, > + svm_pages->drm->dev, > page, order, > dma_dir); > - if (dma_mapping_error(gpusvm->drm->dev, > + if (dma_mapping_error(svm_pages->drm->dev, > svm_pages->dma_addr[j].addr)) { > err = -EFAULT; > goto err_unmap; [Severity: High] This isn't a bug introduced by this patch, but does this leak device memory DMA mappings if an error occurs here? If dma_mapping_error() fails (or if a later operation fails and jumps to err_unmap), __drm_gpusvm_unmap_pages() is called. However, svm_pages->dpagemap is not updated until the very end of drm_gpusvm_get_pages() on the success path. Because svm_pages->dpagemap is still NULL in the error path, __drm_gpusvm_unmap_pages() evaluates the condition else if (dpagemap && dpagemap->ops->device_unmap) as false and silently skips cleanup for all successfully mapped device memory pages. > @@ -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 can this lead to an uninitialized memory read if linking fails on the very first page? If dma_iova_try_alloc() succeeds but dma_iova_link() fails on the first iteration (i == 0), the code jumps to err_unmap with num_dma_mapped = 0 before initializing svm_pages->dma_addr[0]. The error path then calls __drm_gpusvm_unmap_pages(..., 0). Because use_iova is true, it unconditionally evaluates svm_pages->dma_addr[0].dir to call dma_iova_destroy(), reading from the uninitialized memory of the dma_addr array (which was allocated via kvmalloc_objs without zeroing). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
