[PATCH 2/2] n_tty: Access echo_* variables carefully.

[Tetsuo Handa]

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

Signed-off-by: Tetsuo Handa 
Reported-by: syzbot 
Cc: Peter Hurley 
---
 drivers/tty/n_tty.c | 42 --
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index b279f873..4317422 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -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,13 +618,20 @@ 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
 * op code or a control character value.
@@ -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(>output_lock);
head = ldata->echo_head;
ldata->echo_mark = head;
old = 

[PATCH 2/2] n_tty: Access echo_* variables carefully.

[Tetsuo Handa]

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

Signed-off-by: Tetsuo Handa 
Reported-by: syzbot 
Cc: Peter Hurley 
---
 drivers/tty/n_tty.c | 42 --
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index b279f873..4317422 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -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,13 +618,20 @@ 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
 * op code or a control character value.
@@ -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(>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