Messages in this thread | | | Date | Tue, 3 Oct 2023 20:14:52 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH 1/1] tty: n_gsm: Avoid sleeping during .write() whilst atomic |
| |
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
| |