linux-kernel - Re: [PATCH v2] serial: 8250: fix shared interrupts issues with SMP and RT kernels

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080709192943.GA12278@polina.dev.rtsoft.ru>
Date:	Wed, 9 Jul 2008 23:29:43 +0400
From:	Anton Vorontsov <avorontsov@...mvista.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Ingo Molnar <mingo@...e.hu>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	Daniel Walker <dwalker@...sta.com>
Subject: Re: [PATCH v2] serial: 8250: fix shared interrupts issues with SMP
	and RT kernels

On Tue, Jul 01, 2008 at 07:02:54PM +0400, Anton Vorontsov wrote:
[...]
> Signed-off-by: Anton Vorontsov <avorontsov@...mvista.com>
> ---
> 
> On Tue, Jul 01, 2008 at 02:43:53PM +0100, Alan Cox wrote:
[...]
> > It looks correct to me except that you cannot use spin_lock/disable_irq
> > in that way safely. You must always disable_irq before taking the lock,
> > or prove it is safe and use disable_irq_nosync
> > 
> > The reason:
> > 		CPU#0 spin_lock_... [taken]
> > 		CPU#1 IRQ
> > 		CPU#1 spin_lock [waits]
> > 		CPU#0 disable_irq (deadlock)
> 
> This deadlock possibility is interesting by itself, thanks for
> mentioning it.
> 
> But this can't happen here. IRQ will not grab the up->port.lock,
> because port isn't registered in the 8250 IRQ handling chain (yet).

Alan, are there any issues with the proof or the patch itself?

Thanks,

> As for _nosync, probably this is good idea indeed, and should be safe
> AFAICS.
> 
>  drivers/serial/8250.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 76ccef7..cad0c2d 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1831,7 +1831,9 @@ static int serial8250_startup(struct uart_port *port)
>  		 * the interrupt is enabled.  Delays are necessary to
>  		 * allow register changes to become visible.
>  		 */
> -		spin_lock_irqsave(&up->port.lock, flags);
> +		spin_lock(&up->port.lock);
> +		if (up->port.flags & UPF_SHARE_IRQ)
> +			disable_irq_nosync(up->port.irq);
>  
>  		wait_for_xmitr(up, UART_LSR_THRE);
>  		serial_out_sync(up, UART_IER, UART_IER_THRI);
> @@ -1843,7 +1845,9 @@ static int serial8250_startup(struct uart_port *port)
>  		iir = serial_in(up, UART_IIR);
>  		serial_out(up, UART_IER, 0);
>  
> -		spin_unlock_irqrestore(&up->port.lock, flags);
> +		if (up->port.flags & UPF_SHARE_IRQ)
> +			enable_irq(up->port.irq);
> +		spin_unlock(&up->port.lock);
>  
>  		/*
>  		 * If the interrupt is not reasserted, setup a timer to
> -- 
> 1.5.5.4
> 

-- 
Anton Vorontsov
email: cbouatmailru@...il.com
irc://irc.freenode.net/bd2
--
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/