Re: [PATCH] vt: Fix sleeping functions called from atomic context

[Tetsuo Handa]

On 2021/11/18 18:38, Fabio M. De Francesco wrote:
>> Actually, we don't care of start_tty(). It's not in the path that triggers sleeping in atomic bug.
>> According to Syzbot report and to my ftrace analysis it's __start_tty() that is called by 
>> n_tty_ioctl_helper(), and it is this function that acquires a spinlock and disables interrupts. 
>>
>> I must admit that I've never used git-blame and I'm not sure to understand what you did here :(

I just dumped the source code as of before these commits.

> 
> I've had a chance to look both at commit c545b66c6922 and f9e053dcfc02. They are so strictly 
> related (same code. same author, same date) that I'm not anymore sure of which is to blame.
> 
> However, at this moment I'm scarcely interested in figuring out which one actually is responsible 
> for this issue.

Commit f9e053dcfc02b0ad ("tty: Serialize tty flow control changes with flow_lock") is responsible,
for that commit says

  Split out unlocked __start_tty()/__stop_tty() flavors for use by
  ioctl(TCXONC) in follow-on patch.

and syzbot is hitting n_tty_ioctl_helper(tty, TCXONC, TCOON) path.

>>> Well, how to fix? Introduce a new flag for indicating "starting" state (like drivers/block/loop.c uses Lo_* state) ?
>>
>> I think this is not the correct fix, but I might very well be wrong...
> 
> I've read (again) the code that you mentioned. I've changed my mind about it.
> Now I think that a flag like that could be useful if there are no other means to
> lock __start_tty() and __stop_tty() critical sections.

If ->flow.lock were held from only schedulable context, replacing this spinlock with
a mutex would be possible. But stop_tty() says "may be called from any context" which
means that we can't use a mutex...

Making do_con_write() no-op when called with IRQs disabled would be the minimal change
that can silence the syzbot. But this does not fix the regression for drivers/tty/n_hdlc.c
introduced by f9e053dcfc02b0ad.

--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2902,7 +2902,7 @@ static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int co
 	struct vt_notifier_param param;
 	bool rescan;
 
-	if (in_interrupt())
+	if (in_interrupt() || irqs_disabled())
 		return count;
 
 	console_lock();
@@ -3358,7 +3358,7 @@ static void con_flush_chars(struct tty_struct *tty)
 {
 	struct vc_data *vc;
 
-	if (in_interrupt())	/* from flush_to_ldisc */
+	if (in_interrupt() || irqs_disabled())	/* from flush_to_ldisc */
 		return;
 
 	/* if we race with con_close(), vt may be null */

According to scripts/get_maintainer.pl , Greg and Jiri are maintainers for the n_hdlc driver.
Greg and Jiri, what do you think? Is sacrificing ability to write to consoles when
n_hdlc_send_frames() is called from __start_tty() path considered tolerable? (Maybe
OK for now and stable kernels, for nobody was reporting this problem suggests that
nobody depends on this ability.)

But if we must fix the regression for drivers/tty/n_hdlc.c , we need to use something
like https://lkml.kernel.org/r/7d851c88-f657-dfd8-34ab-4891ac6388dc@xxxxxxxxxxxxxxxxxxx
in order to achieve what f9e053dcfc02b0ad meant. That is, extend tty->stopped in order
to be able to represent "started state (currently indicated by tty->stopped == false)",
"stopped state (currently indicated by tty->stopped == true)" and "changing state
(currently impossible due to bool)", but this approach might need to touch many locations,
and I worry that touching many locations introduces some oversight bugs.