linux-kernel - Re: [PATCH 1/1] tty: serial: handle HAS

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: <0e07c26d-5136-44d8-b9a0-96154050ae8e@roeck-us.net>
Date: Wed, 4 Dec 2024 14:44:11 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Arnd Bergmann <arnd@...nel.org>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Niklas Schnelle <schnelle@...ux.ibm.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 linux-serial@...r.kernel.org, Heiko Carstens <hca@...ux.ibm.com>,
 linux-kernel@...r.kernel.org, Yang Yingliang <yangyingliang@...wei.com>
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

On 12/4/24 14:17, Arnd Bergmann wrote:
> On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:
>> On Mon, Nov 25, 2024 at 04:59:00PM +0100, Arnd Bergmann wrote:
>>> On Mon, Nov 25, 2024, at 12:06, Arnd Bergmann wrote:
>>>> +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
>>>> +
>>>
>>> Unfortunately, this breaks on non-x86 because of the check
>>> added in 59cfc45f17d6 ("serial: 8250: Do nothing if nr_uarts=0").
>>>
>>> I still think it's the right idea, but need to unwind further
>>> to make this possible, and find a different fix for the bug
>>> from that commit.
>>>
>>
>> I decided to apply the patch below to my fixes branch. It doesn't change
>> the code, it just gets rid of the warning backtrace.
> 
> I got stuck in this rabbit hole of running into more issues
> with the 8250 driver. Any time you touch something, it breaks
> elsewhere.
> 
> I've uploaded what I have here now:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=8250-cleanup
> 
> but this probably needs more testing, and a few smaller changes
> 
>>   drivers/tty/serial/8250/8250_port.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c
>> b/drivers/tty/serial/8250/8250_port.c
>> index 4d63d80e78a9..649e74e9b52f 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -467,7 +467,8 @@ static void set_io_from_upio(struct uart_port *p)
>>   		break;
>>   #endif
>>   	default:
>> -		WARN(1, "Unsupported UART type %x\n", p->iotype);
>> +		WARN(p->iotype != UPIO_PORT || p->iobase,
>> +		     "Unsupported UART type %x\n", p->iotype);
>>   		p->serial_in = no_serial_in;
>>   		p->serial_out = no_serial_out;
>>   	}
> 
> I had considered this at first but didn't really want to do
> it like this because we should not be registering empty ports
> on platforms that don't use the setserial style override for
> the port configuration.
> 
> It does of course address the warning, just not the
> underlying bug.
> 

I had a look myself, and concluded that a clean fix will likely require
substantial changes to avoid regressions. Your patch series pretty much
confirms that. This is why I came up with the hack below. Yes, it doesn't
get rid of the underlying problem, but IMO it is good enough for 6.13,
or at least not worse than 6.12, while at the same time avoiding the
warning backtrace. It seems to me that a clean fix would be 6.14 material,
and I really don't want those backtraces to clog up test logs until then.

Thanks,
Guenter