Re: [PATCH 1/1] tty: n_gsm: Avoid sleeping during .write() whilst atomic

Greg Kroah-Hartman
2023-10-03T18:14:58Z

On Tue, Oct 03, 2023 at 06:00:20PM +0100, Lee Jones wrote:
> The important part of the call stack being:
>
> gsmld_write() # Takes a lock and disables IRQs
> con_write()
> console_lock()

Wait, why is the n_gsm line discipline being used for a console?

What hardware/protocol wants this to happen?

gsm I thought was for a very specific type of device, not a console.

As per:
https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html
this is a specific modem protocol, why is con_write() being called?

> __might_sleep()
> __might_resched() # Complains that IRQs are disabled
>
> To fix this, let's ensure mutual exclusion by using a protected shared
> variable (busy) instead. We'll use the current locking mechanism to
> protect it, but ensure that the locks are released and IRQs re-enabled
> by the time we transit further down the call chain which may sleep.
>
> Cc: Daniel Starke <daniel.starke@siemens.com>
> Cc: Fedor Pchelkin <pchelkin@ispras.ru>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-serial@vger.kernel.org
> Reported-by: syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> drivers/tty/n_gsm.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 1f3aba607cd51..b83a97d58381f 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -270,6 +270,7 @@ struct gsm_mux {
> struct tty_struct *tty; /* The tty our ldisc is bound to */
> spinlock_t lock;
> struct mutex mutex;
> + bool busy;
> unsigned int num;
> struct kref ref;
>
> @@ -3253,6 +3254,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
> gsm->dead = true; /* Avoid early tty opens */
> gsm->wait_config = false; /* Disabled */
> gsm->keep_alive = 0; /* Disabled */
> + gsm->busy = false;
>
> /* Store the instance to the mux array or abort if no space is
> * available.
> @@ -3718,11 +3720,21 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
>
> ret = -ENOBUFS;
> spin_lock_irqsave(&gsm->tx_lock, flags);
> + if (gsm->busy) {
> + spin_unlock_irqrestore(&gsm->tx_lock, flags);
> + return -EBUSY;

So you just "busted" the re-entrant call chain here, are you sure this
is ok for this protocl? Can it handle -EBUSY?

Daniel, any thoughts?

And Lee, you really don't have this hardware, right? So why are you
dealing with bug reports for it? :)

thanks,

greg k-h