Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Missing drm_pagemap teardown leads to module/drm_device reference leaks, and devm_kzalloc usage leads to use-after-free and kfree() panic. - [Medium] Resource leak in amdgpu_svm_migration_init error path. --
commit 3b8c30d59e9fe758a437cc04f508914b553acea9 Author: Junhua Shen <[email protected]> drm/amdgpu: add VRAM migration infrastructure for drm_pagemap This commit introduces the VRAM migration infrastructure for AMDGPU SVM using drm_pagemap. It adds the amdgpu_pagemap wrapper, defines the relevant owner macros, and implements amdgpu_svm_migration_init() to register ZONE_DEVICE pages and initialize the pagemap. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > new file mode 100644 > index 0000000000000..6b40f7477793a > --- /dev/null > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c [ ... ] > +const struct drm_pagemap_ops amdgpu_svm_drm_pagemap_ops = { }; [Severity: High] Does this missing .destroy callback cause issues during teardown? If drm_pagemap_destroy() falls back to kfree(dpagemap), will calling kfree() on memory allocated via devm_kzalloc() below trigger a kernel panic? [ ... ] > +int amdgpu_svm_migration_init(struct amdgpu_device *adev) > +{ > + struct amdgpu_pagemap *svm_dm; > + struct dev_pagemap *pgmap; > + struct resource *res = NULL; > + unsigned long size; > + void *r; [ ... ] > + svm_dm = devm_kzalloc(adev->dev, sizeof(*svm_dm), GFP_KERNEL); > + if (!svm_dm) > + return -ENOMEM; [ ... ] > + r = devm_memremap_pages(adev->dev, pgmap); > + if (IS_ERR(r)) { > + dev_err(adev->dev, "SVM: failed to register HMM device > memory\n"); > + if (pgmap->type == MEMORY_DEVICE_PRIVATE && res) > + devm_release_mem_region(adev->dev, res->start, > resource_size(res)); > + pgmap->type = 0; > + return PTR_ERR(r); > + } > + > + if (drm_pagemap_init(&svm_dm->dpagemap, pgmap, adev_to_drm(adev), > + > &amdgpu_svm_drm_pagemap_ops)) { [Severity: High] Can this initialization leak drm_device and module references? Since there isn't a teardown action registered (like devm_add_action) to call drm_pagemap_put() on unbind, it seems these references might never be released. Additionally, if the device unbinds while VRAM pages are active, devm will free svm_dm. Could this cause a use-after-free when active pages later access the freed dpagemap? > + dev_err(adev->dev, "SVM: failed to init drm_pagemap\n"); > + return -EINVAL; [Severity: Medium] If drm_pagemap_init() fails, does this error path permanently leak the ZONE_DEVICE pagemap structures and the IO region? Because the caller might continue device initialization without unbinding, the devm-managed resources from devm_request_free_mem_region() and devm_memremap_pages() seem to remain active indefinitely. Should this path manually clean up these resources before returning? > + } > + svm_dm->adev = adev; > + svm_dm->hpa_base = pgmap->range.start; > + svm_dm->initialized = true; > + adev->apagemap = svm_dm; > + > + dev_info(adev->dev, "SVM: registered %ldMB device memory, > hpa_base=0x%llx\n", > + size >> 20, svm_dm->hpa_base); > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
