Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1198862 > unrolled thread
| Started by | Johannes Thumshirn <jthumshirn@suse.de> |
|---|---|
| First post | 2015-08-03 16:00 +0200 |
| Last post | 2015-08-04 09:10 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios() Johannes Thumshirn <jthumshirn@suse.de> - 2015-08-03 16:00 +0200
Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios() Peter Hurley <peter@hurleysoftware.com> - 2015-08-03 17:40 +0200
Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios() Johannes Thumshirn <jthumshirn@suse.de> - 2015-08-04 09:10 +0200
| From | Johannes Thumshirn <jthumshirn@suse.de> |
|---|---|
| Date | 2015-08-03 16:00 +0200 |
| Subject | [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios() |
| Message-ID | <pTorv-4RW-1@gated-at.bofh.it> |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Peter Hurley <peter@hurleysoftware.com> |
|---|---|
| Date | 2015-08-03 17:40 +0200 |
| Subject | Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios() |
| Message-ID | <pTq0i-7cM-41@gated-at.bofh.it> |
| In reply to | #1198862 |
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)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Johannes Thumshirn <jthumshirn@suse.de> |
|---|---|
| Date | 2015-08-04 09:10 +0200 |
| Message-ID | <pTEwh-3hI-1@gated-at.bofh.it> |
| In reply to | #1198971 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web