Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Device mappings are permanently leaked on the error path because `svm_pages->dpagemap` is assigned too late. -- commit 69970ff2a9a2262bbdbdbce9ebee1a8d4c896b22 Author: Honglei Huang <[email protected]> drm/gpusvm: move dma_addr allocation before the notifier lock The dma_addr allocation was in a lazy allocation flow, it needs unlock and goto map_pages. The allocation only needs npages, so just do it before taking the lock. Drop the map_pages label and the relock flow, so the sequence becomes fault, allocate, then lock, validate, map and unlock. No functional change intended. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index 507ef6f0a60e7..3496cb5688365 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c [ ... ] > @@ -1575,18 +1583,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > goto retry; > } > > - if (!svm_pages->dma_addr) { > - /* Unlock and restart mapping to allocate memory. */ > - drm_gpusvm_notifier_unlock(gpusvm); > - svm_pages->dma_addr = > - kvmalloc_objs(*svm_pages->dma_addr, npages); > - if (!svm_pages->dma_addr) { > - err = -ENOMEM; > - goto err_free; > - } > - goto map_pages; > - } > - > zdd = NULL; > pagemap = NULL; > num_dma_mapped = 0; [Severity: High] This is a pre-existing issue, but does the error path here permanently leak device mappings? If dpagemap->ops->device_map() succeeds for some pages in drm_gpusvm_get_pages(), but a subsequent mapping operation (like dma_map_page() or dma_iova_link()) fails, the code jumps to err_unmap: err_unmap: svm_pages->flags.has_dma_mapping = true; __drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped); Inside __drm_gpusvm_unmap_pages(), it reads: struct drm_pagemap *dpagemap = svm_pages->dpagemap; And later checks it before unmapping: else if (dpagemap && dpagemap->ops->device_unmap) dpagemap->ops->device_unmap(dpagemap, dev, addr); However, back in drm_gpusvm_get_pages(), svm_pages->dpagemap is only assigned 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; } Does this mean that during the error path, svm_pages->dpagemap is NULL, causing __drm_gpusvm_unmap_pages() to silently skip calling dpagemap->ops->device_unmap() and permanently leak the mappings? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
