tty/vt: fix sleeping function called from invalid context in do_con_write()

[PATCH v2] tty/vt: fix sleeping function called from invalid context in do_con_write()

Posted by Zeng Heng 2 years, 9 months ago

Here is a BUG report from syzkaller:

BUG: sleeping function called from invalid context at kernel/printk/printk.c:2565

3 locks held by mingetty/6405:
 #0: ffff8881109b7098 (&tty->ldisc_sem){++++}-{0:0},
	at: tty_ldisc_ref_wait+0x22/0x80
 #1: ffff8881109b7130 (&tty->atomic_write_lock){+.+.}-{3:3},
	at: file_tty_write.constprop.0+0x26f/0x8c0
 #2: ffff8880147293e0 (&gsm->tx_lock){....}-{2:2},
	at: gsmld_write+0x5e/0x140

Call Trace:
 __might_resched.cold+0x222/0x26b
 console_lock+0x17/0x80
 do_con_write+0x10f/0x1e30
 con_write+0x21/0x40
 gsmld_write+0xcb/0x140
 file_tty_write.constprop.0+0x471/0x8c0
 vfs_write+0x9ef/0xde0
 ksys_write+0x127/0x250
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

And another bug report caused by the same reason is shown as below:

BUG: spinlock wrong CPU on CPU#2, mingetty/30460
 lock: 0xffff8880340553c8, .magic: dead4ead, .owner: mingetty/30460, .owner_cpu: 1
Call Trace:
 dump_stack_lvl+0xcd/0x134
 do_raw_spin_unlock+0x1af/0x230
 _raw_spin_unlock_irqrestore+0x1e/0x70
 gsmld_write+0xde/0x140

In gsmld_write(), in case of race condition, it would fetch the spin-lock
and disable IRQ. But in the following trace, do_con_write() attempt to down
semaphore which would probably cause re-schedule task and in further, IRQs
woud be ignored for a quite time.

Add if condition in do_con_write(). When the current task is in atomic
context, return immediately.

Fixes: 32dd59f96924 ("tty: n_gsm: fix race condition in gsmld_write()")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/tty/vt/vt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 981d2bfcf9a5..7662b6eb0836 100644
--- 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 */
-- 
2.25.1

Re: [PATCH v2] tty/vt: fix sleeping function called from invalid context in do_con_write()

Posted by Jiri Slaby 2 years, 9 months ago

On 12. 12. 22, 3:35, Zeng Heng wrote:
> Here is a BUG report from syzkaller:
> 
> BUG: sleeping function called from invalid context at kernel/printk/printk.c:2565
> 
> 3 locks held by mingetty/6405:
>   #0: ffff8881109b7098 (&tty->ldisc_sem){++++}-{0:0},
> 	at: tty_ldisc_ref_wait+0x22/0x80
>   #1: ffff8881109b7130 (&tty->atomic_write_lock){+.+.}-{3:3},
> 	at: file_tty_write.constprop.0+0x26f/0x8c0
>   #2: ffff8880147293e0 (&gsm->tx_lock){....}-{2:2},
> 	at: gsmld_write+0x5e/0x140
> 
> Call Trace:
>   __might_resched.cold+0x222/0x26b
>   console_lock+0x17/0x80
>   do_con_write+0x10f/0x1e30
>   con_write+0x21/0x40
>   gsmld_write+0xcb/0x140
>   file_tty_write.constprop.0+0x471/0x8c0
>   vfs_write+0x9ef/0xde0
>   ksys_write+0x127/0x250
>   do_syscall_64+0x35/0x80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> And another bug report caused by the same reason is shown as below:
> 
> BUG: spinlock wrong CPU on CPU#2, mingetty/30460
>   lock: 0xffff8880340553c8, .magic: dead4ead, .owner: mingetty/30460, .owner_cpu: 1
> Call Trace:
>   dump_stack_lvl+0xcd/0x134
>   do_raw_spin_unlock+0x1af/0x230
>   _raw_spin_unlock_irqrestore+0x1e/0x70
>   gsmld_write+0xde/0x140
> 
> In gsmld_write(), in case of race condition, it would fetch the spin-lock
> and disable IRQ. But in the following trace, do_con_write() attempt to down
> semaphore which would probably cause re-schedule task and in further, IRQs
> woud be ignored for a quite time.

Wasn't this fixed in gsmld yet? In any case:

NACK

as it is wrong. This should be fixed in gsmld instead.

> Add if condition in do_con_write(). When the current task is in atomic
> context, return immediately.
> 
> Fixes: 32dd59f96924 ("tty: n_gsm: fix race condition in gsmld_write()")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>   drivers/tty/vt/vt.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 981d2bfcf9a5..7662b6eb0836 100644
> --- 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 */

-- 
js
suse labs