lkml.org 
[lkml]   [2021]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled
From
On 2021/12/02 4:05, Linus Torvalds wrote:
> On Wed, Dec 1, 2021 at 5:41 AM Tetsuo Handa
> <penguin-kernel@i-love.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?

\
 
 \ /
  Last update: 2021-12-02 16:42    [W:0.097 / U:1.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site