[PATCH v2] serial: imx: Fix sysrq deadlock

[PATCH v2] serial: imx: Fix sysrq deadlock

[Fabio Estevam]

The following sysrq command causes the following deadlock:

 # echo t > /proc/sysrq-trigger
 ....
[   20.325246] ======================================================
[   20.325252] WARNING: possible circular locking dependency detected
[   20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
Not tainted
[   20.325273] ------------------------------------------------------
[   20.325279] sh/236 is trying to acquire lock:
[   20.325293] c1618614 (console_owner){-...}-{0:0}, at:
console_unlock+0x180/0x5bc
[   20.325361]
[   20.325361] but task is already holding lock:
[   20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
show_workqueue_state+0x104/0x3c8
[   20.325432]
[   20.325432] which lock already depends on the new lock.

...

[   20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
[   20.325690]        __queue_work+0x114/0x810
[   20.325710]        queue_work_on+0x54/0x94
[   20.325727]        __imx_uart_rxint.constprop.0+0x1b4/0x2e0
[   20.325760]        imx_uart_int+0x270/0x310

This problem happens because uart_handle_sysrq_char() is called
with the lock held.

Fix this by using the same approach done in commit 5697df7322fe ("serial:
fsl_lpuart: split sysrq handling"), which calls 
uart_unlock_and_check_sysrq() to drop the lock prior to 
uart_handle_sysrq_char().

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- I noticed that when sending break + t via the terminal, the characters
were sometimes lost. Do the minimal changes to fix the deadlock without
missing the sysrq input.

 drivers/tty/serial/imx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b121cd869e9..1c768dd3896d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 	unsigned int rx, flg, ignored = 0;
 	struct tty_port *port = &sport->port.state->port;
 
+	uart_unlock_and_check_sysrq(&sport->port);
 	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
 		u32 usr2;
 
@@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 out:
 	tty_flip_buffer_push(port);
 
+	spin_lock(&sport->port.lock);
 	return IRQ_HANDLED;
 }
 
-- 
2.25.1

Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]

Hello Fabio,

On Wed, Sep 29, 2021 at 06:43:24PM -0300, Fabio Estevam wrote:
> The following sysrq command causes the following deadlock:
> 
>  # echo t > /proc/sysrq-trigger
>  ....
> [   20.325246] ======================================================
> [   20.325252] WARNING: possible circular locking dependency detected
> [   20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
> Not tainted
> [   20.325273] ------------------------------------------------------
> [   20.325279] sh/236 is trying to acquire lock:
> [   20.325293] c1618614 (console_owner){-...}-{0:0}, at:
> console_unlock+0x180/0x5bc
> [   20.325361]
> [   20.325361] but task is already holding lock:
> [   20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
> show_workqueue_state+0x104/0x3c8
> [   20.325432]
> [   20.325432] which lock already depends on the new lock.
> 
> ...
> 
> [   20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
> [   20.325690]        __queue_work+0x114/0x810
> [   20.325710]        queue_work_on+0x54/0x94
> [   20.325727]        __imx_uart_rxint.constprop.0+0x1b4/0x2e0
> [   20.325760]        imx_uart_int+0x270/0x310
> 
> This problem happens because uart_handle_sysrq_char() is called
> with the lock held.
> 
> Fix this by using the same approach done in commit 5697df7322fe ("serial:
> fsl_lpuart: split sysrq handling"), which calls 
> uart_unlock_and_check_sysrq() to drop the lock prior to 
> uart_handle_sysrq_char().
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - I noticed that when sending break + t via the terminal, the characters
> were sometimes lost. Do the minimal changes to fix the deadlock without
> missing the sysrq input.
> 
>  drivers/tty/serial/imx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..1c768dd3896d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	unsigned int rx, flg, ignored = 0;
>  	struct tty_port *port = &sport->port.state->port;
>  
> +	uart_unlock_and_check_sysrq(&sport->port);
>  	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
>  		u32 usr2;
>  
> @@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  out:
>  	tty_flip_buffer_push(port);
>  
> +	spin_lock(&sport->port.lock);
>  	return IRQ_HANDLED;

Hmm, this releases the port lock. Are you sure it's correct to e.g.
modify sport->port.icount and various registers and call serial core
functions without holding it?
Also consider imx1 where we have a different irq for tx, rx and
handshaking, so unlocking port.lock might result in a call to
imx_uart_txint or imx_uart_rtsint.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Johan Hovold]

On Wed, Sep 29, 2021 at 06:43:24PM -0300, Fabio Estevam wrote:
> The following sysrq command causes the following deadlock:
> 
>  # echo t > /proc/sysrq-trigger
>  ....
> [   20.325246] ======================================================
> [   20.325252] WARNING: possible circular locking dependency detected
> [   20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
> Not tainted
> [   20.325273] ------------------------------------------------------
> [   20.325279] sh/236 is trying to acquire lock:
> [   20.325293] c1618614 (console_owner){-...}-{0:0}, at:
> console_unlock+0x180/0x5bc
> [   20.325361]
> [   20.325361] but task is already holding lock:
> [   20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
> show_workqueue_state+0x104/0x3c8
> [   20.325432]
> [   20.325432] which lock already depends on the new lock.
> 
> ...
> 
> [   20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
> [   20.325690]        __queue_work+0x114/0x810
> [   20.325710]        queue_work_on+0x54/0x94
> [   20.325727]        __imx_uart_rxint.constprop.0+0x1b4/0x2e0
> [   20.325760]        imx_uart_int+0x270/0x310
> 
> This problem happens because uart_handle_sysrq_char() is called
> with the lock held.
> 
> Fix this by using the same approach done in commit 5697df7322fe ("serial:
> fsl_lpuart: split sysrq handling"), which calls 
> uart_unlock_and_check_sysrq() to drop the lock prior to 
> uart_handle_sysrq_char().
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - I noticed that when sending break + t via the terminal, the characters
> were sometimes lost. Do the minimal changes to fix the deadlock without
> missing the sysrq input.
> 
>  drivers/tty/serial/imx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..1c768dd3896d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	unsigned int rx, flg, ignored = 0;
>  	struct tty_port *port = &sport->port.state->port;
>  
> +	uart_unlock_and_check_sysrq(&sport->port);

This is just so broken; you can't just drop the lock. And you clearly
haven't even tried to understand how uart_unlock_and_check_sysrq()
works.

Please take a closer look at the commit you're trying to mimic.

>  	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
>  		u32 usr2;
>  
> @@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  out:
>  	tty_flip_buffer_push(port);
>  
> +	spin_lock(&sport->port.lock);
>  	return IRQ_HANDLED;
>  }

Johan

Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Fabio Estevam]

Hi Johan,

On 30/09/2021 04:54, Johan Hovold wrote:

> This is just so broken; you can't just drop the lock. And you clearly
> haven't even tried to understand how uart_unlock_and_check_sysrq()
> works.
> 
> Please take a closer look at the commit you're trying to mimic.

Thanks for the feedback.

I have changed it to:


diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b121cd869e9..b7cda50602d5 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -803,7 +803,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void 
*dev_id)
  				continue;
  		}

-		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
+		if (uart_prepare_sysrq_char(&sport->port, rx))
  			continue;

  		if (unlikely(rx & URXD_ERR)) {
@@ -844,6 +844,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void 
*dev_id)
  	}

  out:
+	uart_unlock_and_check_sysrq(&sport->port);
  	tty_flip_buffer_push(port);

  	return IRQ_HANDLED;
@@ -959,6 +960,7 @@ static irqreturn_t imx_uart_int(int irq, void 
*dev_id)
  		imx_uart_writel(sport, USR1_AGTIM, USR1);

  		__imx_uart_rxint(irq, dev_id);
+		spin_lock(&sport->port.lock);
  		ret = IRQ_HANDLED;
  	}

@@ -1977,9 +1979,7 @@ imx_uart_console_write(struct console *co, const 
char *s, unsigned int count)
  	unsigned int ucr1;
  	int locked = 1;

-	if (sport->port.sysrq)
-		locked = 0;
-	else if (oops_in_progress)
+	if (oops_in_progress)
  		locked = spin_trylock_irqsave(&sport->port.lock, flags);
  	else
  		spin_lock_irqsave(&sport->port.lock, flags);

This makes the deadlock not happen after running:
echo t > /proc/sysrq-trigger

, but entering <break> + t via the console does not work anymore.


It returns the sysrq help instead:

sysrq: HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d) 
terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i) 
thaw-filesystems(j) sak(k) show-backtrace-all-active-cpu
s(l) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o) 
show-registers(p) show-all-timers(q) unraw(r) sync(s) 
show-task-states(t) unmount(u) show-blocked-tasks(w) 
dump-ftrace-buffer(z)

