Messages in this thread | | | From | "Fabio M. De Francesco" <> | Subject | Re: [PATCH] vt: Fix sleeping functions called from atomic context | Date | Wed, 17 Nov 2021 08:02:42 +0100 |
| |
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
| |