Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] Canceling `job_done_work` without explicitly draining `job_done_list` directly leaks completed jobs and resources. Pre-existing issues: - [High] Missing drm_dev_enter() in DRM ioctls allows concurrent V3D unbind to directly cause a NULL pointer dereference. - [High] Spurious interrupt execution directly after `vc4->v3d` clearance causes a NULL pointer crash. -- commit c625fc5a6f0d8fd20c510d6e8829315b01c15ab3 Author: Fan Wu <[email protected]> drm/vc4: drain the hangcheck timer and works on V3D unbind The hangcheck timer, which every submitted job arms and which queues reset_work once a job stops making progress, and the job_done_work, which the render-done interrupt queues to release completed jobs, are never drained at teardown. This commit shuts the hangcheck timer down and cancels reset_work and job_done_work during vc4_v3d_unbind(). > diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c > index f32410420d3e4..11d4df3164b49 100644 > --- a/drivers/gpu/drm/vc4/vc4_v3d.c > +++ b/drivers/gpu/drm/vc4/vc4_v3d.c [ ... ] > @@ -497,8 +498,18 @@ static void vc4_v3d_unbind(struct device *dev, struct > device *master, > struct drm_device *drm = data; > struct vc4_dev *vc4 = to_vc4_dev(drm); > > + /* Kill the timer, then reset_work: vc4_irq_reset() re-enables it. */ > + if (vc4->gen == VC4_GEN_4) { > + timer_shutdown_sync(&vc4->hangcheck.timer); > + cancel_work_sync(&vc4->hangcheck.reset_work); > + } > + > vc4_irq_uninstall(drm); [Severity: High] This is a pre-existing issue, but could this lead to a spurious interrupt execution causing a NULL pointer crash? When vc4_v3d_unbind() executes, it disables the hardware interrupts and later sets vc4->v3d = NULL, but it does not unregister the IRQ handler (since it is devm-managed by the parent device). If a spurious interrupt arrives after this point, would vc4_irq() execute and unconditionally dereference vc4->v3d via the V3D_READ macro, causing an immediate crash? > > + /* Nothing can queue job_done_work any more; drain it. */ > + if (vc4->gen == VC4_GEN_4) > + cancel_work_sync(&vc4->job_done_work); [Severity: Medium] Will this cancel_work_sync() directly leak completed jobs and resources? If there are completed jobs pending in the job_done_list when the device is unbound, the work is canceled before it can process the list. Since no subsequent cleanup code (e.g., vc4_gem_destroy()) iterates over and frees the items in job_done_list, won't these jobs and their associated BOs be permanently leaked? > + > /* Disable the binner's overflow memory address, so the next > * driver probe (if any) doesn't try to reuse our old > * allocation. [Severity: High] This isn't a bug introduced by this patch, but does the ioctl path lack drm_dev_enter() protection, allowing concurrent V3D unbind to directly cause a NULL pointer dereference? A user-space application could concurrently call DRM_IOCTL_VC4_SUBMIT_CL while the administrator unbinds the v3d component via sysfs. The ioctl checks if (!vc4->v3d) without holding a reference (e.g., drm_dev_enter()): drivers/gpu/drm/vc4/vc4_gem.c:vc4_submit_cl_ioctl() { if (!vc4->v3d) { DRM_DEBUG("VC4_SUBMIT_CL with no VC4 V3D probed\n"); return -ENODEV; } ... } Immediately after this check, vc4_v3d_unbind() executes and sets vc4->v3d = NULL. Would the ioctl then proceed to access vc4->v3d (e.g., in vc4_v3d_pm_get()), resulting in a direct NULL pointer dereference? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
