linux-kernel - Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

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:   Fri, 11 Nov 2016 13:28:24 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>, linux-usb@...r.kernel.org,
        x86@...nel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability

On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:

> Things become complicated when it comes to USB debug port.
> But it's still addressable.
> 
> At this time, we can do it like this.
> 
> write()
> {
> 	if (in_nmi() && raw_spin_is_locked(&lock))
> 		return;
> 
> 	raw_spinlock_irqsave(&lock, flags);
> 	....
> 

Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
icky.

Also, there's a bunch of exception contexts that do not set in_nmi().
That is in_nmi() is really only set for #NM. #MC and #DB and
others do not set this.

> This will filter some messages from NMI handler in case that
> another thread is holding the spinlock. I have no idea about
> how much chance could a debug user faces this. But it might
> further be fixed with below enhancement.
> 
> write()
> {
> 	if (in_nmi() && raw_spin_is_locked(&lock)) {
> 		produce_a_pending_item_in_ring();
> 		return;
> 	}
> 
> 	raw_spinlock_irqsave(&lock, flags);
> 
> 	while (!pending_item_ring_is_empty)
> 		consume_a_pending_item_in_ring();
> 
> 	....
> 
> 
> We can design the pending item ring in a producer-consumer
> model. It's easy to avoid race between the producer and
> consumer.

Problem is that the consumer might never happen, those are the fun most
bugs.

Not being able to deal with random nested exception context really
reduces the utility of this thing.

Again, a UART rules. Make a virtual UART in hardware, that'd be totally
awesome. This thing, I'm not convinced its worth having.