Messages in this thread | | | Date | Tue, 26 Apr 2022 21:12:48 +0200 | Subject | Re: [PATCH net] drivers: net: can: Fix deadlock in grcan_close() | From | Oliver Hartkopp <> |
| |
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?
> } > netif_stop_queue(dev); > grcan_stop_hardware(dev);
Regards, Oliver
| |