| Message ID | 20220518105321.32746-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b8cedb7093b2d1394cae9b86494cba4b62d3a30a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v0] nfc: pn533: Fix buggy cleanup order | expand |
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Wed, 18 May 2022 18:53:21 +0800 you wrote: > When removing the pn533 device (i2c or USB), there is a logic error. The > original code first cancels the worker (flush_delayed_work) and then > destroys the workqueue (destroy_workqueue), leaving the timer the last > one to be deleted (del_timer). This result in a possible race condition > in a multi-core preempt-able kernel. That is, if the cleanup > (pn53x_common_clean) is concurrently run with the timer handler > (pn533_listen_mode_timer), the timer can queue the poll_work to the > already destroyed workqueue, causing use-after-free. > > [...] Here is the summary with links: - [v0] nfc: pn533: Fix buggy cleanup order https://git.kernel.org/netdev/net/c/b8cedb7093b2 You are awesome, thank you!
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c index a491db46e3bd..a0532647b040 100644 --- a/drivers/nfc/pn533/pn533.c +++ b/drivers/nfc/pn533/pn533.c @@ -2787,13 +2787,14 @@ void pn53x_common_clean(struct pn533 *priv) { struct pn533_cmd *cmd, *n; + /* delete the timer before cleanup the worker */ + del_timer_sync(&priv->listen_timer); + flush_delayed_work(&priv->poll_work); destroy_workqueue(priv->wq); skb_queue_purge(&priv->resp_q); - del_timer(&priv->listen_timer); - list_for_each_entry_safe(cmd, n, &priv->cmd_queue, queue) { list_del(&cmd->queue); kfree(cmd);
When removing the pn533 device (i2c or USB), there is a logic error. The original code first cancels the worker (flush_delayed_work) and then destroys the workqueue (destroy_workqueue), leaving the timer the last one to be deleted (del_timer). This result in a possible race condition in a multi-core preempt-able kernel. That is, if the cleanup (pn53x_common_clean) is concurrently run with the timer handler (pn533_listen_mode_timer), the timer can queue the poll_work to the already destroyed workqueue, causing use-after-free. This patch reorder the cleanup: it uses the del_timer_sync to make sure the handler is finished before the routine will destroy the workqueue. Note that the timer cannot be activated by the worker again. static void pn533_wq_poll(struct work_struct *work) ... rc = pn533_send_poll_frame(dev); if (rc) return; if (cur_mod->len == 0 && dev->poll_mod_count > 1) mod_timer(&dev->listen_timer, ...); That is, the mod_timer can be called only when pn533_send_poll_frame() returns no error, which is impossible because the device is detaching and the lower driver should return ENODEV code. Signed-off-by: Lin Ma <linma@zju.edu.cn> --- drivers/nfc/pn533/pn533.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)