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

[Greg Kroah-Hartman]

On Wed, Oct 04, 2023 at 10:09:18AM +0100, Lee Jones wrote:
> On Wed, 04 Oct 2023, Greg Kroah-Hartman wrote:
> 
> > On Tue, Oct 03, 2023 at 07:55:00PM +0100, Lee Jones wrote:
> > > On Tue, 03 Oct 2023, Greg Kroah-Hartman wrote:
> > > 
> > > > 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?
> > > 
> > > What it's meant for and what random users can make it do are likely to
> > > be quite separate questions.  This scenario is user driven and can be
> > > replicated simply by issuing a few syscalls (open, ioctl, write).
> > 
> > I would recommend that any distro/system that does not want to support
> > this specific hardware protocol, just disable it for now (it's marked as
> > experimental too), if they don't want to deal with the potential
> > sleep-while-atomic issue.
> 
> n_gsm is available on all the systems I have available.

Then file a bug with your distro to disable it?  No real general purpose
distro should enable it from what I can tell.

> The mention of
> 'EXPERIMENTAL' in the module description appears to have zero effect on
> whether distros choose to make it available or not.  If you're saying
> that we know this module is BROKEN however, then perhaps we should mark
> it as such.

Or we just prevent it from being bound to a console as that's not
something that should be happening.

And again, the "worst" that can happen is the calling process locks up,
due to a lock inversion, right?

> > > > And Lee, you really don't have this hardware, right?  So why are you
> > > > dealing with bug reports for it?  :)
> > > 
> > > 'cos Syzkaller.
> > 
> > Ah, yeah, fake report, no real issue here then :)
> 
> Ouch!  The way I see it, the present issue with Syzkaller is that we do
> not have the resources to remedy all of the issues it flags.  Passing
> off all reports as 'not real issues' is going to make engineers who
> decide to work on them feel undervalued and is likely have a detrimental
> effect overall.  As an ambassador for young and new people trying to get
> into Kernel Engineering in general, is that really the outcome you're
> after?

That's not what I'm saying here at all, what I'm saying is "pick issues
that are real".  syzbot does not always make it obvious what is, and is
not, a real issue.  There have been long threads and discussions about
this and some developers are now just ignoring all syzbot reports (see
the filesystem thread on the ksummit discuss list for more details.)

For this specific issue, it's been much-reported, and is not trivial,
and I would argue, not a "real" problem in the grand scheme of things
for normal users to worry about.

thanks,

greg k-h