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 <[email protected]> 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/[email protected] > 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/[email protected]?part=1