Thanks

Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Johan Hovold]

On Thu, Sep 30, 2021 at 10:45:31AM -0300, Fabio Estevam wrote:
> Hi Johan,
> 
> On 30/09/2021 04:54, Johan Hovold wrote:
> 
> > This is just so broken; you can't just drop the lock. And you clearly
> > haven't even tried to understand how uart_unlock_and_check_sysrq()
> > works.
> > 
> > Please take a closer look at the commit you're trying to mimic.
> 
> Thanks for the feedback.
> 
> I have changed it to:
> 
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..b7cda50602d5 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -803,7 +803,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void 
> *dev_id)
>   				continue;
>   		}
> 
> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> +		if (uart_prepare_sysrq_char(&sport->port, rx))

Why did you drop the cast? If there's anything in the high bits you'd
see the help text printed as you report below (even if it seems
unlikely).

>   			continue;
> 
>   		if (unlikely(rx & URXD_ERR)) {
> @@ -844,6 +844,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void 
> *dev_id)
>   	}
> 
>   out:
> +	uart_unlock_and_check_sysrq(&sport->port);
>   	tty_flip_buffer_push(port);
> 
>   	return IRQ_HANDLED;
> @@ -959,6 +960,7 @@ static irqreturn_t imx_uart_int(int irq, void 
> *dev_id)
>   		imx_uart_writel(sport, USR1_AGTIM, USR1);
> 
>   		__imx_uart_rxint(irq, dev_id);
> +		spin_lock(&sport->port.lock);
>   		ret = IRQ_HANDLED;
>   	}

