linux-kernel - Re: [PATCH 2.6.26] SERIAL DRIVER: Handle Multiple consecutive sysrq from the serial

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] [day] [month] [year] [list]

Message-ID: <48D1FC1C.90409@extricom.com>
Date:	Thu, 18 Sep 2008 09:58:36 +0300
From:	Eran Liberty <liberty@...ricom.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH 2.6.26] SERIAL DRIVER: Handle Multiple consecutive sysrq
 from the serial

Andrew Morton wrote:
> On Mon, 15 Sep 2008 19:30:05 +0300
> Eran Liberty <liberty@...ricom.com> wrote:
>
>   
>> Dear Penguins,
>>
>> Let me start of by saying my particular hardware must be buggy in some 
>> way. When I issue a sysrq (Ctrl A+ F from minicom) I get a lot of sysrq 
>> triggers.
>>
>> I have worked around the problem and I think this workaround is a viable 
>> patch even for platforms which do not exhibit this peculiar behavior.
>>
>> upon getting numerous interrupts which request sysrq the function 
>> uart_handle_break in include/linux/serial_core.h is hit multiple times.
>> The current code which looks like this:
>>
>> static inline int uart_handle_break(struct uart_port *port)
>>  {
>>         struct uart_info *info = port->info;
>>  #ifdef SUPPORT_SYSRQ
>>         if (port->cons && port->cons->index == port->line) {
>>                if (!port->sysrq) {
>>                        port->sysrq = jiffies + HZ*5;
>>                        return 1;
>>                }
>>                port->sysrq = 0;
>>         }
>>  #endif
>>         if (port->flags & UPF_SAK)
>>                 do_SAK(info->tty);
>>        return 0;
>> }
>>
>> Will basicly toggle port->sysrq between a timeout value and zero. If you 
>> are lucky this penguin rullet will stop on timeout and the next 
>> character hit will trigger the sysrq in the function 
>> "uart_handle_sysrq_char". But if you are not so lucky the last sysrq 
>> interupt will toggle port->sysrq to zero and the next char hit will be 
>> ignored (not trigger sysrq).
>>
>> The suggested patch will do the next few things:
>>
>> 1. "port->sysrq" is now the time when the last sysrq was triggered and 
>> not the timeout for the the next char
>> 2. Stamped "port->sysrq" every time there is a sysrq rather then toggled 
>> it up and down.
>> 3. Always continue to consider UPF_SAK.
>> 4. "port->sysrq" is toggled back to zero only in uart_handle_break() and 
>> only if the a char has been accepted after the sysrq timeout (5 sec)
>> 5. uart_handle_break() will ignore extra chars received in super human 
>> speed after the last sysrq (0.01 sec)
>>
>>     
>
> yes, that could be irritating.
>
>   
>> Index: include/linux/serial_core.h
>> ===================================================================
>> --- include/linux/serial_core.h	(revision 119)
>> +++ include/linux/serial_core.h	(revision 120)
>>     
>
> We prefer patches in `patch -p1' form, please.
>
> Even after fixing that, none of it applied, so I typed it in again.
>   
So  should I resubmit or have you done then nasty work for me (thanks :) )?
>   
>> @@ -447,8 +447,8 @@
>>  uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>>  {
>>  #ifdef SUPPORT_SYSRQ
>> -	if (port->sysrq) {
>> -		if (ch && time_before(jiffies, port->sysrq)) {
>> +	if (port->sysrq && time_after(jiffies, port->sysrq + (unsigned long)(HZ*0.01))) {
>> +		if (ch && time_before(jiffies, port->sysrq + HZ*5)) {
>>  			handle_sysrq(ch, port->info ? port->info->tty : NULL);
>>  			port->sysrq = 0;
>>  			return 1;
>> @@ -467,19 +467,17 @@
>>   */
>>  static inline int uart_handle_break(struct uart_port *port)
>>  {
>> +	int ret = 0;
>>  	struct uart_info *info = port->info;
>>  #ifdef SUPPORT_SYSRQ
>>  	if (port->cons && port->cons->index == port->line) {
>> -		if (!port->sysrq) {
>> -			port->sysrq = jiffies + HZ*5;
>> -			return 1;
>> -		}
>> -		port->sysrq = 0;
>> +		port->sysrq = jiffies;
>> +		ret = 1;
>>  	}
>>  #endif
>>  	if (port->flags & UPF_SAK)
>>  		do_SAK(info->tty);
>> -	return 0;
>> +	return ret;
>>  }
>>     
>
> The 0.01 is a big no-no.  Sometimes gcc like to go into stupid mode and
> starts doing floating point stuff.
>
> A suitable fix would be to use HZ/100.  But that assumes that HZ is
> always >= 100.  That's a pretty good assumption, and various parts of
> the kernel will explode if HZ is set too small.  However it's always
> good to ensure that someone else's stuff will explode before yours
> does, so how about we make it HZ/50?  Will that still work OK for you?
>   

HZ/50 is totally OK by me.


-- Liberty
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/