linux-kernel - RE: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore

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: <DB3PR0402MB3916F184893B6C6D2881F4EFF5020@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Wed, 5 Sep 2018 01:20:32 +0000
From:   Anson Huang <anson.huang@....com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jslaby@...e.com" <jslaby@...e.com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore

Hi, Uwe

Anson Huang
Best Regards!


> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> Sent: Wednesday, September 5, 2018 4:32 AM
> To: Anson Huang <anson.huang@....com>
> Cc: gregkh@...uxfoundation.org; jslaby@...e.com;
> linux-serial@...r.kernel.org; linux-kernel@...r.kernel.org; dl-linux-imx
> <linux-imx@....com>
> Subject: Re: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore
> 
> Hello,
> 
> On Thu, Aug 09, 2018 at 06:06:08PM +0800, Anson Huang wrote:
> > In noirq suspend/resume stage with no_console_suspend enabled,
> > .imx_console_write() may be called to print out log_buf message by
> > .printk(), so there will be race condition between
> > .imx_console_write() and .serial_imx_save/restore_context(),
> > need to add lock to protect the registers save/restore operations.
> 
> The function names changes some time ago. Also I'd drop the leading dots in
> the names which I would understand as members in a struct.
> 
> Is this a theoretical issue, or did you see actual breakage?
 
I will update the function name and remove the dots.

And we did see actual breakage during stress test, although the reproduce rate
is NOT that high, but the issue came out and fixed by this patch. 

> 
> > Signed-off-by: Anson Huang <Anson.Huang@....com>
> > ---
> >  drivers/tty/serial/imx.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> > 239c0fa..a09ccef 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -2376,9 +2376,12 @@ static int imx_uart_remove(struct
> > platform_device *pdev)
> >
> >  static void imx_uart_restore_context(struct imx_port *sport)  {
> > +	unsigned long flags;
> > +
> >  	if (!sport->context_saved)
> >  		return;
> 
> Is it safe to check .context_saved without holding the lock?
 
Ah, my mistake, will put the check inside the lock. Please help
review V2 patch set, thanks.

Anson.

> 
> > +	spin_lock_irqsave(&sport->port.lock, flags);
> >  	imx_uart_writel(sport, sport->saved_reg[4], UFCR);
> >  	imx_uart_writel(sport, sport->saved_reg[5], UESC);
> >  	imx_uart_writel(sport, sport->saved_reg[6], UTIM); @@ -2390,11
> > +2393,15 @@ static void imx_uart_restore_context(struct imx_port *sport)
> >  	imx_uart_writel(sport, sport->saved_reg[2], UCR3);
> >  	imx_uart_writel(sport, sport->saved_reg[3], UCR4);
> >  	sport->context_saved = false;
> > +	spin_unlock_irqrestore(&sport->port.lock, flags);
> >  }
> >
> >  static void imx_uart_save_context(struct imx_port *sport)  {
> > +	unsigned long flags;
> > +
> >  	/* Save necessary regs */
> > +	spin_lock_irqsave(&sport->port.lock, flags);
> >  	sport->saved_reg[0] = imx_uart_readl(sport, UCR1);
> >  	sport->saved_reg[1] = imx_uart_readl(sport, UCR2);
> >  	sport->saved_reg[2] = imx_uart_readl(sport, UCR3); @@ -2406,6
> > +2413,7 @@ static void imx_uart_save_context(struct imx_port *sport)
> >  	sport->saved_reg[8] = imx_uart_readl(sport, UBMR);
> >  	sport->saved_reg[9] = imx_uart_readl(sport, IMX21_UTS);
> >  	sport->context_saved = true;
> > +	spin_unlock_irqrestore(&sport->port.lock, flags);
> >  }
> >
> >  static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C74
> a28e347934451a59bf08d612a57f8c%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636716899353510311&amp;sdata=lByZDd%2BbQitjEf%2FaiN3
> e26El3ErT0fnmvaKCqckF7Qc%3D&amp;reserved=0  |