[PATCH v2] drm/xe/i2c: cancel the client work on remove

[PATCH v2] drm/xe/i2c: cancel the client work on remove

[Fan Wu]

xe_i2c_notifier() stores the DesignWare adapter in i2c->adapter and
schedules i2c->work when the adapter is registered under the xe I2C
platform device, and xe_i2c_client_work() then instantiates the AMC
client device on that adapter.

xe_i2c_remove() tears down the AMC, unregisters the client devices,
the bus notifier and the adapter platform device, but it never drains
i2c->work. A work item that is still queued or running when the
adapter is unregistered dereferences i2c->adapter in
i2c_new_client_device() after platform_device_unregister() has
released the adapter. The work item is also embedded in the
devm-allocated struct xe_i2c, so a work item still queued after the
drm device devm unwind frees that allocation runs its callback on
freed memory.

Cancel the work in xe_i2c_remove(), after bus_unregister_notifier()
so that no new instance can be scheduled, and before the adapter is
unregistered so that a running instance still finds a live adapter.

This issue was found by an in-house static analysis tool.

Fixes: f0e53aadd702 ("drm/xe: Support for I2C attached MCUs")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---

Changes in v2:
- Rebase onto drm-xe-next as requested. xe_i2c_remove() now calls
  xe_amc_exit() first and no longer tears down an irqdomain. The bus
  notifier still needs to be unregistered before draining i2c->work,
  so keep cancel_work_sync() at that point. (Rodrigo Vivi)

Link: https://lore.kernel.org/r/20260909052328.654682-1-fanwu01@zju.edu.cn
 drivers/gpu/drm/xe/xe_i2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
index d8fa68206f41..143ca7145e4f 100644
--- a/drivers/gpu/drm/xe/xe_i2c.c
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -334,6 +334,7 @@ static void xe_i2c_remove(void *data)
 	}
 
 	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
+	cancel_work_sync(&i2c->work);
 	xe_i2c_unregister_adapter(i2c);
 }
 

Re: [PATCH v2] drm/xe/i2c: cancel the client work on remove

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Late work cancellation creates an unsynchronized data race and resource leak on the i2c->client array.

Pre-existing issues:
- [High] Dangling pointers in xe->i2c and i2c->amc cause a use-after-free when handling I2C SMBus alert interrupts due to devm teardown order.
--

commit 6a096e96b5e0c4286797ea9eb48856f09bbb9287
Author: Fan Wu <fanwu01@zju.edu.cn>
Subject: drm/xe/i2c: cancel the client work on remove

This commit aims to fix an issue in xe_i2c_remove() where i2c->work is not
drained before unregistering the adapter. It adds a call to cancel_work_sync()
after bus_unregister_notifier() to prevent a use-after-free if the work runs
after the adapter is unregistered.

Link: https://lore.kernel.org/r/20260909052328.654682-1-fanwu01@zju.edu.cn

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> index d8fa68206f418..143ca7145e4f4 100644
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
> @@ -334,6 +334,7 @@ static void xe_i2c_remove(void *data)
>  	}
>  
>  	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> +	cancel_work_sync(&i2c->work);

[Severity: High]
Does placing the work cancellation here introduce a data race with the client
unregistration loop?

Since cancel_work_sync() is executed after the loop that clears i2c->client
in xe_i2c_remove(), xe_i2c_client_work() can run concurrently on another CPU:

CPU1 (in xe_i2c_remove)                 CPU2 (in xe_i2c_client_work)
for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) {
    i2c_unregister_device(i2c->client[i]);
    i2c->client[i] = NULL;
}
                                        i2c->client[XE_I2C_CLIENT_AMC] =
                                            i2c_new_client_device(...);
cancel_work_sync(&i2c->work);

If the worker writes to the array after the loop has passed the index or
finished entirely, the newly created client device is leaked.

Could cancel_work_sync() and bus_unregister_notifier() be moved to the
beginning of the removal function to prevent this?


[Severity: High]
This isn't a bug introduced by this patch, but does this devres teardown path
leave dangling pointers that could cause a use-after-free during an SMBus
alert interrupt?

In xe_i2c_remove(), xe_amc_exit() is called before we reach this point, which
frees i2c->amc:

drivers/gpu/drm/xe/xe_amc.c:xe_amc_exit() {
    ...
    cancel_work_sync(&i2c->amc->work);
    kfree(i2c->amc);
}

Since i2c->amc is not set to NULL, and hardware interrupts are not disabled in
xe_i2c_remove(), a concurrent or subsequent interrupt can trigger the alert
handler:

drivers/gpu/drm/xe/xe_i2c.c:xe_i2c_handle_smbus_alert() {
    ...
    if (stat & DW_IC_SMBUS_INTR_ALERT && i2c->amc)
        xe_amc_handle_alert(i2c);
}

This calls into xe_amc_handle_alert() which dereferences the freed memory to
queue the work item:

drivers/gpu/drm/xe/xe_amc.c:xe_amc_handle_alert() {
    ...
    queue_work(system_long_wq, &i2c->amc->work);
}

Additionally, xe->i2c is not set to NULL before the devm subsystem frees the
i2c structure, so the active interrupt handler xe_i2c_irq_present() can also
dereference the dangling xe->i2c pointer.

Should hardware interrupts be disabled and these pointers cleared in this
removal function?

>  	xe_i2c_unregister_adapter(i2c);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912085932.101598-1-fanwu01@zju.edu.cn?part=1