linux-kernel - Re: [PATCH tty-next v3 5/6] serial: 8250: Switch to nbcon console

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: <848qu8nyzo.fsf@jogness.linutronix.de>
Date: Mon, 28 Oct 2024 14:28:35 +0106
From: John Ogness <john.ogness@...utronix.de>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Petr Mladek <pmladek@...e.com>, Sergey Senozhatsky
 <senozhatsky@...omium.org>, Steven Rostedt <rostedt@...dmis.org>, Thomas
 Gleixner <tglx@...utronix.de>, Esben Haabendal <esben@...nix.com>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org, Geert
 Uytterhoeven <geert+renesas@...der.be>, Arnd Bergmann <arnd@...db.de>, Uwe
 Kleine-König <u.kleine-koenig@...gutronix.de>, Tony
 Lindgren
 <tony@...mide.com>, Rengarajan S <rengarajan.s@...rochip.com>, Peter
 Collingbourne <pcc@...gle.com>, Serge Semin <fancer.lancer@...il.com>,
 Lino Sanfilippo <l.sanfilippo@...bus.com>
Subject: Re: [PATCH tty-next v3 5/6] serial: 8250: Switch to nbcon console

On 2024-10-25, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
>> +/*
>> + * Only to be used directly by the console write callbacks, which may not
>> + * require the port lock. Use serial8250_clear_IER() instead for all other
>> + * cases.
>> + */
>> +static void __serial8250_clear_IER(struct uart_8250_port *up)
>>  {
>>  	if (up->capabilities & UART_CAP_UUE)
>>  		serial_out(up, UART_IER, UART_IER_UUE);
>
>>  		serial_out(up, UART_IER, 0);
>>  }
>>  
>> +static inline void serial8250_clear_IER(struct uart_8250_port *up)
>> +{
>> +	__serial8250_clear_IER(up);
>
> Shouldn't this have a lockdep annotation to differentiate with the
> above?

Yes, but the follow-up patch adds the annotation as a clean "revert
patch". I can add a line about that in the commit message.

>> +static void serial8250_console_byte_write(struct uart_8250_port *up,
>> +					  struct nbcon_write_context *wctxt)
>> +{
>> +	const char *s = READ_ONCE(wctxt->outbuf);
>> +	const char *end = s + READ_ONCE(wctxt->len);
>
> Is there any possibility that outbuf value be changed before we get
> the len and at the end we get the wrong pointer?

No. I was concerned about compiler optimization, since @outbuf can
become NULL. However, it can only become NULL if ownership was
transferred, and that is properly checked anyway. I will remove the
READ_ONCE() usage for v4.

>>  struct uart_8250_port {
>
>>  	u16			lsr_save_mask;
>>  #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>>  	unsigned char		msr_saved_flags;
>> +	struct irq_work		modem_status_work;
>> +
>> +	bool			console_line_ended;	/* line fully output */
>>  
>>  	struct uart_8250_dma	*dma;
>>  	const struct uart_8250_ops *ops;
>
> Btw, have you run `pahole` on this? Perhaps there are better places
> for new members?

Indeed there are. Placing it above the MSR_SAVE_FLAGS macro will reduce
an existing 3-byte hole to 2-bytes and avoid creating a new 7-byte
hole.

Thanks.

John