Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Wed, 4 Oct 2023 09:57:20 +0100
From:   Lee Jones <lee@kernel.org>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     "Starke, Daniel" <daniel.starke@siemens.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Fedor Pchelkin <pchelkin@ispras.ru>,
        Jiri Slaby <jirislaby@kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com" 
        <syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com>
Subject: Re: [PATCH 1/1] tty: n_gsm: Avoid sleeping during .write() whilst
 atomic
Message-ID: <20231004085720.GA9374@google.com>
References: <20231003170020.830242-1-lee@kernel.org>
 <2023100320-immorally-outboard-573a@gregkh>
 <DB9PR10MB588170E923A6ED8B3D6D9613E0CBA@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
 <2023100421-negotiate-stammer-1b35@gregkh>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2023100421-negotiate-stammer-1b35@gregkh>
Precedence: bulk

On Wed, 04 Oct 2023, Greg Kroah-Hartman wrote:

> On Wed, Oct 04, 2023 at 05:59:09AM +0000, Starke, Daniel wrote:
> > > Daniel, any thoughts?
> > 
> > Our application of this protocol is only with specific modems to enable
> > circuit switched operation (handling calls, selecting/querying networks,
> > etc.) while doing packet switched communication (i.e. IP traffic over PPP).
> > The protocol was developed for such use cases.
> > 
> > Regarding the issue itself:
> > There was already an attempt to fix all this by switching from spinlocks to
> > mutexes resulting in ~20% performance loss. However, the patch was reverted
> > as it did not handle the T1 timer leading into sleep during atomic within
> > gsm_dlci_t1() on every mutex lock there.

That's correct.  When I initially saw this report, my initial thought
was to replace the spinlocks with mutexts, but having read the previous
accepted attempt and it's subsequent reversion I started to think of
other ways to solve this issue.  This solution, unlike the last, does
not involve adding sleep inducing locks into atomic contexts, nor
should it negatively affect performance.

> > There was also a suggestion to fix this in do_con_write() as
> > tty_operations::write() appears to be documented as "not allowed to sleep".
> > The patch for this was rejected. It did not fix the issue within n_gsm.
> > 
> > Link: https://lore.kernel.org/all/20221203215518.8150-1-pchelkin@ispras.ru/
> > Link: https://lore.kernel.org/all/20221212023530.2498025-1-zengheng4@huawei.com/
> > Link: https://lore.kernel.org/all/5a994a13-d1f2-87a8-09e4-a877e65ed166@kernel.org/
> 
> Ok, I thought I remembered this, I'll just drop this patch from my
> review queue and wait for a better solution if it ever comes up as this
> isn't a real issue that people are seeing on actual systems, but just a
> syzbot report.

What does the "better solution" look like?

-- 
Lee Jones [李琼斯]