linux-kernel - Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks

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]

Date:	Tue, 19 Jun 2012 10:35:15 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
CC:	Tomoya MORINAGA <tomoya.rohm@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Feng Tang <feng.tang@...el.com>,
	Alexander Stein <alexander.stein@...tec-electronic.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alan Cox <alan@...ux.intel.com>, linux-serial@...r.kernel.org
Subject: Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive
 spinlocks

On 06/19/2012 02:14 AM, Alan Cox wrote:
> On Mon, 18 Jun 2012 14:41:46 -0700
> Darren Hart <dvhart@...ux.intel.com> wrote:
> 
>>
>>
>> On 06/05/2012 04:48 PM, Tomoya MORINAGA wrote:
>>> On Wed, Jun 6, 2012 at 7:07 AM, Darren Hart <dvhart@...ux.intel.com> wrote:
>>>> Are there still concerns about the additional lock? I'll resend V2
>>>> tomorrow with the single whitespace fix if I don't hear anything back today.
>>>
>>> I understand your saying. Looks good.
>>> However, I am not expert of linux-uart core system.
>>> So, I'd like UART maintainer to give us your opinion.
>>
>> Greg, Alan,
>>
>> any concerns with the locking approach I've adopted in the patch?
> 
> Only the one I noted in my reply the first time around

Hi Alan,

I've hunted, but I can't seem to find this reply. :-/

> which is that you
> can't permit tty->low_latency=1 unless your tty receive path is not an
> IRQ path. From a locking point of view the change makes sense anyway.

I ran into this on the PREEMPT_RT kernel which always uses
tty->low_latency and converts the interrupt handler into a thread.

There is a follow-on patch for RT only to address a sleeping while
atomic bug in pch_console_write(), but I felt _this_ locking structure
change was appropriate for mainline.

> 
> Going back over it your console locking also needs care - an oops or
> printk within the areas the private lock covers will hang the box. That
> should also probably be a trylock style lock as with the other lock on
> that path

I presume you are referring to pch_console_write()?

> static void
> pch_console_write(struct console *co, const char *s, unsigned int count)
> {
> 	struct eg20t_port *priv;
> 	unsigned long flags;
> 	u8 ier;
> 	int locked = 1;
> 
> 	priv = pch_uart_ports[co->index];
> 
> 	touch_nmi_watchdog();
> 
> 	local_irq_save(flags);
> 	spin_lock(&priv->lock);
> 	if (priv->port.sysrq) {
> 		/* serial8250_handle_port() already took the lock */
> 		locked = 0;
> 	} else if (oops_in_progress) {
> 		locked = spin_trylock(&priv->port.lock);
> 	} else
> 		spin_lock(&priv->port.lock);

I see, the oops_in_progress test right? My thinking was that the
oops_in_progress was only relevant to the port.lock as that could be
taken outside of the pch_uart driver, while the priv.lock is only used
within the driver. But, as the oops uses the pch_console_write itself, I
can see the recursive spinlock failure case there.

As for the printk, it seems the 8250 driver would also suffer from that
in the serial8250_console_write function on the port.lock, and it does
not make any allowances for printk.

I would like to hold the priv.lock for a smaller window, but ordering
requires that I take it prior to the port.lock.

So I can test for oops_in_progress on the priv->lock too, but that won't
address the printk issue. Is the oops the bigger concern?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

--
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/