Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Calling cancel_work_sync() on the potentially uninitialized extcon_wq triggers a WARN_ON(!work->func) kernel splat. Pre-existing issues: - [High] Canceling work items without first disabling their scheduling sources (IRQ and extcon notifier) leaves a race window where works can be requeued, leading to a use-after-free. -- --- Patch [1]: [PATCH v3 1/4] drm/bridge: it6505: cancel outstanding work before teardown in remove() --- commit 06b35e4209da852ce0af8e4b1dd2aec2e9e172b4 Author: Daniel Golle <[email protected]> drm/bridge: it6505: cancel outstanding work before teardown in remove() This commit adds calls to cancel_work_sync() and cancel_delayed_work_sync() in it6505_i2c_remove() to ensure background works do not dereference freed memory after the device is unbound. It notes that extcon_wq might not be initialized, but claims a zero-allocated work_struct is safe to cancel. > cancel_work_sync() on extcon_wq is safe even > when it was never INIT_WORK()'d, since it6505 is zero-allocated and a > zeroed work_struct is reported as not pending. [Severity: Medium] Is this description accurate? Looking at __flush_work() in kernel/workqueue.c, the workqueue core asserts WARN_ON(!work->func) when flushing a work item. If the DRM bridge was never attached, extcon_wq remains zero-allocated. During module removal, calling cancel_work_sync(&it6505->extcon_wq) will unconditionally call __flush_work(work, true), which immediately triggers the WARN_ON because the work function is NULL. On systems with panic_on_warn, this leads to a kernel panic. > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 8ecb43611dba8..2f349aefb7054 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -3644,6 +3644,10 @@ static void it6505_i2c_remove(struct i2c_client > *client) > { > struct it6505 *it6505 = i2c_get_clientdata(client); > > + cancel_work_sync(&it6505->link_works); > + cancel_work_sync(&it6505->hdcp_wait_ksv_list); > + cancel_delayed_work_sync(&it6505->hdcp_work); > + cancel_work_sync(&it6505->extcon_wq); [Severity: High] This is a pre-existing issue, but does this leave a race window where these work items could be requeued? The driver uses devres for the IRQ and extcon notifier, which remain active during these cancel_work_sync() calls. Furthermore, the hardware interrupt is disabled after the works are cancelled. If we look further down in it6505_i2c_remove(): ... drm_bridge_remove(&it6505->bridge); drm_dp_aux_unregister(&it6505->aux); it6505_debugfs_remove(it6505); it6505_poweroff(it6505); ... The interrupt is only disabled inside it6505_poweroff(it6505). If an interrupt or extcon event fires immediately after the work is cancelled but before it6505_poweroff() and devres cleanup, won't the handler run and re-queue the work via schedule_work()? This could lead to a use-after-free when devres frees the it6505 structure while the re-queued work is still pending. > drm_bridge_remove(&it6505->bridge); > drm_dp_aux_unregister(&it6505->aux); > it6505_debugfs_remove(it6505); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
