linux-kernel - Re: [PATCH] serial: 8250: drop lockdep annotation from serial8250_clear

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: <ZNn7KHY3iMRarqAZ@alley>
Date:   Mon, 14 Aug 2023 12:00:08 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Jiri Slaby <jirislaby@...nel.org>, gregkh@...uxfoundation.org,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vlastimil Babka <vbabka@...e.cz>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] serial: 8250: drop lockdep annotation from
 serial8250_clear_IER()

On Mon 2023-08-14 10:21:14, John Ogness wrote:
> Hi Jiri,
> 
> Thanks for the follow-up. You responded faster than I could correct
> myself.
> 
> On 2023-08-14, Jiri Slaby <jirislaby@...nel.org> wrote:
> >>> The port lock is not always held when calling serial8250_clear_IER().
> >>> When an oops is in progress, the lock is tried to be taken and when it
> >>> is not, a warning is issued:
> >> 
> >>> The other option would be to make the lockdep test conditional on
> >>> 'oops_in_progress'
> 
> Actually I find this suggestion more appropriate. It makes it clear that
> we are willing to take such risks and do not want to see the warnings in
> a panic situation. However, I would end up having to revert that change
> as well, so it really does not matter to me at this point. Either way I
> will be reverting this patch.

A "solution" would be to move debug_locks_off() before bust_spinlocks(1)
in panic().

debug_locks_off() is currently called before console_flush_on_panic().
I guess that it is because it ignores the result of console_trylock().
But the particular console drivers ignore a trylock result on
the port_lock already after the earlier bust_spinlocks(1).

My concern is that it would hide any other potential races, for
example, in __crash_kexec() or panic notifiers. So, I think that
it might cause more harm then good. Especially because the race
is quite uncommon. It requires activity on the serial port from
two processes during panic().

I personally vote to keep it as is unless people see this warning
on daily basis. After all, the lockdep splat is correct. The serial
console might not work correctly in panic() when there is the race.

Best Regards,
Petr