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

From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com,
	Marco Elver <elver@google.com>, Jiri Slaby <jirislaby@kernel.org>,
	Max Filippov <jcmvbkbc@gmail.com>,
	David Sterba <dsterba@suse.com>,
	Bhaskar Chowdhury <unixbhaskar@gmail.com>,
	Igor Matheus Andrade Torrente <igormtorrente@gmail.com>,
	nick black <dankamongmen@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vt: Fix sleeping functions called from atomic context
Date: Wed, 17 Nov 2021 08:02:42 +0100	[thread overview]
Message-ID: <2063032.8CZl0nWAlc@localhost.localdomain> (raw)
In-Reply-To: <f68adf33-4a98-ac4b-fe07-ac4909eeefff@i-love.sakura.ne.jp>

On Wednesday, November 17, 2021 2:55:44 AM CET Tetsuo Handa wrote:
> On 2021/11/16 23:49, Fabio M. De Francesco wrote:
> > -	if (in_interrupt())
> > +	if (!preemptible())
> >  		return count;
> 
> preemptible() is "an unconditional 0" if CONFIG_PREEMPT_COUNT=n .
> Is preemptible() really what we want? ;-) 
> 
Greg K-H made me notice that Linux has had this code for many years and no 
one has ever found problems with it, therefore, whatever triggered Syzbot 
must be recent changes up in the calls chain.

This reported bug must be better investigated because there is a high 
probability that the "real" issue is somewhere else.

Unfortunately, as I replied to Greg, I know very little of tty/vt so I'll 
take time to understand the relevant documentation and the related code.

For instance, I don't know why we need to disable IRQs with spin_lock_irq() 
up in the call chain in n_tty_ioctl_helper(). Would a "normal" spin_lock() be 
enough? Otherwise, what about spin_lock_bh()?

Do we really need to disable IRQs?

Aside from the reasoning reported above, for a moment let's speculate on the 
remote possibility that this bugs are in do_con_write() and 
con_flush_chars()...

When I asked Syzbot to run a test on my diff, the code was different than 
what you see in this patch. 

Marco Elver had proposed this "if (!preemptible()) return <something>;". I 
didn't even know of that macro, so I proposed to open code the equivalent 
implementation that Linux uses if CONFIG_PREEMPT_COUNT=y.

The end result, applying De Morgan's law on his proposal, was "if 
((preempt_count() || irqs_disabled()) return <something>;".

Now, out of curiosity, if Greg's argument (with which I agree in full) was 
not brought to my attention, what would happen by using that test if we have 
kernels compiled with CONFIG_PREEMPT_COUNT=n (as it is in your example)?

I'd appreciate if you have time and want to answer my last question and 
(possibly) also the one about using spin_lock() or spin_lock_bh() in 
n_tty_ioctl_helper().

@Greg, the same two questions are for you, if you have time and want to  
kindly help :)

Thanks to you all,

Fabio M. De Francesco