linux-kernel - Re: [RFC][PATCH] serial: amba-pl011: Make sure we initialize the port.lock spinlock

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: <jhj1ro9bzhg.mognet@arm.com>
Date:   Mon, 27 Apr 2020 00:26:19 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Russell King <linux@...linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, linux-serial@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [RFC][PATCH] serial: amba-pl011: Make sure we initialize the port.lock spinlock


On 25/04/20 05:04, John Stultz wrote:
> On Thu, Apr 23, 2020 at 4:14 PM Valentin Schneider
> <valentin.schneider@....com> wrote:
>> On 23/04/20 23:00, John Stultz wrote:
>> > Which seems to be due to the fact that after allocating the uap
>> > structure, the pl011 code doesn't initialize the spinlock.
>> >
>> > This patch fixes it by initializing the spinlock and the warning
>> > has gone away.
>> >
>>
>> Thanks for having a look. It does seem like the reasonable thing to do, and
>> I no longer get the warning on h960.
>>
>> That said, I got more curious as this doesn't show up on my Juno (same
>> Image). Digging into it I see that uart_add_one_port() has a call to
>> uart_port_spin_lock_init() a few lines before uart_configure_port() (in
>> which the above warning gets triggered). That thing says:
>>
>>  * Ensure that the serial console lock is initialised early.
>>  * If this port is a console, then the spinlock is already initialised.
>>
>> Which requires me to ask: are we doing the right thing here?
>
> So I got a little bit of time to look at this before I got pulled off
> to other things (and now its Friday night, so I figured I'd reply
> before I forget it on Monday).
>
> I did check and lockdep is tripping when we add ttyAMA6 which is the
> serial console on the board. I wasn't able to trace back to why we
> hadn't already called spin_lock_init() in the console code, but it
> seems we haven't.
>

So on the Juno (ttyAMA0), the first time I see us hitting
uart_port_spin_lock_init() is via:

  uart_add_one_port() ->  uart_port_spin_lock_init()

Since port->cons->(index, line) is (-1, 0) at this point in time,
uart_console(port) returns false and we init the spinlock. When then
happily trickle down to uart_configure_port() -> register_console()


On the Hikey960 (ttyAMA6) I see the same initial flow, but (index, line)
is (6, 6), so uart_console(port) returns true and we skip the
spin_lock_init(). When then hit the splat on the rest of the way down
uart_add_one_port().


I did a tiny bit of git spelunking; I found a commit that changed
uart_console_enabled() into uart_console() within
uart_port_spin_lock_init():

  a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")

Reverting just that one change in uart_port_spin_lock_init() seems to go
fine on both Juno & HiKey960, but I think that doesn't play well with the
rest of the aforementioned commit. I think that this initial (index, line)
tuple is to blame, though I've added Andy in Cc just in case.

> Also I checked on HiKey as well, and there I'm seeing the same lockdep
> splat and this fix seems to resolve it. So more digging is needed. If
> anyone has a better idea of what might be awry or if the lock does
> need to be initialized in the driver (it's a bit inconsistent, I see
> some drivers do but others don't), let me know.
>



> thanks
> -john