Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] The commit message falsely claims that gr2d_probe() calls pm_runtime_use_autosuspend(), and adds a redundant cleanup to gr2d_remove() based on this false premise. Pre-existing issues: - [High] Unbinding the gr2d driver while a DRM channel is open causes a Use-After-Free when the DRM file is later closed by userspace. -- --- Patch [1]: [PATCH] drm/tegra: gr2d: fix autosuspend cleanup during teardown --- commit 9b1fea12d4967b1c2b4a943c3958df804b6cd6a8 Author: Guangshuo Li <[email protected]> drm/tegra: gr2d: fix autosuspend cleanup during teardown This commit adds a pm_runtime_dont_use_autosuspend() call to the driver's remove function. It attempts to fix a leaked usage count reference that the commit message states was taken in gr2d_probe(). > gr2d_probe() calls pm_runtime_use_autosuspend(), but gr2d_remove() > does not call the matching pm_runtime_dont_use_autosuspend() during > teardown. [Severity: Medium] Is this description accurate? Looking at the code, gr2d_probe() does not call pm_runtime_use_autosuspend(). Furthermore, pm_runtime_dont_use_autosuspend() is already properly called during teardown inside gr2d_exit(), which is reached via host1x_client_unregister(). Does this patch add a redundant cleanup based on an incorrect premise? > diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c > index 21f4dd0fa6aff..8063e06b0b87a 100644 > --- a/drivers/gpu/drm/tegra/gr2d.c > +++ b/drivers/gpu/drm/tegra/gr2d.c > @@ -293,6 +293,7 @@ static void gr2d_remove(struct platform_device *pdev) > { > struct gr2d *gr2d = platform_get_drvdata(pdev); > > + pm_runtime_dont_use_autosuspend(&pdev->dev); > pm_runtime_disable(&pdev->dev); > host1x_client_unregister(&gr2d->client.base); > } [Severity: High] This is a pre-existing issue, but does unbinding the driver here cause a use-after-free if a DRM channel remains open? If an administrator unbinds the tegra-gr2d driver while userspace has a DRM channel open, gr2d_remove() will execute. The struct gr2d driver data will then be freed by devres. When userspace subsequently closes the DRM file descriptor, tegra_drm_postclose() calls tegra_drm_context_free(), which dereferences the freed client ops: drivers/gpu/drm/tegra/drm.c:tegra_drm_context_free() { ... context->client->ops->close_channel(context); ... } Should the DRM layer be coordinated with to revoke active contexts before the device structure is destroyed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
