Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

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?