| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <86452127-70e8-c0cf-de18-6f98e77849a6@i-love.sakura.ne.jp>
Date: Fri, 3 Dec 2021 00:40:42 +0900
From: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Peter Hurley <peter@...leysoftware.com>,
Jiri Slaby <jirislaby@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled
On 2021/12/02 4:05, Linus Torvalds wrote:
> On Wed, Dec 1, 2021 at 5:41 AM Tetsuo Handa
> <penguin-kernel@...ove.sakura.ne.jp> wrote:
>>
>> What do you think? Can we apply this?
>
> I think this patch is only papering over the problem, and the issue goes deeper.
I know. After this "stop bleeding" patch, I am planning to propose a patch for
fixing a regression introduced by commit f9e053dcfc02b0ad ("tty: Serialize
tty flow control changes with flow_lock"), something like shown below.
drivers/tty/tty.h | 2 --
drivers/tty/tty_io.c | 41 ++++++++++++++++++++---------------------
drivers/tty/tty_ioctl.c | 15 +++++++++------
3 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/drivers/tty/tty.h b/drivers/tty/tty.h
index b710c5ef89ab..b19460dca58b 100644
--- a/drivers/tty/tty.h
+++ b/drivers/tty/tty.h
@@ -60,8 +60,6 @@ void tty_ldisc_unlock(struct tty_struct *tty);
int __tty_check_change(struct tty_struct *tty, int sig);
int tty_check_change(struct tty_struct *tty);
-void __stop_tty(struct tty_struct *tty);
-void __start_tty(struct tty_struct *tty);
void tty_vhangup_session(struct tty_struct *tty);
void tty_open_proc_set_tty(struct file *filp, struct tty_struct *tty);
int tty_signal_session_leader(struct tty_struct *tty, int exit_session);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6616d4a0d41d..84f4296eefed 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -761,15 +761,6 @@ int tty_hung_up_p(struct file *filp)
}
EXPORT_SYMBOL(tty_hung_up_p);
-void __stop_tty(struct tty_struct *tty)
-{
- if (tty->flow.stopped)
- return;
- tty->flow.stopped = true;
- if (tty->ops->stop)
- tty->ops->stop(tty);
-}
-
/**
* stop_tty - propagate flow control
* @tty: tty to stop
@@ -791,21 +782,15 @@ void stop_tty(struct tty_struct *tty)
unsigned long flags;
spin_lock_irqsave(&tty->flow.lock, flags);
- __stop_tty(tty);
+ if (!tty->flow.stopped) {
+ tty->flow.stopped = true;
+ if (tty->ops->stop)
+ tty->ops->stop(tty);
+ }
spin_unlock_irqrestore(&tty->flow.lock, flags);
}
EXPORT_SYMBOL(stop_tty);
-void __start_tty(struct tty_struct *tty)
-{
- if (!tty->flow.stopped || tty->flow.tco_stopped)
- return;
- tty->flow.stopped = false;
- if (tty->ops->start)
- tty->ops->start(tty);
- tty_wakeup(tty);
-}
-
/**
* start_tty - propagate flow control
* @tty: tty to start
@@ -821,8 +806,22 @@ void start_tty(struct tty_struct *tty)
{
unsigned long flags;
+ /*
+ * do_con_write() from tty_wakeup() needs to sleep. But I'm not sure
+ * whether all callers are allowed to sleep, for stop_tty() says that
+ * callers might not be allowed to sleep...
+ */
+ might_sleep();
+
spin_lock_irqsave(&tty->flow.lock, flags);
- __start_tty(tty);
+ if (tty->flow.stopped && !tty->flow.tco_stopped) {
+ spin_unlock_irqrestore(&tty->flow.lock, flags);
+ if (tty->ops->start)
+ tty->ops->start(tty);
+ tty_wakeup(tty);
+ spin_lock_irqsave(&tty->flow.lock, flags);
+ tty->flow.stopped = false;
+ }
spin_unlock_irqrestore(&tty->flow.lock, flags);
}
EXPORT_SYMBOL(start_tty);
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 63181925ec1a..84c0742efd34 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -857,6 +857,7 @@ EXPORT_SYMBOL_GPL(tty_perform_flush);
int n_tty_ioctl_helper(struct tty_struct *tty, unsigned int cmd,
unsigned long arg)
{
+ static DEFINE_MUTEX(tty_tco_mutex);
int retval;
switch (cmd) {
@@ -866,20 +867,22 @@ int n_tty_ioctl_helper(struct tty_struct *tty, unsigned int cmd,
return retval;
switch (arg) {
case TCOOFF:
- spin_lock_irq(&tty->flow.lock);
+ if (mutex_lock_killable(&tty_tco_mutex))
+ return -EINTR;
if (!tty->flow.tco_stopped) {
tty->flow.tco_stopped = true;
- __stop_tty(tty);
+ stop_tty(tty);
}
- spin_unlock_irq(&tty->flow.lock);
+ mutex_unlock(&tty_tco_mutex);
break;
case TCOON:
- spin_lock_irq(&tty->flow.lock);
+ if (mutex_lock_killable(&tty_tco_mutex))
+ return -EINTR;
if (tty->flow.tco_stopped) {
tty->flow.tco_stopped = false;
- __start_tty(tty);
+ start_tty(tty);
}
- spin_unlock_irq(&tty->flow.lock);
+ mutex_unlock(&tty_tco_mutex);
break;
case TCIOFF:
if (STOP_CHAR(tty) != __DISABLED_CHAR)
I think that since tty->flow.tco_stopped is updated by only ioctl(TCXONC)
which is schedulable context, we can serialize using a mutex. Then, as
long as start_tty() can be called from schedulable context, we can allow
do_con_write() to work.
>
> It may be that "papering over the issue" successfully hides it
> completely, but it's still a horribly bad approach.
>
>>> - if (in_interrupt())
>>> + if (in_interrupt() || irqs_disabled())
>>> return count;
>
> This kind of stuff is broken. Pretty much always.
>
> And in this case, it's still broken, because things like "called under
> a non-irq spinlock" would still not show up.
As far as I'm aware, the commit did not introduce "called under a non-irq
spinlock" case.
>
> And no, I do *not* mean that the code should try to figure that out. I
> mean that the problem goes further up, and that the fact that we get
> to do_con_write() in the first place when we're in an invalid context
> is wrong, wrong, wrong.
>
> How the heck do we get here from just an ioctl?
Just an ioctl, but the commit made it to get inside spin_lock_irqsave()
section when tty_wakeup() is called.
>
> Looking at the backtrace, I see
>
> n_hdlc_send_frames+0x24b/0x490 drivers/tty/n_hdlc.c:290
> tty_wakeup+0xe1/0x120 drivers/tty/tty_io.c:534
> __start_tty drivers/tty/tty_io.c:806 [inline]
> __start_tty+0xfb/0x130 drivers/tty/tty_io.c:799
>
> and apparently it's that hdlc line discipline (and
> n_hdlc_send_frames() in particular) that is the problem here.
>
> I think that's where the fix should be.
Do you mean that we should change the behavior of n_hdlc_send_frames()
rather than trying to make __start_tty() schedulable again?
Then, n_hdlc_send_frames() saying "this function is called after adding
a frame to the send buffer list and by the tty wakeup callback." but
expecting tty->ops->write (which is do_con_write() which needs to sleep)
not to sleep is wrong?
Then, what we can do with n_hdlc_send_frames() ?
Make n_hdlc_send_frames() no-op when called from atomic context?
Powered by blists - more mailing lists