tty: throttling race fix

The tty throttling code can race due to the lock drops. It takes very high loads but this has been observed and verified by Rob Duncan. The basic problem is that on an SMP box we can go CPU #1 CPU #2 need to throttle ? suppose we should buffer space cleared are we throttled yes ? - unthrottle call throttle method This changeet take the termios lock to protect against this. The termios lock isn't the initial obvious candidate but many implementations of throttle methods already need to poke around their own termios structures (and nobody really locks them against a racing change of flow control). This does mean that anyone who is setting tty->low_latency = 1 and then calling tty_flip_buffer_push from their unthrottle method is going to end up collapsing in a pile of locks. However we've removed all the known bogus users of low_latency = 1 and such use isn't safe anyway for other reasons so catching it would be an improvement. Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

tty: throttling race fix
The tty throttling code can race due to the lock drops. It takes very high loads but this has been observed and verified by Rob Duncan. The basic problem is that on an SMP box we can go CPU #1 CPU #2 need to throttle ? suppose we should buffer space cleared are we throttled yes ? - unthrottle call throttle method This changeet take the termios lock to protect against this. The termios lock isn't the initial obvious candidate but many implementations of throttle methods already need to poke around their own termios structures (and nobody really locks them against a racing change of flow control). This does mean that anyone who is setting tty->low_latency = 1 and then calling tty_flip_buffer_push from their unthrottle method is going to end up collapsing in a pile of locks. However we've removed all the known bogus users of low_latency = 1 and such use isn't safe anyway for other reasons so catching it would be an improvement. Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Alan Cox · Linus Torvalds
1 parent 5b0ed5263c
Showing 2 changed files with 17 additions and 2 deletions Side-by-side Diff
drivers/char/tty_ioctl.c
include/linux/tty_driver.h
@@ -97,14 +97,19 @@
  *	@tty: terminal
  *
  *	Indicate that a tty should stop transmitting data down the stack.
+ *	Takes the termios mutex to protect against parallel throttle/unthrottle
+ *	and also to ensure the driver can consistently reference its own
+ *	termios data at this point when implementing software flow control.
  */
  
 void tty_throttle(struct tty_struct *tty)
 {
+	mutex_lock(&tty->termios_mutex);
 	/* check TTY_THROTTLED first so it indicates our state */
 	if (!test_and_set_bit(TTY_THROTTLED, &tty->flags) &&
 	    tty->ops->throttle)
 		tty->ops->throttle(tty);
+	mutex_unlock(&tty->termios_mutex);
 }
 EXPORT_SYMBOL(tty_throttle);
  
  
  
@@ -113,13 +118,21 @@
  *	@tty: terminal
  *
  *	Indicate that a tty may continue transmitting data down the stack.
+ *	Takes the termios mutex to protect against parallel throttle/unthrottle
+ *	and also to ensure the driver can consistently reference its own
+ *	termios data at this point when implementing software flow control.
+ *
+ *	Drivers should however remember that the stack can issue a throttle,
+ *	then change flow control method, then unthrottle.
  */
  
 void tty_unthrottle(struct tty_struct *tty)
 {
+	mutex_lock(&tty->termios_mutex);
 	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags) &&
 	    tty->ops->unthrottle)
 		tty->ops->unthrottle(tty);
+	mutex_unlock(&tty->termios_mutex);
 }
 EXPORT_SYMBOL(tty_unthrottle);
  
@@ -127,7 +127,8 @@
  * 	the line discipline are close to full, and it should somehow
  * 	signal that no more characters should be sent to the tty.
  *
- *	Optional: Always invoke via tty_throttle();
+ *	Optional: Always invoke via tty_throttle(), called under the
+ *	termios lock.
  * 
  * void (*unthrottle)(struct tty_struct * tty);
  *
@@ -135,7 +136,8 @@
  * 	that characters can now be sent to the tty without fear of
  * 	overrunning the input buffers of the line disciplines.
  * 
- *	Optional: Always invoke via tty_unthrottle();
+ *	Optional: Always invoke via tty_unthrottle(), called under the
+ *	termios lock.
  *
  * void (*stop)(struct tty_struct *tty);
  *
...	...	@@ -97,14 +97,19 @@
97	97	* @tty: terminal
98	98	*
99	99	* Indicate that a tty should stop transmitting data down the stack.
	100	+ * Takes the termios mutex to protect against parallel throttle/unthrottle
	101	+ * and also to ensure the driver can consistently reference its own
	102	+ * termios data at this point when implementing software flow control.
100	103	*/
101	104
102	105	void tty_throttle(struct tty_struct *tty)
103	106	{
	107	+ mutex_lock(&tty->termios_mutex);
104	108	/* check TTY_THROTTLED first so it indicates our state */
105	109	if (!test_and_set_bit(TTY_THROTTLED, &tty->flags) &&
106	110	tty->ops->throttle)
107	111	tty->ops->throttle(tty);
	112	+ mutex_unlock(&tty->termios_mutex);
108	113	}
109	114	EXPORT_SYMBOL(tty_throttle);
110	115
111	116
112	117
...	...	@@ -113,13 +118,21 @@
113	118	* @tty: terminal
114	119	*
115	120	* Indicate that a tty may continue transmitting data down the stack.
	121	+ * Takes the termios mutex to protect against parallel throttle/unthrottle
	122	+ * and also to ensure the driver can consistently reference its own
	123	+ * termios data at this point when implementing software flow control.
	124	+ *
	125	+ * Drivers should however remember that the stack can issue a throttle,
	126	+ * then change flow control method, then unthrottle.
116	127	*/
117	128
118	129	void tty_unthrottle(struct tty_struct *tty)
119	130	{
	131	+ mutex_lock(&tty->termios_mutex);
120	132	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags) &&
121	133	tty->ops->unthrottle)
122	134	tty->ops->unthrottle(tty);
	135	+ mutex_unlock(&tty->termios_mutex);
123	136	}
124	137	EXPORT_SYMBOL(tty_unthrottle);
125	138
...	...	@@ -127,7 +127,8 @@
127	127	* the line discipline are close to full, and it should somehow
128	128	* signal that no more characters should be sent to the tty.
129	129	*
130		- * Optional: Always invoke via tty_throttle();
	130	+ * Optional: Always invoke via tty_throttle(), called under the
	131	+ * termios lock.
131	132	*
132	133	* void (unthrottle)(struct tty_struct tty);
133	134	*
...	...	@@ -135,7 +136,8 @@
135	136	* that characters can now be sent to the tty without fear of
136	137	* overrunning the input buffers of the line disciplines.
137	138	*
138		- * Optional: Always invoke via tty_unthrottle();
	139	+ * Optional: Always invoke via tty_unthrottle(), called under the
	140	+ * termios lock.
139	141	*
140	142	* void (stop)(struct tty_struct tty);
141	143	*