It's a step in the right direction, but you need to restructure the code
so that you don't need to drop and reacquire the lock.

> @@ -1977,9 +1979,7 @@ imx_uart_console_write(struct console *co, const 
> char *s, unsigned int count)
>   	unsigned int ucr1;
>   	int locked = 1;
> 
> -	if (sport->port.sysrq)
> -		locked = 0;
> -	else if (oops_in_progress)
> +	if (oops_in_progress)
>   		locked = spin_trylock_irqsave(&sport->port.lock, flags);
>   	else
>   		spin_lock_irqsave(&sport->port.lock, flags);

And you need to fix the commit summary and commit message since you're
actually fixing any deadlock. You're just suppressing a false positive
lockdep warning due to the above sysrq hack.

> This makes the deadlock not happen after running:
> echo t > /proc/sysrq-trigger
> 
> , but entering <break> + t via the console does not work anymore.
> 
> 
> It returns the sysrq help instead:
> 
> sysrq: HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d) 
> terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i) 
> thaw-filesystems(j) sak(k) show-backtrace-all-active-cpu
> s(l) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o) 
> show-registers(p) show-all-timers(q) unraw(r) sync(s) 
> show-task-states(t) unmount(u) show-blocked-tasks(w) 
> dump-ftrace-buffer(z)

So either you're just pushing garbage to the sysrq handler due to the
dropped cast above or you may, for example, have a NUL char in the
receiver due to the break that you don't discard.

I'd start with logging the key that gets passed to the sysrq handler.

Johan

Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Fabio Estevam]

Hi Johan,

On Fri, Oct 1, 2021 at 4:53 AM Johan Hovold <johan@kernel.org> wrote:

> Why did you drop the cast? If there's anything in the high bits you'd
> see the help text printed as you report below (even if it seems
> unlikely).

That was it, thanks!

I have taken your feedback into consideration and sent a v3.

The only one that I didn't do was to reorganize the code to avoid the
unlock/lock as
this would require a significant rework.

Thanks,

Fabio Estevam

Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Johan Hovold]

On Fri, Oct 01, 2021 at 07:17:53AM -0300, Fabio Estevam wrote:
> Hi Johan,
> 
> On Fri, Oct 1, 2021 at 4:53 AM Johan Hovold <johan@kernel.org> wrote:
> 
> > Why did you drop the cast? If there's anything in the high bits you'd
> > see the help text printed as you report below (even if it seems
> > unlikely).
> 
> That was it, thanks!
> 
> I have taken your feedback into consideration and sent a v3.
> 
> The only one that I didn't do was to reorganize the code to avoid the
> unlock/lock as
> this would require a significant rework.

Judging from a quick look at the code is very straight-forward, and we
don't want to add interrupt latency just to shut up lockdep.

Johan