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

[Greg Kroah-Hartman]

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@xxxxxxxxxxx>
> Cc: Fedor Pchelkin <pchelkin@xxxxxxxxx>
> Cc: Jiri Slaby <jirislaby@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-serial@xxxxxxxxxxxxxxx
> Reported-by: syzbot+5f47a8cea6a12b77a876@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> ---
>  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