| 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
| ||
|
Message-ID: <CAOCk7NqZmBYN4tY0_V8xzvBfWShDCP8gTa60Aoc78wK2tXx=6A@mail.gmail.com>
Date: Mon, 2 Dec 2019 09:40:55 -0700
From: Jeffrey Hugo <jeffrey.l.hugo@...il.com>
To: Leo Yan <leo.yan@...aro.org>
Cc: Andy Gross <agross@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Stephen Boyd <swboyd@...omium.org>,
Nicolas Dechesne <nicolas.dechesne@...aro.org>,
MSM <linux-arm-msm@...r.kernel.org>,
linux-serial@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] tty: serial: msm_serial: Fix deadlock caused by
recursive output
On Wed, Nov 27, 2019 at 7:16 AM Leo Yan <leo.yan@...aro.org> wrote:
>
> The uart driver might run into deadlock caused by recursive output; the
> basic flow is after uart driver has acquired the port spinlock, then it
> invokes some functions, e.g. dma engine operations and allocate dma
> descriptor with kzalloc(), if the system has very less free memory, the
> kernel will give out warning by printing logs, thus recursive output
> will happen and at the second time the attempting to acquire lock will
> cause deadlock. The detailed flow is shown as below:
>
> msm_uart_irq()
> spin_lock_irqsave(&port->lock, flags) => First time to acquire lock
> msm_handle_tx(port)
> msm_handle_tx_dma()
> dmaengine_prep_slave_single()
> bam_prep_slave_sg()
> kzalloc()
> __kmalloc()
> ___slab_alloc()
> alloc_pages_current()
> __alloc_pages_nodemask()
> warn_alloc()
> printk()
> msm_console_write()
> __msm_console_write()
> spin_lock(&port->lock) => Cause deadlock
>
> This patch fixes the deadlock issue for recursive output; it adds a
> variable 'curr_user' to indicate the uart port is used by which CPU, if
> the CPU has acquired spinlock and wants to execute recursive output,
> it will directly bail out. Here we don't choose to avoid locking and
> print out log, the reason is in this case we don't want to reset the
> uart port with function msm_reset_dm_count(); otherwise it can introduce
> confliction with other flows and results in uart port malfunction and
> later cannot output anymore.
Is this not fixable? Sure, fixing the deadlock is an improvement, but
dropping logs (particularly a memory warning like in your example)
seems undesirable.
>
> Fixes: 99693945013a ("tty: serial: msm: Add RX DMA support")
> Fixes: 3a878c430fd6 ("tty: serial: msm: Add TX DMA support")
> Signed-off-by: Leo Yan <leo.yan@...aro.org>
> ---
> drivers/tty/serial/msm_serial.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 889538182e83..06076cd2948f 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -182,6 +182,7 @@ struct msm_port {
> bool break_detected;
> struct msm_dma tx_dma;
> struct msm_dma rx_dma;
> + struct cpumask curr_user;
> };
>
> #define UART_TO_MSM(uart_port) container_of(uart_port, struct msm_port, uart)
> @@ -436,6 +437,7 @@ static void msm_complete_tx_dma(void *args)
> u32 val;
>
> spin_lock_irqsave(&port->lock, flags);
> + cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
>
> /* Already stopped */
> if (!dma->count)
> @@ -470,6 +472,7 @@ static void msm_complete_tx_dma(void *args)
>
> msm_handle_tx(port);
> done:
> + cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
> spin_unlock_irqrestore(&port->lock, flags);
> }
>
> @@ -544,6 +547,7 @@ static void msm_complete_rx_dma(void *args)
> u32 val;
>
> spin_lock_irqsave(&port->lock, flags);
> + cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
>
> /* Already stopped */
> if (!dma->count)
> @@ -590,6 +594,7 @@ static void msm_complete_rx_dma(void *args)
>
> msm_start_rx_dma(msm_port);
> done:
> + cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
> spin_unlock_irqrestore(&port->lock, flags);
>
> if (count)
> @@ -931,6 +936,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
> u32 val;
>
> spin_lock_irqsave(&port->lock, flags);
> + cpumask_set_cpu(smp_processor_id(), &msm_port->curr_user);
> misr = msm_read(port, UART_MISR);
> msm_write(port, 0, UART_IMR); /* disable interrupt */
>
> @@ -962,6 +968,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
> msm_handle_delta_cts(port);
>
> msm_write(port, msm_port->imr, UART_IMR); /* restore interrupt */
> + cpumask_clear_cpu(smp_processor_id(), &msm_port->curr_user);
> spin_unlock_irqrestore(&port->lock, flags);
>
> return IRQ_HANDLED;
> @@ -1572,6 +1579,7 @@ static inline struct uart_port *msm_get_port_from_line(unsigned int line)
> static void __msm_console_write(struct uart_port *port, const char *s,
> unsigned int count, bool is_uartdm)
> {
> + struct msm_port *msm_port = UART_TO_MSM(port);
> int i;
> int num_newlines = 0;
> bool replaced = false;
> @@ -1593,6 +1601,8 @@ static void __msm_console_write(struct uart_port *port, const char *s,
> locked = 0;
> else if (oops_in_progress)
> locked = spin_trylock(&port->lock);
> + else if (cpumask_test_cpu(smp_processor_id(), &msm_port->curr_user))
> + return;
> else
> spin_lock(&port->lock);
>
> --
> 2.17.1
>
Powered by blists - more mailing lists