2-line function check_unthrottle() is now only called from
n_tty_read(); merge into caller.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 66ce178..8f9f665 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -195,21 +195,6 @@ static void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
}
/**
- * check_unthrottle - allow new receive data
- * @tty; tty device
- *
- * Check whether to call the driver unthrottle functions
- *
- * Can sleep, may be called under the atomic_read_lock mutex but
- * this is not guaranteed.
- */
-static void check_unthrottle(struct tty_struct *tty)
-{
- if (tty->count)
- tty_unthrottle(tty);
-}
-
-/**
* reset_buffer_flags - reset buffer state
* @tty: terminal to reset
*
@@ -1971,7 +1956,8 @@ do_it_again:
*/
if (n_tty_chars_in_buffer(tty) <= TTY_THRESHOLD_UNTHROTTLE) {
n_tty_set_room(tty);
- check_unthrottle(tty);
+ if (tty->count)
+ tty_unthrottle(tty);
}
if (b - buf >= minimum)
--
1.8.1.2
The tty driver can become stuck throttled due to race conditions
between throttle and unthrottle, when the decision to throttle
or unthrottle is conditional. The following example helps to
illustrate the race:
CPU 0 | CPU 1
|
if (condition A) |
| <processing such that A not true>
| if (!condition A)
| unthrottle()
throttle() |
|
Note the converse is also possible; ie.,
CPU 0 | CPU 1
|
| if (!condition A)
<processing such that A true> |
if (condition A) |
throttle() |
| unthrottle()
|
Add new throttle/unthrottle functions based on the familiar model
of task state and schedule/wake. For example,
while (1) {
tty_set_flow_change(tty, TTY_THROTTLE_SAFE);
if (!condition)
break;
if (!tty_throttle_safe(tty))
break;
}
__tty_set_flow_change(tty, 0);
In this example, if an unthrottle occurs after the condition is
evaluated but before tty_throttle_safe(), then tty_throttle_safe()
will return non-zero, looping and forcing the re-evaluation of
condition.
Reported-by: Vincent Pillet <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_ioctl.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/tty.h | 18 ++++++++++++++
2 files changed, 82 insertions(+)
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index d58b92c..132d452 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -106,6 +106,7 @@ void tty_throttle(struct tty_struct *tty)
if (!test_and_set_bit(TTY_THROTTLED, &tty->flags) &&
tty->ops->throttle)
tty->ops->throttle(tty);
+ tty->flow_change = 0;
mutex_unlock(&tty->termios_mutex);
}
EXPORT_SYMBOL(tty_throttle);
@@ -129,11 +130,74 @@ void tty_unthrottle(struct tty_struct *tty)
if (test_and_clear_bit(TTY_THROTTLED, &tty->flags) &&
tty->ops->unthrottle)
tty->ops->unthrottle(tty);
+ tty->flow_change = 0;
mutex_unlock(&tty->termios_mutex);
}
EXPORT_SYMBOL(tty_unthrottle);
/**
+ * tty_throttle_safe - flow control
+ * @tty: terminal
+ *
+ * Similar to tty_throttle() but will only attempt throttle
+ * if tty->flow_change is TTY_THROTTLE_SAFE. Prevents an accidental
+ * throttle due to race conditions when throttling is conditional
+ * on factors evaluated prior to throttling.
+ *
+ * Returns 0 if tty is throttled (or was already throttled)
+ */
+
+int tty_throttle_safe(struct tty_struct *tty)
+{
+ int ret = 0;
+
+ mutex_lock(&tty->termios_mutex);
+ if (!test_bit(TTY_THROTTLED, &tty->flags)) {
+ if (tty->flow_change != TTY_THROTTLE_SAFE)
+ ret = 1;
+ else {
+ __set_bit(TTY_THROTTLED, &tty->flags);
+ if (tty->ops->throttle)
+ tty->ops->throttle(tty);
+ }
+ }
+ mutex_unlock(&tty->termios_mutex);
+
+ return ret;
+}
+
+/**
+ * tty_unthrottle_safe - flow control
+ * @tty: terminal
+ *
+ * Similar to tty_unthrottle() but will only attempt unthrottle
+ * if tty->flow_change is TTY_UNTHROTTLE_SAFE. Prevents an accidental
+ * unthrottle due to race conditions when unthrottling is conditional
+ * on factors evaluated prior to unthrottling.
+ *
+ * Returns 0 if tty is unthrottled (or was already unthrottled)
+ */
+
+int tty_unthrottle_safe(struct tty_struct *tty)
+{
+ int ret = 0;
+
+ mutex_lock(&tty->termios_mutex);
+ if (test_bit(TTY_THROTTLED, &tty->flags)) {
+ if (tty->flow_change != TTY_UNTHROTTLE_SAFE)
+ ret = 1;
+ else {
+ __clear_bit(TTY_THROTTLED, &tty->flags);
+ if (tty->ops->unthrottle)
+ tty->ops->unthrottle(tty);
+ }
+ }
+ mutex_unlock(&tty->termios_mutex);
+
+ return ret;
+}
+
+/**
* tty_wait_until_sent - wait for I/O to finish
* @tty: tty we are waiting for
* @timeout: how long we will wait
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 2c109a3..e7a1bad 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -258,6 +258,7 @@ struct tty_struct {
unsigned char warned:1;
unsigned char ctrl_status; /* ctrl_lock */
unsigned int receive_room; /* Bytes free for queue */
+ int flow_change;
struct tty_struct *link;
struct fasync_struct *fasync;
@@ -317,6 +318,21 @@ struct tty_file_private {
#define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
+/* Values for tty->flow_change */
+#define TTY_THROTTLE_SAFE 1
+#define TTY_UNTHROTTLE_SAFE 2
+
+static inline void __tty_set_flow_change(struct tty_struct *tty, int val)
+{
+ tty->flow_change = val;
+}
+
+static inline void tty_set_flow_change(struct tty_struct *tty, int val)
+{
+ tty->flow_change = val;
+ smp_mb();
+}
+
#ifdef CONFIG_TTY
extern void console_init(void);
extern void tty_kref_put(struct tty_struct *tty);
@@ -399,6 +415,8 @@ extern int tty_write_room(struct tty_struct *tty);
extern void tty_driver_flush_buffer(struct tty_struct *tty);
extern void tty_throttle(struct tty_struct *tty);
extern void tty_unthrottle(struct tty_struct *tty);
+extern int tty_throttle_safe(struct tty_struct *tty);
+extern int tty_unthrottle_safe(struct tty_struct *tty);
extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
extern void tty_driver_remove_tty(struct tty_driver *driver,
struct tty_struct *tty);
--
1.8.1.2
As noted in the following comment:
/* FIXME: there is a tiny race here if the receive room check runs
before the other work executes and empties the buffer (upping
the receiving room and unthrottling. We then throttle and get
stuck. This has been observed and traced down by Vincent Pillet/
We need to address this when we sort out out the rx path locking */
Use new safe throttle/unthrottle functions to re-evaluate conditions
if interrupted by the complement flow control function.
Reported-by: Vincent Pillet <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 8f9f665..96d11ec 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1474,14 +1474,14 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
* mode. We don't want to throttle the driver if we're in
* canonical mode and don't have a newline yet!
*/
- if (tty->receive_room < TTY_THRESHOLD_THROTTLE)
- tty_throttle(tty);
-
- /* FIXME: there is a tiny race here if the receive room check runs
- before the other work executes and empties the buffer (upping
- the receiving room and unthrottling. We then throttle and get
- stuck. This has been observed and traced down by Vincent Pillet/
- We need to address this when we sort out out the rx path locking */
+ while (1) {
+ tty_set_flow_change(tty, TTY_THROTTLE_SAFE);
+ if (tty->receive_room >= TTY_THRESHOLD_THROTTLE)
+ break;
+ if (!tty_throttle_safe(tty))
+ break;
+ }
+ __tty_set_flow_change(tty, 0);
}
int is_ignored(int sig)
@@ -1954,11 +1954,17 @@ do_it_again:
* longer than TTY_THRESHOLD_UNTHROTTLE in canonical mode,
* we won't get any more characters.
*/
- if (n_tty_chars_in_buffer(tty) <= TTY_THRESHOLD_UNTHROTTLE) {
+ while (1) {
+ tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
+ if (n_tty_chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
+ break;
+ if (!tty->count)
+ break;
n_tty_set_room(tty);
- if (tty->count)
- tty_unthrottle(tty);
+ if (!tty_unthrottle_safe(tty))
+ break;
}
+ __tty_set_flow_change(tty, 0);
if (b - buf >= minimum)
break;
--
1.8.1.2
tty->flags needs to be atomically modified.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_ioctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 2febed5..ce67f2c 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -156,7 +156,7 @@ int tty_throttle_safe(struct tty_struct *tty)
if (tty->flow_change != TTY_THROTTLE_SAFE)
ret = 1;
else {
- __set_bit(TTY_THROTTLED, &tty->flags);
+ set_bit(TTY_THROTTLED, &tty->flags);
if (tty->ops->throttle)
tty->ops->throttle(tty);
}
@@ -187,7 +187,7 @@ int tty_unthrottle_safe(struct tty_struct *tty)
if (tty->flow_change != TTY_UNTHROTTLE_SAFE)
ret = 1;
else {
- __clear_bit(TTY_THROTTLED, &tty->flags);
+ clear_bit(TTY_THROTTLED, &tty->flags);
if (tty->ops->unthrottle)
tty->ops->unthrottle(tty);
}
--
1.8.1.2