Messages in this thread | | | Date | Wed, 27 Apr 2022 14:47:12 +0200 | Subject | Re: [PATCH net] drivers: net: can: Fix deadlock in grcan_close() | From | Andreas Larsson <> |
| |
On 2022-04-26 21:12, Oliver Hartkopp wrote: > On 25.04.22 06:24, Duoming Zhou wrote: >> There are deadlocks caused by del_timer_sync(&priv->hang_timer) >> and del_timer_sync(&priv->rr_timer) in grcan_close(), one of >> the deadlocks are shown below: >> >> (Thread 1) | (Thread 2) >> | grcan_reset_timer() >> grcan_close() | mod_timer() >> spin_lock_irqsave() //(1) | (wait a time) >> ... | grcan_initiate_running_reset() >> del_timer_sync() | spin_lock_irqsave() //(2) >> (wait timer to stop) | ... >> >> We hold priv->lock in position (1) of thread 1 and use >> del_timer_sync() to wait timer to stop, but timer handler >> also need priv->lock in position (2) of thread 2. >> As a result, grcan_close() will block forever. >> >> This patch extracts del_timer_sync() from the protection of >> spin_lock_irqsave(), which could let timer handler to obtain >> the needed lock. >> >> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> >> --- >> drivers/net/can/grcan.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c >> index d0c5a7a60da..1189057b5d6 100644 >> --- a/drivers/net/can/grcan.c >> +++ b/drivers/net/can/grcan.c >> @@ -1102,8 +1102,10 @@ static int grcan_close(struct net_device *dev) >> priv->closing = true; >> if (priv->need_txbug_workaround) { >> + spin_unlock_irqrestore(&priv->lock, flags); >> del_timer_sync(&priv->hang_timer); >> del_timer_sync(&priv->rr_timer); >> + spin_lock_irqsave(&priv->lock, flags); > > It looks weird to unlock and re-lock the operations like this. This > breaks the intended locking for the closing process. > > Isn't there any possibility to e.g. move that entire if-section before > the lock?
All functions wishing to start the timers both check priv->closing and then, if false, start the timer within the priv->lock spinlock. Given that, it should be ok that del_timer_sync is not done within the spinlock as therefore no one can restart any timers after priv->closing has been set to true.
It looks a bit weird, but setting priv->closing to true needs to happen within the priv->lock spinlock protection and needs to happen before del_timer_sync to avoid a race between grcan_close and someone starting the timer.
Reviewed-by: Andreas Larsson <andreas@gaisler.com>
-- Andreas Larsson
| |