| Message ID | 20221123201625.435907114@linutronix.de |
|---|---|
| State | Accepted |
| Commit | e0d3da982c96aeddc1bbf1cf9469dbb9ebdca657 |
| Headers | show |
| Series | timers: Provide timer_shutdown[_sync]() | expand |
On Wed, 23 Nov 2022, Thomas Gleixner wrote: > While discussing solutions for the teardown problem which results from > circular dependencies between timers and workqueues, where timers schedule > work from their timer callback and workqueues arm the timers from work > items, it was discovered that the recent fix to the QCA code is incorrect. > > That commit fixes the obvious problem of using del_timer() instead of > del_timer_sync() and reorders the teardown calls to > > destroy_workqueue(wq); > del_timer_sync(t); > > This makes it less likely to explode, but it's still broken: > > destroy_workqueue(wq); > /* After this point @wq cannot be touched anymore */ > > ---> timer expires > queue_work(wq) <---- Results in a NULl pointer dereference The last NIT (for now...): s/NULl/NULL Thanks, Anna-Maria
--- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -696,9 +696,15 @@ static int qca_close(struct hci_uart *hu skb_queue_purge(&qca->tx_wait_q); skb_queue_purge(&qca->txq); skb_queue_purge(&qca->rx_memdump_q); + /* + * Shut the timers down so they can't be rearmed when + * destroy_workqueue() drains pending work which in turn might try + * to arm a timer. After shutdown rearm attempts are silently + * ignored by the timer core code. + */ + timer_shutdown_sync(&qca->tx_idle_timer); + timer_shutdown_sync(&qca->wake_retrans_timer); destroy_workqueue(qca->workqueue); - del_timer_sync(&qca->tx_idle_timer); - del_timer_sync(&qca->wake_retrans_timer); qca->hu = NULL; kfree_skb(qca->rx_skb);
While discussing solutions for the teardown problem which results from circular dependencies between timers and workqueues, where timers schedule work from their timer callback and workqueues arm the timers from work items, it was discovered that the recent fix to the QCA code is incorrect. That commit fixes the obvious problem of using del_timer() instead of del_timer_sync() and reorders the teardown calls to destroy_workqueue(wq); del_timer_sync(t); This makes it less likely to explode, but it's still broken: destroy_workqueue(wq); /* After this point @wq cannot be touched anymore */ ---> timer expires queue_work(wq) <---- Results in a NULl pointer dereference deep in the work queue core code. del_timer_sync(t); Use the new timer_shutdown_sync() function to ensure that the timers are disarmed, no timer callbacks are running and the timers cannot be armed again. This restores the original teardown sequence: timer_shutdown_sync(t); destroy_workqueue(wq); which is now correct because the timer core silently ignores potential rearming attempts which can happen when destroy_workqueue() drains pending work before mopping up the workqueue. Fixes: 72ef98445aca ("Bluetooth: hci_qca: Use del_timer_sync() before freeing") Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Tested-by: Guenter Roeck <linux@roeck-us.net> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Acked-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Cc: Marcel Holtmann <marcel@holtmann.org> Cc: Johan Hedberg <johan.hedberg@gmail.com> Cc: linux-bluetooth@vger.kernel.org Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: netdev@vger.kernel.org Link: https://lore.kernel.org/all/87iljhsftt.ffs@tglx --- drivers/bluetooth/hci_qca.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)