On 04/07/2016 11:33 PM, Yang Yingliang wrote:
> My kthread calls serial8250_suspend_port/serial8250_resume_port when the
> system is booting, it triggers a BUG_ON:
>
> BUG: failure at
> drivers/tty/serial/8250/8250_core.c:1852/serial_unlink_irq_chain()!
> Kernel panic - not syncing: BUG!
> CPU: 21 PID: 927 Comm: uart_debug_thre Not tainted 4.1.18+ #96
> Call trace:
> [<ffffffc00008988c>] dump_backtrace+0x0/0x128
> [<ffffffc0000899c8>] show_stack+0x14/0x1c
> [<ffffffc0005e3d8c>] dump_stack+0x88/0xa8
> [<ffffffc0005e2cf0>] panic+0xe4/0x228
> [<ffffffc0003c92f4>] univ8250_release_irq+0xc0/0x124
> [<ffffffc0003c8474>] serial8250_do_shutdown+0xdc/0x144
> [<ffffffc0003c84fc>] serial8250_shutdown+0x20/0x28
> [<ffffffc0003c26e4>] uart_suspend_port+0x200/0x234
> [<ffffffc0003c715c>] serial8250_suspend_port+0x5c/0xac
> [<ffffffc000460a50>] uart_debug+0x24/0x3c
> [<ffffffc0000cdf58>] kthread+0xdc/0xf0
>
> I found port->flags is used without lock in tty_port_block_til_ready().
Actually the port->flags are updated within the port->lock critical
section, but as you show below, mixing spinlocked protection with
atomic bit ops is bad.
I fixed this problem more generally, as there are many more places
the port flags are accessed in a non-atomic way.
Please test my series "Replace kernel-defined ASYNC_ bits"; note
that series also has two other patch dependencies which are detailed
in the cover letter.
Regards,
Peter Hurley
> The flags
> will be overwritten with old value after it updates in uart_suspend_port().
> CPU A CPU B
> in uart_suspend_port in tty_port_block_til_ready
> get port->flags
> clear ASYNCB_INITIALIZED
> set port->flags
> The flags still has ASYNCB_INITIALIZED. When uart_suspend_port is called next
> time,
> it trriggers the BUG_ON. So use set_bit/clear_bit to avoid the flags being
> overwritten.
>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: Jiri Slaby <jsl...@suse.cz>
> Signed-off-by: Yang Yingliang <yangyingli...@huawei.com>
> ---
> drivers/tty/tty_port.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 40b3183..d57cf70 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -237,7 +237,7 @@ void tty_port_hangup(struct tty_port *port)
>
> spin_lock_irqsave(&port->lock, flags);
> port->count = 0;
> - port->flags &= ~ASYNC_NORMAL_ACTIVE;
> + clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
> tty = port->tty;
> if (tty)
> set_bit(TTY_IO_ERROR, &tty->flags);
> @@ -376,14 +376,14 @@ int tty_port_block_til_ready(struct tty_port *port,
> /* if non-blocking mode is set we can pass directly to open unless
> the port has just hung up or is in another error state */
> if (tty->flags & (1 << TTY_IO_ERROR)) {
> - port->flags |= ASYNC_NORMAL_ACTIVE;
> + set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
> return 0;
> }
> if (filp->f_flags & O_NONBLOCK) {
> /* Indicate we are open */
> if (tty->termios.c_cflag & CBAUD)
> tty_port_raise_dtr_rts(port);
> - port->flags |= ASYNC_NORMAL_ACTIVE;
> + set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
> return 0;
> }
>
> @@ -443,7 +443,7 @@ int tty_port_block_til_ready(struct tty_port *port,
> port->count++;
> port->blocked_open--;
> if (retval == 0)
> - port->flags |= ASYNC_NORMAL_ACTIVE;
> + set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
> spin_unlock_irqrestore(&port->lock, flags);
> return retval;
> }
> @@ -533,7 +533,8 @@ void tty_port_close_end(struct tty_port *port, struct
> tty_struct *tty)
> spin_lock_irqsave(&port->lock, flags);
> wake_up_interruptible(&port->open_wait);
> }
> - port->flags &= ~(ASYNC_NORMAL_ACTIVE | ASYNC_CLOSING);
> + clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
> + clear_bit(ASYNCB_CLOSING, &port->flags);
> wake_up_interruptible(&port->close_wait);
> spin_unlock_irqrestore(&port->lock, flags);
> }
>
