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: <ZNtE_dcBaDm-wbHt@alley>
Date:   Tue, 15 Aug 2023 11:27:25 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Jiri Slaby <jirislaby@...nel.org>
Cc:     John Ogness <john.ogness@...utronix.de>,
        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 12:21:23, Jiri Slaby wrote:
> On 14. 08. 23, 12:00, Petr Mladek wrote:
> > 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.
> 
> Sorry, but no, the warning is not correct at all. The code path deliberately
> does NOT take the lock and calls a function which is currently annotated
> that the lock is _always_ taken. Therefore, the warning is clearly a false
> positive and I see no reason in keeping it.

There might be a misunderstanding. I only want to keep panic()
implementation as it is for now. I mean to keep calling
debug_locks_off() right before console_flush_on_panic().
The lockdep should stay on before to report potential problems
in non-printk code, like kexec, panic notifiers.

But I am fine with disabling the particular lockdep_assert_held_once()
during panic().

It should stay during the normal system state to catch not
yet discovered races. John is working hard on preventing any
races which might blow up after introducing the printk kthreads.

I mean something like:

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ecfdc4534123..9533c1eedfb1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -704,7 +704,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 static void serial8250_clear_IER(struct uart_8250_port *up)
 {
 	/* Port locked to synchronize UART_IER access against the console. */
-	lockdep_assert_held_once(&up->port.lock);
+	if (!oops_in_progress)
+		lockdep_assert_held_once(&up->port.lock);
 
 	if (up->capabilities & UART_CAP_UUE)
 		serial_out(up, UART_IER, UART_IER_UUE);


Best Regards,
Petr