Re: [PATCH v3 1/4] drm/bridge: it6505: cancel outstanding work before teardown in remove()

Hi Daniel,

On Tue, Jul 21, 2026 at 10:41 AM <[email protected]> wrote:
>
> 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.

Sashiko seems to be correct here. This is going to cause a very loud
warning, which we likely don't want.

ChenYu

> > 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