linux-kernel - Re: [PATCH] serial: fix possible imx deadlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 19 Mar 2010 14:57:21 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Sascha Hauer <s.hauer@...gutronix.de>
Cc:	John Ogness <john.ogness@...utronix.de>,
	Sascha Hauer <kernel@...gutronix.de>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] serial: fix possible imx deadlock

On Mon, 15 Mar 2010 14:45:57 +0100
Sascha Hauer <s.hauer@...gutronix.de> wrote:

> On Mon, Mar 15, 2010 at 10:08:58AM +0100, John Ogness wrote:
> > This patch fixes a possible deadlock situation with the port and timer
> > locks. The calling paths for the deadlock situation are:
> > 
> > 1. imx_set_termios() -> LOCK(port.lock)
> >       -> del_timer_sync() -> LOCK(timer.base.lock)
> > 
> > 2. run_timer_softirq() -> LOCK(timer.base.lock)
> >       -> imx_timeout() -> LOCK(port.lock)
> > 
> > This patch is against 2.6.34-rc1.
> > 
> > Signed-off-by: John Ogness <john.ogness@...utronix.de>
> > ---
> >  drivers/serial/imx.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > diff -Naurp a/drivers/serial/imx.c b/drivers/serial/imx.c
> > --- a/drivers/serial/imx.c	2010-03-08 19:45:44.000000000 +0100
> > +++ b/drivers/serial/imx.c	2010-03-15 09:52:39.855261060 +0100
> > @@ -836,6 +836,12 @@ imx_set_termios(struct uart_port *port, 
> >  	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
> >  	quot = uart_get_divisor(port, baud);
> >  
> > +	/*
> > +	 * Stop our timer. This is done with the port unlocked
> > +	 * to avoid possible deadlock against the timer function.
> > +	 */
> > +	del_timer_sync(&sport->timer);
> > +
> 
> Is this call needed here anyway? Only few drivers have it. Does anyone
> have more insights on this?

The other drivers might be buggy ;)

I think the del_timer can probably be removed, as the later
imx_enable_ms() will do a mod_timer() on it.  Unless that
imx_enable_ms() doesn't get performed, of course..

Plus it could be that we want to kill the timer because we're about to
change various pieces of state and we don't want the timer handler
running while that state is in flux.  Presumably that's why it's all
done under the spinlock.

Moving the del_timer_sync() to outside the spinlock might introduce
races though - perhaps userspace can cause the timer to get re-enabled
after it's been del_timer()'ed.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/