Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754844AbcDKPU5 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 11 Apr 2016 11:20:57 -0400
Received: from mail-pf0-f172.google.com ([209.85.192.172]:33086 "EHLO
	mail-pf0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750971AbcDKPUx (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 11 Apr 2016 11:20:53 -0400
Subject: Re: [PATCH] drivers/tty: fix BUG_ON when calls
 serial8250_suspend_port
To: Yang Yingliang <yangyingliang@huawei.com>
References: <1460097235-7356-1-git-send-email-yangyingliang@huawei.com>
Cc: linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.cz>
From: Peter Hurley <peter@hurleysoftware.com>
Message-ID: <570BC0CF.5050800@hurleysoftware.com>
Date: Mon, 11 Apr 2016 08:20:47 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <1460097235-7356-1-git-send-email-yangyingliang@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3940
Lines: 106

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 <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Yang Yingliang <yangyingliang@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);
>  }
>