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
