patch "n_tty: Access echo_* variables carefully." added to tty-linus
Subsystems (2)
This is a note to let you know that I've just added the patch titled
n_tty: Access echo_* variables carefully.
to my tty git tree which can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
in the tty-linus branch.
The patch will show up in the next release of the linux-next tree (usually sometime within the next 24 hours during the week.)
The patch will hopefully also be merged in Linus's tree for the next -rc kernel release.
If you have any questions about this process, please let me know.
Quoted 1 line
From
ebec3f8f5271139df618ebdf8427e24ba102ba94Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[email protected]>
Date: Sat, 26 May 2018 09:53:14 +0900
Subject: n_tty: Access echo_* variables carefully.
syzbot is reporting stalls at __process_echoes() [1]. This is because since ldata->echo_commit < ldata->echo_tail becomes true for some reason, the discard loop is serving as almost infinite loop. This patch tries to avoid falling into ldata->echo_commit < ldata->echo_tail situation by making access to echo_* variables more carefully.
Since reset_buffer_flags() is called without output_lock held, it should not touch echo_* variables. And omit a call to reset_buffer_flags() from n_tty_open() by using vzalloc().
Since add_echo_byte() is called without output_lock held, it needs memory barrier between storing into echo_buf[] and incrementing echo_head counter. echo_buf() needs corresponding memory barrier before reading echo_buf[]. Lack of handling the possibility of not-yet-stored multi-byte operation might be the reason of falling into ldata->echo_commit < ldata->echo_tail situation, for if I do WARN_ON(ldata->echo_commit == tail + 1) prior to echo_buf(ldata, tail + 1), the WARN_ON() fires.
Also, explicitly masking with buffer for the former "while" loop, and use ldata->echo_commit > tail for the latter "while" loop.
[1] https://syzkaller.appspot.com/bug?id=17f23b094cd80df750e5b0f8982c521ee6bcbf40
--- drivers/tty/n_tty.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-)
drivers/tty/n_tty.c +24 -18
@@ -143,6 +143,7 @@ static inline unsigned char *read_buf_addr(struct n_tty_data *ldata, size_t i)static inline unsigned char echo_buf (struct n_tty_data *ldata,size_t i) { +smp_rmb ();/* Matches smp_wmb() in add_echo_byte(). */ return ldata->echo_buf[i & (N_TTY_BUF_SIZE -1 )]; }
@@ -318,9 +319,7 @@ static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)static void reset_buffer_flags (struct n_tty_data *ldata) { ldata->read_head = ldata->canon_head = ldata->read_tail =0 ; - ldata->echo_head = ldata->echo_tail = ldata->echo_commit =0 ; ldata->commit_head =0 ; - ldata->echo_mark =0 ; ldata->line_start =0 ; ldata->erasing =0 ;
@@ -619,12 +618,19 @@ static size_t __process_echoes(struct tty_struct *tty) old_space = space =tty_write_room (tty); tail = ldata->echo_tail; -while (ldata->echo_commit != tail) { +while (MASK (ldata->echo_commit) !=MASK (tail)) { c =echo_buf (ldata, tail);if (c ==ECHO_OP_START ) {unsigned char op;int no_space_left =0 ; +/* +* Since add_echo_byte() is called without holding +* output_lock, we might see only portion of multi-byte +* operation. +*/ +if (MASK (ldata->echo_commit) ==MASK (tail +1 )) + goto not_yet_stored;/* * If the buffer byte is the start of a multi-byte * operation, get the next byte, which is either the
@@ -636,6 +642,8 @@ static size_t __process_echoes(struct tty_struct *tty)unsigned int num_chars, num_bs;case ECHO_OP_ERASE_TAB : +if (MASK (ldata->echo_commit) ==MASK (tail +2 )) + goto not_yet_stored; num_chars =echo_buf (ldata, tail +2 );/*
@@ -730,7 +738,8 @@ static size_t __process_echoes(struct tty_struct *tty)/* If the echo buffer is nearly full (so that the possibility exists * of echo overrun before the next commit), then discard enough * data at the tail to prevent a subsequent overrun */ -while (ldata->echo_commit - tail >=ECHO_DISCARD_WATERMARK ) { +while (ldata->echo_commit > tail && + ldata->echo_commit - tail >=ECHO_DISCARD_WATERMARK ) {if (echo_buf (ldata, tail) ==ECHO_OP_START ) {if (echo_buf (ldata, tail +1 ) ==ECHO_OP_ERASE_TAB ) tail +=3 ;
@@ -740,6 +749,7 @@ static size_t __process_echoes(struct tty_struct *tty) tail++; } + not_yet_stored: ldata->echo_tail = tail;return old_space - space; }
@@ -750,6 +760,7 @@ static void commit_echoes(struct tty_struct *tty)size_t nr, old, echoed;size_t head; +mutex_lock (&ldata->output_lock); head = ldata->echo_head; ldata->echo_mark = head; old = ldata->echo_commit - ldata->echo_tail;
@@ -758,10 +769,12 @@ static void commit_echoes(struct tty_struct *tty)* is over the threshold (and try again each time another * block is accumulated) */ nr = head - ldata->echo_tail; -if (nr <ECHO_COMMIT_WATERMARK || (nr %ECHO_BLOCK > old %ECHO_BLOCK )) +if (nr <ECHO_COMMIT_WATERMARK || + (nr %ECHO_BLOCK > old %ECHO_BLOCK )) { +mutex_unlock (&ldata->output_lock);return ; + } -mutex_lock (&ldata->output_lock); ldata->echo_commit = head; echoed =__process_echoes (tty);mutex_unlock (&ldata->output_lock);
@@ -812,7 +825,9 @@ static void flush_echoes(struct tty_struct *tty)static inline void add_echo_byte (unsigned char c,struct n_tty_data *ldata) { - *echo_buf_addr (ldata, ldata->echo_head++) = c; + *echo_buf_addr (ldata, ldata->echo_head) = c; +smp_wmb ();/* Matches smp_rmb() in echo_buf(). */ + ldata->echo_head++; }/**
@@ -1881,30 +1896,21 @@ static int n_tty_open(struct tty_struct *tty)struct n_tty_data *ldata;/* Currently a malloc failure here can panic */ - ldata =vmalloc (sizeof (*ldata)); + ldata =vzalloc (sizeof (*ldata));if (!ldata) - goto err; +return -ENOMEM ; ldata->overrun_time = jiffies;mutex_init (&ldata->atomic_read_lock);mutex_init (&ldata->output_lock); tty->disc_data = ldata; -reset_buffer_flags (tty->disc_data); - ldata->column =0 ; - ldata->canon_column =0 ; - ldata->num_overrun =0 ; - ldata->no_room =0 ; - ldata->lnext =0 ; tty->closing =0 ;/* indicate buffer work may resume */ clear_bit (TTY_LDISC_HALTED , &tty->flags);n_tty_set_termios (tty,NULL );tty_unthrottle (tty); -return 0 ; -err: -return -ENOMEM ; }static inline int input_available_p (struct tty_struct *tty,int poll)
Signature · 1 line
2.18.0