Received-SPF: pass (google.com: domain of linux-kernel+bounces-155473-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <563ec0ed-a851-450b-aed6-986f6ea324ca@I-love.SAKURA.ne.jp>
Date: Wed, 24 Apr 2024 00:26:53 +0900
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Andrew Morton
 <akpm@linux-foundation.org>,
        "Starke, Daniel" <daniel.starke@siemens.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-security-module <linux-security-module@vger.kernel.org>
References: <e696e720-0cd3-4505-8469-a94815b39467@I-love.SAKURA.ne.jp>
 <CAHk-=wjEZvnn51dhhLqBKUd=cuFhbYA47_OyfUOPB-0zKToL7Q@mail.gmail.com>
 <CAHk-=wjzqaqcicTtWfBtXyytJs1nqjJNved2JFsLVsVLYgVkuQ@mail.gmail.com>
 <CAHk-=wjW3PdOZ7PJ+RHUKRc8SqQhcWXCACOvmwBkKUKABHKqwg@mail.gmail.com>
 <6103a212-f84f-4dad-9d33-a18235bd970a@I-love.SAKURA.ne.jp>
 <CAHk-=wgjZ0DJgeo5Sk-Kc5vw8TXGuxXftPV79Wv221ncstk1tA@mail.gmail.com>
 <CAHk-=wg+hJ9Y8AKjp9qD7E_-pgBFdWGLiqzi1qth8LNpuST1cA@mail.gmail.com>
Content-Language: en-US
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <CAHk-=wg+hJ9Y8AKjp9qD7E_-pgBFdWGLiqzi1qth8LNpuST1cA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2024/04/22 1:04, Linus Torvalds wrote:
>> Now, your 'struct tty_operations' flag saying 'my ->write() function is OK with
>> atomic context' is expected to be set to all drivers.
> 
> I'm not convinced. The only thing I know is that the comment in
> question is wrong, and has been wrong for over a decade (and honestly,
> probably pretty much forever).
> 
> So how confident are we that other tty write functions are ok?
> 
> Also, since you think that only con_write() has a problem, why the
> heck are you then testing for ptys etc? From a quick check, the
> pty->ops->write() function is fine.

I tried to make deny-listing as close as allow-listing. But it seems that
ipw_write() in drivers/tty/ipwireless/tty.c (e.g. /dev/ttyIPWp0 ) does
sleep as well as con_write() in drivers/tty/vt/vt.c .

I couldn't judge serial_write() in drivers/usb/serial/usb-serial.c (e.g.
/dev/ttyUSB0 ). But since device number for /dev/ttyIPWp0 is assigned
dynamically, we can't rely on major/minor for detecting /dev/ttyIPWp0 from
gsmld_open() side.

That is, we need to handle this problem from "struct tty_operations" side
(like I initially tried).


On 2024/04/22 2:18, Linus Torvalds wrote:
> On Sun, 21 Apr 2024 at 09:04, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> The only option is to *mark* the ones that are atomic. Which was my suggestion.

Since majority of "struct tty_operations"->write() seems to be atomic,
I prefer updating only ones which cannot be atomic.

> 
> Actually, another option would be to just return an error at 'set_ldisc()' time.
> 
> Sadly, the actual "tty->ops->set_ldisc()" function not only returns
> 'void' (easy enough to change - there aren't that many of them), but
> it's called too late after the old ldisc has already been dropped.
> It's basically a "inform tty about new ldisc" and is not useful for a
> "is this ok"?
> 
> But we could trivially add a "ldisc_ok()" function, and have the vt
> driver say "I only accept N_TTY".
> 
> Something like this ENTIRELY UNTESTED patch.
> 
> Again - this is untested, and maybe there are other tty drivers that
> have issues with the stranger line disciplines, but this at least
> seems simple and fairly easy to explain why we do what we do..

This patch works for me. You can propose a formal patch.