| Message ID | e82ff9358d4ef90a7e9f624534d6d54fc193467f.1649310812.git.duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix deadlocks caused by del_timer_sync() | expand |
On 07. 04. 22, 8:33, Duoming Zhou wrote: > There is a deadlock in sa1100_set_termios(), which is shown > below: > > (Thread 1) | (Thread 2) > | sa1100_enable_ms() > sa1100_set_termios() | mod_timer() > spin_lock_irqsave() //(1) | (wait a time) > ... | sa1100_timeout() > del_timer_sync() | spin_lock_irqsave() //(2) > (wait timer to stop) | ... > > We hold sport->port.lock in position (1) of thread 1 and > use del_timer_sync() to wait timer to stop, but timer handler > also need sport->port.lock in position (2) of thread 2. As a result, > sa1100_set_termios() 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/tty/serial/sa1100.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c > index 5fe6cccfc1a..3a5f12ced0b 100644 > --- a/drivers/tty/serial/sa1100.c > +++ b/drivers/tty/serial/sa1100.c > @@ -476,7 +476,9 @@ sa1100_set_termios(struct uart_port *port, struct ktermios *termios, > UTSR1_TO_SM(UTSR1_ROR); > } > > + spin_unlock_irqrestore(&sport->port.lock, flags); Unlocking the lock at this point doesn't look safe at all. Maybe moving the timer deletion before the lock? There is no current maintainer to ask. Most of the driver originates from rmk. Ccing him just in case. FWIW the lock was moved by this commit around linux 2.5.55 (from full-history-linux [1]) commit f38aef3e62c26a33ea360a86fde9b27e183a3748 Author: Russell King <rmk@flint.arm.linux.org.uk> Date: Fri Jan 3 15:42:09 2003 +0000 [SERIAL] Convert change_speed() to settermios() [1] https://archive.org/download/git-history-of-linux/full-history-linux.git.tar > del_timer_sync(&sport->timer); > + spin_lock_irqsave(&sport->port.lock, flags); > > /* > * Update the per-port timeout. thanks,
Hello, On Thu, 7 Apr 2022 09:02:05 +0200 Jiri Slaby wrote: > > There is a deadlock in sa1100_set_termios(), which is shown > > below: > > > > (Thread 1) | (Thread 2) > > | sa1100_enable_ms() > > sa1100_set_termios() | mod_timer() > > spin_lock_irqsave() //(1) | (wait a time) > > ... | sa1100_timeout() > > del_timer_sync() | spin_lock_irqsave() //(2) > > (wait timer to stop) | ... > > > > We hold sport->port.lock in position (1) of thread 1 and > > use del_timer_sync() to wait timer to stop, but timer handler > > also need sport->port.lock in position (2) of thread 2. As a result, > > sa1100_set_termios() 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/tty/serial/sa1100.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c > > index 5fe6cccfc1a..3a5f12ced0b 100644 > > --- a/drivers/tty/serial/sa1100.c > > +++ b/drivers/tty/serial/sa1100.c > > @@ -476,7 +476,9 @@ sa1100_set_termios(struct uart_port *port, struct ktermios *termios, > > UTSR1_TO_SM(UTSR1_ROR); > > } > > > > + spin_unlock_irqrestore(&sport->port.lock, flags); > > Unlocking the lock at this point doesn't look safe at all. Maybe moving > the timer deletion before the lock? There is no current maintainer to > ask. Most of the driver originates from rmk. Ccing him just in case. Thanks a lot for your time and advice. I think moving the del_timer_sync() before the lock is good. Because we may use "sa1100_enable_ms(&sport->port)" to start the timer after we have set termios. > FWIW the lock was moved by this commit around linux 2.5.55 (from > full-history-linux [1]) > commit f38aef3e62c26a33ea360a86fde9b27e183a3748 > Author: Russell King <rmk@flint.arm.linux.org.uk> > Date: Fri Jan 3 15:42:09 2003 +0000 > > [SERIAL] Convert change_speed() to settermios() > > [1] > https://archive.org/download/git-history-of-linux/full-history-linux.git.tar > > > del_timer_sync(&sport->timer); > > + spin_lock_irqsave(&sport->port.lock, flags); > > > > /* > > * Update the per-port timeout. Best regards, Duoming Zhou
diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c index 5fe6cccfc1a..3a5f12ced0b 100644 --- a/drivers/tty/serial/sa1100.c +++ b/drivers/tty/serial/sa1100.c @@ -476,7 +476,9 @@ sa1100_set_termios(struct uart_port *port, struct ktermios *termios, UTSR1_TO_SM(UTSR1_ROR); } + spin_unlock_irqrestore(&sport->port.lock, flags); del_timer_sync(&sport->timer); + spin_lock_irqsave(&sport->port.lock, flags); /* * Update the per-port timeout.
There is a deadlock in sa1100_set_termios(), which is shown below: (Thread 1) | (Thread 2) | sa1100_enable_ms() sa1100_set_termios() | mod_timer() spin_lock_irqsave() //(1) | (wait a time) ... | sa1100_timeout() del_timer_sync() | spin_lock_irqsave() //(2) (wait timer to stop) | ... We hold sport->port.lock in position (1) of thread 1 and use del_timer_sync() to wait timer to stop, but timer handler also need sport->port.lock in position (2) of thread 2. As a result, sa1100_set_termios() 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/tty/serial/sa1100.c | 2 ++ 1 file changed, 2 insertions(+)