Received-SPF: pass (google.com: domain of linux-kernel+bounces-152524-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <6103a212-f84f-4dad-9d33-a18235bd970a@I-love.SAKURA.ne.jp>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 21 Apr 2024 09:04:31 -0700
Message-ID: <CAHk-=wgjZ0DJgeo5Sk-Kc5vw8TXGuxXftPV79Wv221ncstk1tA@mail.gmail.com>
Subject: Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
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>
Content-Type: text/plain; charset="UTF-8"

On Sun, 21 Apr 2024 at 06:28, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> "struct tty_ldisc_ops" says that ->write() function (e.g. gsmld_write())
> is allowed to sleep and "struct tty_operations" says that ->write() function
> (e.g. con_write()) is not allowed to sleep.

Well, clearly con_write() *is* allowed to sleep. The very first thing
it does is that

        console_lock();

thing, which uses a sleeping semaphore.

But yes, the comment in the header does say "may not sleep".

Clearly that comment doesn't actually reflect reality - and never did.
The console lock sleeping isn't some new thing (ie it doesn't come
from the somewhat recent printk changes).

So the comment is bogus and wrong.

> Thus, I initially proposed
> https://lkml.kernel.org/r/9cd9d3eb-418f-44cc-afcf-7283d51252d6@I-love.SAKURA.ne.jp
> which makes con_write() no-op when called with IRQs disabled.

The thing is, that's not the only thing that makes atomic context.

And some atomic contexts cannot be detected at run-time, they are
purely static (ie being inside a spinlock withg a !PREEMPT kernel
build).

So you cannot test for this.

The only option is to *mark* the ones that are atomic. Which was my suggestion.

> My major/minor approach is based on a suggestion from Jiri that we just somehow
> disallow attaching this line discipline to a console

Since we already know that the comment is garbage, why do you think
it's just a con_write() that has this issue?

And if it is only the console that has this issue, why are you testing
for other major/minor numbers?

> 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.

                  Linus