[PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[Johannes Thumshirn]

Fix panic caused by a race between men_z135_intr() and men_z135_set_termios().

men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock
spinlock, but men_z135_intr() does a spin_lock_irqsave() and
men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock
when an interrupt is called while the lock is being helt by
men_z135_set_termios().

This was discovered using a insmod, hardware looppback send/receive, rmmod
stress test.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Andreas Werner <andreas.werner@men.de>
---
 drivers/tty/serial/men_z135_uart.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
index 35c5550..d020435 100644
--- a/drivers/tty/serial/men_z135_uart.c
+++ b/drivers/tty/serial/men_z135_uart.c
@@ -656,6 +656,7 @@ static void men_z135_set_termios(struct uart_port *port,
 				struct ktermios *old)
 {
 	struct men_z135_port *uart = to_men_z135(port);
+	unsigned long flags;
 	unsigned int baud;
 	u32 conf_reg;
 	u32 bd_reg;
@@ -717,7 +718,7 @@ static void men_z135_set_termios(struct uart_port *port,
 
 	baud = uart_get_baud_rate(port, termios, old, 0, uart_freq / 16);
 
-	spin_lock(&port->lock);
+	spin_lock_irqsave(&port->lock, flags);
 	if (tty_termios_baud_rate(termios))
 		tty_termios_encode_baud_rate(termios, baud, baud);
 
@@ -725,7 +726,7 @@ static void men_z135_set_termios(struct uart_port *port,
 	iowrite32(bd_reg, port->membase + MEN_Z135_BAUD_REG);
 
 	uart_update_timeout(port, termios->c_cflag, baud);
-	spin_unlock(&port->lock);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static const char *men_z135_type(struct uart_port *port)
-- 
2.4.3

Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[Peter Hurley]

Hi Johannes,

On 08/03/2015 09:58 AM, Johannes Thumshirn wrote:
> Fix panic caused by a race between men_z135_intr() and men_z135_set_termios().
> 
> men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock
> spinlock, but men_z135_intr() does a spin_lock_irqsave() and
> men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock
> when an interrupt is called while the lock is being helt by
> men_z135_set_termios().


The irq handler can and should use normal spin_lock()/unlock().

The set_termios() method should used spin_lock_irq(); there's no need to save the
interrupt state because that method will never be called from interrupt context.

So the 'flags' local can be dropped from the patch.

Also, the port lock is already initialized in uart_add_one_port() and should
not be initialized by the probe() function.

Regards,
Peter Hurley

> This was discovered using a insmod, hardware looppback send/receive, rmmod
> stress test.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Andreas Werner <andreas.werner@men.de>
> ---
>  drivers/tty/serial/men_z135_uart.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
> index 35c5550..d020435 100644
> --- a/drivers/tty/serial/men_z135_uart.c
> +++ b/drivers/tty/serial/men_z135_uart.c
> @@ -656,6 +656,7 @@ static void men_z135_set_termios(struct uart_port *port,
>  				struct ktermios *old)
>  {
>  	struct men_z135_port *uart = to_men_z135(port);
> +	unsigned long flags;
>  	unsigned int baud;
>  	u32 conf_reg;
>  	u32 bd_reg;
> @@ -717,7 +718,7 @@ static void men_z135_set_termios(struct uart_port *port,
>  
>  	baud = uart_get_baud_rate(port, termios, old, 0, uart_freq / 16);
>  
> -	spin_lock(&port->lock);
> +	spin_lock_irqsave(&port->lock, flags);
>  	if (tty_termios_baud_rate(termios))
>  		tty_termios_encode_baud_rate(termios, baud, baud);
>  
> @@ -725,7 +726,7 @@ static void men_z135_set_termios(struct uart_port *port,
>  	iowrite32(bd_reg, port->membase + MEN_Z135_BAUD_REG);
>  
>  	uart_update_timeout(port, termios->c_cflag, baud);
> -	spin_unlock(&port->lock);
> +	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
>  static const char *men_z135_type(struct uart_port *port)
> 

Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[Johannes Thumshirn]

Hi Peter,
Peter Hurley <peter@hurleysoftware.com> writes:

> Hi Johannes,
>
> On 08/03/2015 09:58 AM, Johannes Thumshirn wrote:
>> Fix panic caused by a race between men_z135_intr() and men_z135_set_termios().
>> 
>> men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock
>> spinlock, but men_z135_intr() does a spin_lock_irqsave() and
>> men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock
>> when an interrupt is called while the lock is being helt by
>> men_z135_set_termios().
>
>
> The irq handler can and should use normal spin_lock()/unlock().

I always thought an irq handler _must_ use the irqsave versions. Good to
know that.

>
> The set_termios() method should used spin_lock_irq(); there's no need to save the
> interrupt state because that method will never be called from interrupt context.
>
> So the 'flags' local can be dropped from the patch.

Given that the irqsave variant isn't needed that sounds reasonable.

>
> Also, the port lock is already initialized in uart_add_one_port() and should
> not be initialized by the probe() function.

OK, do you prefer (or better Greg and Jiri) prefer that change folded
into this patch or an extra patch?

>
> Regards,
> Peter Hurley
>
>> This was discovered using a insmod, hardware looppback send/receive, rmmod
>> stress test.
>> 
>> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
>> Cc: Andreas Werner <andreas.werner@men.de>
>> ---
>>  drivers/tty/serial/men_z135_uart.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
>> index 35c5550..d020435 100644
>> --- a/drivers/tty/serial/men_z135_uart.c
>> +++ b/drivers/tty/serial/men_z135_uart.c
>> @@ -656,6 +656,7 @@ static void men_z135_set_termios(struct uart_port *port,
>>  				struct ktermios *old)
>>  {
>>  	struct men_z135_port *uart = to_men_z135(port);
>> +	unsigned long flags;
>>  	unsigned int baud;
>>  	u32 conf_reg;
>>  	u32 bd_reg;
>> @@ -717,7 +718,7 @@ static void men_z135_set_termios(struct uart_port *port,
>>  
>>  	baud = uart_get_baud_rate(port, termios, old, 0, uart_freq / 16);
>>  
>> -	spin_lock(&port->lock);
>> +	spin_lock_irqsave(&port->lock, flags);
>>  	if (tty_termios_baud_rate(termios))
>>  		tty_termios_encode_baud_rate(termios, baud, baud);
>>  
>> @@ -725,7 +726,7 @@ static void men_z135_set_termios(struct uart_port *port,
>>  	iowrite32(bd_reg, port->membase + MEN_Z135_BAUD_REG);
>>  
>>  	uart_update_timeout(port, termios->c_cflag, baud);
>> -	spin_unlock(&port->lock);
>> +	spin_unlock_irqrestore(&port->lock, flags);
>>  }
>>  
>>  static const char *men_z135_type(struct uart_port *port)
>> 
>

-- 
Johannes Thumshirn                                           Storage
jthumshirn@suse.de                                 +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600  D0D0 0393 969D 2D76 0850

Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[Peter Hurley]

On 08/04/2015 03:02 AM, Johannes Thumshirn wrote:
> Peter Hurley <peter@hurleysoftware.com> writes:
>> On 08/03/2015 09:58 AM, Johannes Thumshirn wrote:
>>> Fix panic caused by a race between men_z135_intr() and men_z135_set_termios().
>>>
>>> men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock
>>> spinlock, but men_z135_intr() does a spin_lock_irqsave() and
>>> men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock
>>> when an interrupt is called while the lock is being helt by
>>> men_z135_set_termios().
>>
>>
>> The irq handler can and should use normal spin_lock()/unlock().
> 
> I always thought an irq handler _must_ use the irqsave versions. Good to
> know that.

Your irq handler does not need to protect itself from re-entrancy (by using
the same irq handler for different irqs) and your serial driver doesn't support
console (so can't be deadlocked by printk() usage either).

>> The set_termios() method should used spin_lock_irq(); there's no need to save the
>> interrupt state because that method will never be called from interrupt context.
>>
>> So the 'flags' local can be dropped from the patch.
> 
> Given that the irqsave variant isn't needed that sounds reasonable.

It's for a different reason; irqs will _always_ be on when your driver's
set_termios() method is called. So you don't see to save the irq state, because
you know it's always on. That's why you can use the spin_lock_irq()/spin_unlock_irq()
version here.

>> Also, the port lock is already initialized in uart_add_one_port() and should
>> not be initialized by the probe() function.
> 
> OK, do you prefer (or better Greg and Jiri) prefer that change folded
> into this patch or an extra patch?

Separate patch please.

I assume this deadlock fix will need to be pushed to -stable as well, yes?

Regards,
Peter Hurley

>>> This was discovered using a insmod, hardware looppback send/receive, rmmod
>>> stress test.
>>>
>>> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
>>> Cc: Andreas Werner <andreas.werner@men.de>
>>> ---
>>>  drivers/tty/serial/men_z135_uart.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
>>> index 35c5550..d020435 100644
>>> --- a/drivers/tty/serial/men_z135_uart.c
>>> +++ b/drivers/tty/serial/men_z135_uart.c
>>> @@ -656,6 +656,7 @@ static void men_z135_set_termios(struct uart_port *port,
>>>  				struct ktermios *old)
>>>  {
>>>  	struct men_z135_port *uart = to_men_z135(port);
>>> +	unsigned long flags;
>>>  	unsigned int baud;
>>>  	u32 conf_reg;
>>>  	u32 bd_reg;
>>> @@ -717,7 +718,7 @@ static void men_z135_set_termios(struct uart_port *port,
>>>  
>>>  	baud = uart_get_baud_rate(port, termios, old, 0, uart_freq / 16);
>>>  
>>> -	spin_lock(&port->lock);
>>> +	spin_lock_irqsave(&port->lock, flags);
>>>  	if (tty_termios_baud_rate(termios))
>>>  		tty_termios_encode_baud_rate(termios, baud, baud);
>>>  
>>> @@ -725,7 +726,7 @@ static void men_z135_set_termios(struct uart_port *port,
>>>  	iowrite32(bd_reg, port->membase + MEN_Z135_BAUD_REG);
>>>  
>>>  	uart_update_timeout(port, termios->c_cflag, baud);
>>> -	spin_unlock(&port->lock);
>>> +	spin_unlock_irqrestore(&port->lock, flags);
>>>  }
>>>  
>>>  static const char *men_z135_type(struct uart_port *port)
>>>
>>
> 

Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[Johannes Thumshirn]

Peter Hurley <peter@hurleysoftware.com> writes:

> On 08/04/2015 03:02 AM, Johannes Thumshirn wrote:
>> Peter Hurley <peter@hurleysoftware.com> writes:
>>> On 08/03/2015 09:58 AM, Johannes Thumshirn wrote:
>>>> Fix panic caused by a race between men_z135_intr() and men_z135_set_termios().
>>>>
>>>> men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock
>>>> spinlock, but men_z135_intr() does a spin_lock_irqsave() and
>>>> men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock
>>>> when an interrupt is called while the lock is being helt by
>>>> men_z135_set_termios().
>>>
>>>
>>> The irq handler can and should use normal spin_lock()/unlock().
>> 
>> I always thought an irq handler _must_ use the irqsave versions. Good to
>> know that.
>
> Your irq handler does not need to protect itself from re-entrancy (by using
> the same irq handler for different irqs) and your serial driver doesn't support
> console (so can't be deadlocked by printk() usage either).
>

So once we have console support (I don't know if this is planned at
all), we must go back to the irqsave variant? But looking at the driver
again I have the feeling that the locking could be made more fine
grained (if this makes sense) and I saw two possible races, please
correct me if I'm wrong.

men_z135_intr() reads the status register and saves a copy in struct
men_z135_port::stat_reg. The men_z135_handle_lsr() and
men_z135_handle_modem_status() functions use this stat_reg value to get
to the LSR and MSR registers. But in the meanwhile the content of the
registers could have been changed by men_z135_intr() again. Is this a
problem or am I on the wrong track here?

>>> The set_termios() method should used spin_lock_irq(); there's no need to save the
>>> interrupt state because that method will never be called from interrupt context.
>>>
>>> So the 'flags' local can be dropped from the patch.
>> 
>> Given that the irqsave variant isn't needed that sounds reasonable.
>
> It's for a different reason; irqs will _always_ be on when your driver's
> set_termios() method is called. So you don't see to save the irq state, because
> you know it's always on. That's why you can use the spin_lock_irq()/spin_unlock_irq()
> version here.
>
>>> Also, the port lock is already initialized in uart_add_one_port() and should
>>> not be initialized by the probe() function.
>> 
>> OK, do you prefer (or better Greg and Jiri) prefer that change folded
>> into this patch or an extra patch?
>
> Separate patch please.

OK.

>
> I assume this deadlock fix will need to be pushed to -stable as well,
> yes?

I wasn't quite sure about this, I 1st had a CC stable for v4.0+ but
then removed it again before sending the patch. So I'll put it back in.

Thanks,
        Johannes

Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios()

[Peter Hurley]

On 08/05/2015 04:06 AM, Johannes Thumshirn wrote:
> Peter Hurley <peter@hurleysoftware.com> writes:
> 
>> On 08/04/2015 03:02 AM, Johannes Thumshirn wrote:
>>> Peter Hurley <peter@hurleysoftware.com> writes:
>>>> On 08/03/2015 09:58 AM, Johannes Thumshirn wrote:
>>>>> Fix panic caused by a race between men_z135_intr() and men_z135_set_termios().
>>>>>
>>>>> men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock
>>>>> spinlock, but men_z135_intr() does a spin_lock_irqsave() and
>>>>> men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock
>>>>> when an interrupt is called while the lock is being helt by
>>>>> men_z135_set_termios().
>>>>
>>>>
>>>> The irq handler can and should use normal spin_lock()/unlock().
>>>
>>> I always thought an irq handler _must_ use the irqsave versions. Good to
>>> know that.
>>
>> Your irq handler does not need to protect itself from re-entrancy (by using
>> the same irq handler for different irqs) and your serial driver doesn't support
>> console (so can't be deadlocked by printk() usage either).
>>
> 
> So once we have console support (I don't know if this is planned at
> all), we must go back to the irqsave variant?

Depends on if the platform for this hardware always disables irqs for interrupt
handlers. If it does, then the irqsave variant _in the interrupt handler_ is
not required.


> But looking at the driver
> again I have the feeling that the locking could be made more fine
> grained (if this makes sense) and I saw two possible races, please
> correct me if I'm wrong.
> 
> men_z135_intr() reads the status register and saves a copy in struct
> men_z135_port::stat_reg. The men_z135_handle_lsr() and
> men_z135_handle_modem_status() functions use this stat_reg value to get
> to the LSR and MSR registers. But in the meanwhile the content of the
> registers could have been changed by men_z135_intr() again. Is this a
> problem or am I on the wrong track here?

The irq handler itself can _never_ be re-entered (as long as the handler only
handles one irq # for a given device). So the race(s) outlined above can never
happen _regardless of which flavor spinlock is used_.

Regards,
Peter Hurley


>>>> The set_termios() method should used spin_lock_irq(); there's no need to save the
>>>> interrupt state because that method will never be called from interrupt context.
>>>>
>>>> So the 'flags' local can be dropped from the patch.
>>>
>>> Given that the irqsave variant isn't needed that sounds reasonable.
>>
>> It's for a different reason; irqs will _always_ be on when your driver's
>> set_termios() method is called. So you don't see to save the irq state, because
>> you know it's always on. That's why you can use the spin_lock_irq()/spin_unlock_irq()
>> version here.
>>
>>>> Also, the port lock is already initialized in uart_add_one_port() and should
>>>> not be initialized by the probe() function.
>>>
>>> OK, do you prefer (or better Greg and Jiri) prefer that change folded
>>> into this patch or an extra patch?
>>
>> Separate patch please.
> 
> OK.
> 
>>
>> I assume this deadlock fix will need to be pushed to -stable as well,
>> yes?
> 
> I wasn't quite sure about this, I 1st had a CC stable for v4.0+ but
> then removed it again before sending the patch. So I'll put it back in.
> 
> Thanks,
>         Johannes
>