tty: WARNING in n_hdlc_tty_read

tty: WARNING in n_hdlc_tty_read

[Dmitry Vyukov]

Hello,

I am hitting the following WARNING on commit
aa53685549a2cfb5f175b0c4a20bc9aa1e5a1b85 (Dec 8):

------------[ cut here ]------------
WARNING: CPU: 2 PID: 24931 at kernel/sched/core.c:7557
__might_sleep+0x138/0x1a0()
do not call blocking ops when !TASK_RUNNING; state=1 set at
[<ffffffff82bf3066>] n_hdlc_tty_read+0x246/0x670
drivers/tty/n_hdlc.c:607
Modules linked in:
CPU: 2 PID: 24931 Comm: syzkaller_execu Not tainted 4.4.0-rc4+ #160
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff880065297928 ffffffff82899b0d ffff880065297998
 ffff880066bc0000 ffffffff85c3bf40 ffff880065297968 ffffffff812ebbb9
 ffffffff813620c8 ffffed000ca52f2f ffffffff85c3bf40 0000000000001d85
Call Trace:
 [<ffffffff812ebcc9>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:472
 [<ffffffff813620c8>] __might_sleep+0x138/0x1a0 kernel/sched/core.c:7552
 [<ffffffff8165e55f>] __might_fault+0xaf/0x1d0 mm/memory.c:3794
 [<     inline     >] copy_to_user ./arch/x86/include/asm/uaccess.h:732
 [<ffffffff82bf3355>] n_hdlc_tty_read+0x535/0x670 drivers/tty/n_hdlc.c:618
 [<ffffffff82bcb345>] tty_read+0x165/0x250 drivers/tty/tty_io.c:1071
 [<ffffffff8170f2b1>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:680
 [<ffffffff81711e18>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:810
 [<ffffffff81711f83>] vfs_readv+0x83/0xb0 fs/read_write.c:834
 [<     inline     >] SYSC_readv fs/read_write.c:860
 [<ffffffff81714861>] SyS_readv+0x111/0x2b0 fs/read_write.c:852
 [<ffffffff85b67576>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
---[ end trace 4ace39d7f6cc3bf1 ]---


Seems to be introduced by:
commit 1035b63d3c6fc34a9b8c4a181366eec6d6158b31
Author: Paul Fulghum
Date:   Tue Jan 18 14:03:25 2011 -0800
    n_hdlc: fix read and write locking

Re: tty: WARNING in n_hdlc_tty_read

[Alan Cox]

On Fri, 11 Dec 2015 14:44:00 +0100
Dmitry Vyukov <dvyukov@google.com> wrote:

> Hello,
> 
> I am hitting the following WARNING on commit
> aa53685549a2cfb5f175b0c4a20bc9aa1e5a1b85 (Dec 8):

What surprises me is that the warnings didn't trigger for whoever
tested the code.

It also looks to me like the claim in the commit that

" 1. neither BKL or tty mutex are required for correct operation"

is probably not entirely true (though close).

Consider the case where two reads occur in parallel. In that situation
nothing protects hdlc->rx_free_buf_list.count during the decision whether
to kfree or n_hdlc_buf_put.

The more specific problem you are hitting I think those is the attempt to
make use of set_current_state and add_wait_queue directly.

After we set the task to "uninterruptible sleep" we dequeue a frame which
is fine and correctly locked, but then call copy_to_user which can block.
No can do.


My first thought would be to rip out all the raw wait queue messing about
and replace the lot with something slightly more 21st century. Something
a bit like


	if (file->f_flags & O_NONBLOCK)
		rbuf = n_hdlc_get(...)
	else
		wait_event_interruptible(&tty->read_wait, (rbuf =
			n_hdlc_get(&n_hdlc->rx_buf_list)) != NULL);

	if (rbuf)
		...

	if (signal_pending(current))
		...

ought to get close - but that doesn't fit the list.count race.

Alan