Fix OpenSSH pty regression on close

commit 0f40fbbcc34e093255a2b2d70b6b0fb48c3f39aa upstream. OpenSSH expects the (non-blocking) read() of pty master to return EAGAIN only if it has received all of the slave-side output after it has received SIGCHLD. This used to work on pre-3.12 kernels. This fix effectively forces non-blocking read() and poll() to block for parallel i/o to complete for all ttys. It also unwinds these changes: 1) f8747d4a466ab2cafe56112c51b3379f9fdb7a12 tty: Fix pty master read() after slave closes 2) 52bce7f8d4fc633c9a9d0646eef58ba6ae9a3b73 pty, n_tty: Simplify input processing on final close 3) 1a48632ffed61352a7810ce089dc5a8bcd505a60 pty: Fix input race when closing Inspired by analysis and patch from Marc Aurele La France <tsi@tuyoix.net> Reported-by: Volth <openssh@volth.com> Reported-by: Marc Aurele La France <tsi@tuyoix.net> BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=52 BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=2492 Signed-off-by: Brian Bloniarz <brian.bloniarz@gmail.com> Reviewed-by: Peter Hurley <peter@hurleysoftware.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Fix OpenSSH pty regression on close
commit 0f40fbbcc34e093255a2b2d70b6b0fb48c3f39aa upstream. OpenSSH expects the (non-blocking) read() of pty master to return EAGAIN only if it has received all of the slave-side output after it has received SIGCHLD. This used to work on pre-3.12 kernels. This fix effectively forces non-blocking read() and poll() to block for parallel i/o to complete for all ttys. It also unwinds these changes: 1) f8747d4a466ab2cafe56112c51b3379f9fdb7a12 tty: Fix pty master read() after slave closes 2) 52bce7f8d4fc633c9a9d0646eef58ba6ae9a3b73 pty, n_tty: Simplify input processing on final close 3) 1a48632ffed61352a7810ce089dc5a8bcd505a60 pty: Fix input race when closing Inspired by analysis and patch from Marc Aurele La France <tsi@tuyoix.net> Reported-by: Volth <openssh@volth.com> Reported-by: Marc Aurele La France <tsi@tuyoix.net> BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=52 BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=2492 Signed-off-by: Brian Bloniarz <brian.bloniarz@gmail.com> Reviewed-by: Peter Hurley <peter@hurleysoftware.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Brian Bloniarz · Greg Kroah-Hartman
1 parent eb57884803
Showing 6 changed files with 43 additions and 74 deletions Side-by-side Diff
Documentation/serial/tty.txt
drivers/tty/n_hdlc.c
drivers/tty/n_tty.c
drivers/tty/pty.c
drivers/tty/tty_buffer.c
include/linux/tty.h
@@ -213,9 +213,6 @@
  
 TTY_OTHER_CLOSED	Device is a pty and the other side has closed.
  
-TTY_OTHER_DONE		Device is a pty and the other side has closed and
-			all pending input processing has been completed.
-
 TTY_NO_WRITE_SPLIT	Prevent driver from splitting up writes into
 			smaller chunks.
  
@@ -600,7 +600,7 @@
 	add_wait_queue(&tty->read_wait, &wait);
  
 	for (;;) {
-		if (test_bit(TTY_OTHER_DONE, &tty->flags)) {
+		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
 			ret = -EIO;
 			break;
 		}
@@ -828,7 +828,7 @@
 		/* set bits for operations that won't block */
 		if (n_hdlc->rx_buf_list.head)
 			mask |= POLLIN | POLLRDNORM;	/* readable */
-		if (test_bit(TTY_OTHER_DONE, &tty->flags))
+		if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
 			mask |= POLLHUP;
 		if (tty_hung_up_p(filp))
 			mask |= POLLHUP;
@@ -1955,18 +1955,6 @@
 		return ldata->commit_head - ldata->read_tail >= amt;
 }
  
-static inline int check_other_done(struct tty_struct *tty)
-{
-	int done = test_bit(TTY_OTHER_DONE, &tty->flags);
-	if (done) {
-		/* paired with cmpxchg() in check_other_closed(); ensures
-		 * read buffer head index is not stale
-		 */
-		smp_mb__after_atomic();
-	}
-	return done;
-}
-
 /**
  *	copy_from_read_buf	-	copy read data directly
  *	@tty: terminal device
@@ -2171,7 +2159,7 @@
 	struct n_tty_data *ldata = tty->disc_data;
 	unsigned char __user *b = buf;
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
-	int c, done;
+	int c;
 	int minimum, time;
 	ssize_t retval = 0;
 	long timeout;
  
  
  
  
@@ -2239,32 +2227,35 @@
 		    ((minimum - (b - buf)) >= 1))
 			ldata->minimum_to_wake = (minimum - (b - buf));
  
-		done = check_other_done(tty);
-
 		if (!input_available_p(tty, 0)) {
-			if (done) {
-				retval = -EIO;
-				break;
-			}
-			if (tty_hung_up_p(file))
-				break;
-			if (!timeout)
-				break;
-			if (file->f_flags & O_NONBLOCK) {
-				retval = -EAGAIN;
-				break;
-			}
-			if (signal_pending(current)) {
-				retval = -ERESTARTSYS;
-				break;
-			}
 			up_read(&tty->termios_rwsem);
+			tty_buffer_flush_work(tty->port);
+			down_read(&tty->termios_rwsem);
+			if (!input_available_p(tty, 0)) {
+				if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+					retval = -EIO;
+					break;
+				}
+				if (tty_hung_up_p(file))
+					break;
+				if (!timeout)
+					break;
+				if (file->f_flags & O_NONBLOCK) {
+					retval = -EAGAIN;
+					break;
+				}
+				if (signal_pending(current)) {
+					retval = -ERESTARTSYS;
+					break;
+				}
+				up_read(&tty->termios_rwsem);
  
-			timeout = wait_woken(&wait, TASK_INTERRUPTIBLE,
-					     timeout);
+				timeout = wait_woken(&wait, TASK_INTERRUPTIBLE,
+						timeout);
  
-			down_read(&tty->termios_rwsem);
-			continue;
+				down_read(&tty->termios_rwsem);
+				continue;
+			}
 		}
  
 		if (ldata->icanon && !L_EXTPROC(tty)) {
  
  
@@ -2446,12 +2437,17 @@
  
 	poll_wait(file, &tty->read_wait, wait);
 	poll_wait(file, &tty->write_wait, wait);
-	if (check_other_done(tty))
-		mask |= POLLHUP;
 	if (input_available_p(tty, 1))
 		mask |= POLLIN | POLLRDNORM;
+	else {
+		tty_buffer_flush_work(tty->port);
+		if (input_available_p(tty, 1))
+			mask |= POLLIN | POLLRDNORM;
+	}
 	if (tty->packet && tty->link->ctrl_status)
 		mask |= POLLPRI | POLLIN | POLLRDNORM;
+	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
+		mask |= POLLHUP;
 	if (tty_hung_up_p(file))
 		mask |= POLLHUP;
 	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
@@ -59,7 +59,7 @@
 	if (!tty->link)
 		return;
 	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
-	tty_flip_buffer_push(tty->link->port);
+	wake_up_interruptible(&tty->link->read_wait);
 	wake_up_interruptible(&tty->link->write_wait);
 	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		set_bit(TTY_OTHER_CLOSED, &tty->flags);
  
@@ -247,9 +247,7 @@
 		goto out;
  
 	clear_bit(TTY_IO_ERROR, &tty->flags);
-	/* TTY_OTHER_CLOSED must be cleared before TTY_OTHER_DONE */
 	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
-	clear_bit(TTY_OTHER_DONE, &tty->link->flags);
 	set_bit(TTY_THROTTLED, &tty->flags);
 	return 0;
  
@@ -37,29 +37,6 @@
  
 #define TTY_BUFFER_PAGE	(((PAGE_SIZE - sizeof(struct tty_buffer)) / 2) & ~0xFF)
  
-/*
- * If all tty flip buffers have been processed by flush_to_ldisc() or
- * dropped by tty_buffer_flush(), check if the linked pty has been closed.
- * If so, wake the reader/poll to process
- */
-static inline void check_other_closed(struct tty_struct *tty)
-{
-	unsigned long flags, old;
-
-	/* transition from TTY_OTHER_CLOSED => TTY_OTHER_DONE must be atomic */
-	for (flags = ACCESS_ONCE(tty->flags);
-	     test_bit(TTY_OTHER_CLOSED, &flags);
-	     ) {
-		old = flags;
-		__set_bit(TTY_OTHER_DONE, &flags);
-		flags = cmpxchg(&tty->flags, old, flags);
-		if (old == flags) {
-			wake_up_interruptible(&tty->read_wait);
-			break;
-		}
-	}
-}
-
 /**
  *	tty_buffer_lock_exclusive	-	gain exclusive access to buffer
  *	tty_buffer_unlock_exclusive	-	release exclusive access
@@ -254,8 +231,6 @@
 	if (ld && ld->ops->flush_buffer)
 		ld->ops->flush_buffer(tty);
  
-	check_other_closed(tty);
-
 	atomic_dec(&buf->priority);
 	mutex_unlock(&buf->lock);
 }
  
@@ -505,10 +480,8 @@
 		 */
 		count = smp_load_acquire(&head->commit) - head->read;
 		if (!count) {
-			if (next == NULL) {
-				check_other_closed(tty);
+			if (next == NULL)
 				break;
-			}
 			buf->head = next;
 			tty_buffer_free(port, head);
 			continue;
@@ -596,5 +569,10 @@
 bool tty_buffer_cancel_work(struct tty_port *port)
 {
 	return cancel_work_sync(&port->buf.work);
+}
+
+void tty_buffer_flush_work(struct tty_port *port)
+{
+	flush_work(&port->buf.work);
 }
@@ -338,7 +338,6 @@
 #define TTY_EXCLUSIVE 		3	/* Exclusive open mode */
 #define TTY_DEBUG 		4	/* Debugging */
 #define TTY_DO_WRITE_WAKEUP 	5	/* Call write_wakeup after queuing new */
-#define TTY_OTHER_DONE		6	/* Closed pty has completed input processing */
 #define TTY_LDISC_OPEN	 	11	/* Line discipline is open */
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
@@ -469,6 +468,7 @@
 extern void tty_buffer_set_lock_subclass(struct tty_port *port);
 extern bool tty_buffer_restart_work(struct tty_port *port);
 extern bool tty_buffer_cancel_work(struct tty_port *port);
+extern void tty_buffer_flush_work(struct tty_port *port);
 extern speed_t tty_termios_baud_rate(struct ktermios *termios);
 extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
 extern void tty_termios_encode_baud_rate(struct ktermios *termios,
...	...	@@ -213,9 +213,6 @@
213	213
214	214	TTY_OTHER_CLOSED Device is a pty and the other side has closed.
215	215
216		-TTY_OTHER_DONE Device is a pty and the other side has closed and
217		- all pending input processing has been completed.
218		-
219	216	TTY_NO_WRITE_SPLIT Prevent driver from splitting up writes into
220	217	smaller chunks.
221	218
...	...	@@ -600,7 +600,7 @@
600	600	add_wait_queue(&tty->read_wait, &wait);
601	601
602	602	for (;;) {
603		- if (test_bit(TTY_OTHER_DONE, &tty->flags)) {
	603	+ if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
604	604	ret = -EIO;
605	605	break;
606	606	}
...	...	@@ -828,7 +828,7 @@
828	828	/* set bits for operations that won't block */
829	829	if (n_hdlc->rx_buf_list.head)
830	830	mask \|= POLLIN \| POLLRDNORM; /* readable */
831		- if (test_bit(TTY_OTHER_DONE, &tty->flags))
	831	+ if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
832	832	mask \|= POLLHUP;
833	833	if (tty_hung_up_p(filp))
834	834	mask \|= POLLHUP;
...	...	@@ -1955,18 +1955,6 @@
1955	1955	return ldata->commit_head - ldata->read_tail >= amt;
1956	1956	}
1957	1957
1958		-static inline int check_other_done(struct tty_struct *tty)
1959		-{
1960		- int done = test_bit(TTY_OTHER_DONE, &tty->flags);
1961		- if (done) {
1962		- /* paired with cmpxchg() in check_other_closed(); ensures
1963		- * read buffer head index is not stale
1964		- */
1965		- smp_mb__after_atomic();
1966		- }
1967		- return done;
1968		-}
1969		-
1970	1958	/**
1971	1959	* copy_from_read_buf - copy read data directly
1972	1960	* @tty: terminal device
...	...	@@ -2171,7 +2159,7 @@
2171	2159	struct n_tty_data *ldata = tty->disc_data;
2172	2160	unsigned char __user *b = buf;
2173	2161	DEFINE_WAIT_FUNC(wait, woken_wake_function);
2174		- int c, done;
	2162	+ int c;
2175	2163	int minimum, time;
2176	2164	ssize_t retval = 0;
2177	2165	long timeout;
2178	2166
2179	2167
2180	2168
2181	2169
...	...	@@ -2239,32 +2227,35 @@
2239	2227	((minimum - (b - buf)) >= 1))
2240	2228	ldata->minimum_to_wake = (minimum - (b - buf));
2241	2229
2242		- done = check_other_done(tty);
2243		-
2244	2230	if (!input_available_p(tty, 0)) {
2245		- if (done) {
2246		- retval = -EIO;
2247		- break;
2248		- }
2249		- if (tty_hung_up_p(file))
2250		- break;
2251		- if (!timeout)
2252		- break;
2253		- if (file->f_flags & O_NONBLOCK) {
2254		- retval = -EAGAIN;
2255		- break;
2256		- }
2257		- if (signal_pending(current)) {
2258		- retval = -ERESTARTSYS;
2259		- break;
2260		- }
2261	2231	up_read(&tty->termios_rwsem);
	2232	+ tty_buffer_flush_work(tty->port);
	2233	+ down_read(&tty->termios_rwsem);
	2234	+ if (!input_available_p(tty, 0)) {
	2235	+ if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
	2236	+ retval = -EIO;
	2237	+ break;
	2238	+ }
	2239	+ if (tty_hung_up_p(file))
	2240	+ break;
	2241	+ if (!timeout)
	2242	+ break;
	2243	+ if (file->f_flags & O_NONBLOCK) {
	2244	+ retval = -EAGAIN;
	2245	+ break;
	2246	+ }
	2247	+ if (signal_pending(current)) {
	2248	+ retval = -ERESTARTSYS;
	2249	+ break;
	2250	+ }
	2251	+ up_read(&tty->termios_rwsem);
2262	2252
2263		- timeout = wait_woken(&wait, TASK_INTERRUPTIBLE,
2264		- timeout);
	2253	+ timeout = wait_woken(&wait, TASK_INTERRUPTIBLE,
	2254	+ timeout);
2265	2255
2266		- down_read(&tty->termios_rwsem);
2267		- continue;
	2256	+ down_read(&tty->termios_rwsem);
	2257	+ continue;
	2258	+ }
2268	2259	}
2269	2260
2270	2261	if (ldata->icanon && !L_EXTPROC(tty)) {
2271	2262
2272	2263
...	...	@@ -2446,12 +2437,17 @@
2446	2437
2447	2438	poll_wait(file, &tty->read_wait, wait);
2448	2439	poll_wait(file, &tty->write_wait, wait);
2449		- if (check_other_done(tty))
2450		- mask \|= POLLHUP;
2451	2440	if (input_available_p(tty, 1))
2452	2441	mask \|= POLLIN \| POLLRDNORM;
	2442	+ else {
	2443	+ tty_buffer_flush_work(tty->port);
	2444	+ if (input_available_p(tty, 1))
	2445	+ mask \|= POLLIN \| POLLRDNORM;
	2446	+ }
2453	2447	if (tty->packet && tty->link->ctrl_status)
2454	2448	mask \|= POLLPRI \| POLLIN \| POLLRDNORM;
	2449	+ if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
	2450	+ mask \|= POLLHUP;
2455	2451	if (tty_hung_up_p(file))
2456	2452	mask \|= POLLHUP;
2457	2453	if (!(mask & (POLLHUP \| POLLIN \| POLLRDNORM))) {
...	...	@@ -59,7 +59,7 @@
59	59	if (!tty->link)
60	60	return;
61	61	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
62		- tty_flip_buffer_push(tty->link->port);
	62	+ wake_up_interruptible(&tty->link->read_wait);
63	63	wake_up_interruptible(&tty->link->write_wait);
64	64	if (tty->driver->subtype == PTY_TYPE_MASTER) {
65	65	set_bit(TTY_OTHER_CLOSED, &tty->flags);
66	66
...	...	@@ -247,9 +247,7 @@
247	247	goto out;
248	248
249	249	clear_bit(TTY_IO_ERROR, &tty->flags);
250		- /* TTY_OTHER_CLOSED must be cleared before TTY_OTHER_DONE */
251	250	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
252		- clear_bit(TTY_OTHER_DONE, &tty->link->flags);
253	251	set_bit(TTY_THROTTLED, &tty->flags);
254	252	return 0;
255	253
...	...	@@ -37,29 +37,6 @@
37	37
38	38	#define TTY_BUFFER_PAGE (((PAGE_SIZE - sizeof(struct tty_buffer)) / 2) & ~0xFF)
39	39
40		-/*
41		- * If all tty flip buffers have been processed by flush_to_ldisc() or
42		- * dropped by tty_buffer_flush(), check if the linked pty has been closed.
43		- * If so, wake the reader/poll to process
44		- */
45		-static inline void check_other_closed(struct tty_struct *tty)
46		-{
47		- unsigned long flags, old;
48		-
49		- /* transition from TTY_OTHER_CLOSED => TTY_OTHER_DONE must be atomic */
50		- for (flags = ACCESS_ONCE(tty->flags);
51		- test_bit(TTY_OTHER_CLOSED, &flags);
52		- ) {
53		- old = flags;
54		- __set_bit(TTY_OTHER_DONE, &flags);
55		- flags = cmpxchg(&tty->flags, old, flags);
56		- if (old == flags) {
57		- wake_up_interruptible(&tty->read_wait);
58		- break;
59		- }
60		- }
61		-}
62		-
63	40	/**
64	41	* tty_buffer_lock_exclusive - gain exclusive access to buffer
65	42	* tty_buffer_unlock_exclusive - release exclusive access
...	...	@@ -254,8 +231,6 @@
254	231	if (ld && ld->ops->flush_buffer)
255	232	ld->ops->flush_buffer(tty);
256	233
257		- check_other_closed(tty);
258		-
259	234	atomic_dec(&buf->priority);
260	235	mutex_unlock(&buf->lock);
261	236	}
262	237
...	...	@@ -505,10 +480,8 @@
505	480	*/
506	481	count = smp_load_acquire(&head->commit) - head->read;
507	482	if (!count) {
508		- if (next == NULL) {
509		- check_other_closed(tty);
	483	+ if (next == NULL)
510	484	break;
511		- }
512	485	buf->head = next;
513	486	tty_buffer_free(port, head);
514	487	continue;
...	...	@@ -596,5 +569,10 @@
596	569	bool tty_buffer_cancel_work(struct tty_port *port)
597	570	{
598	571	return cancel_work_sync(&port->buf.work);
	572	+}
	573	+
	574	+void tty_buffer_flush_work(struct tty_port *port)
	575	+{
	576	+ flush_work(&port->buf.work);
599	577	}
...	...	@@ -338,7 +338,6 @@
338	338	#define TTY_EXCLUSIVE 3 /* Exclusive open mode */
339	339	#define TTY_DEBUG 4 /* Debugging */
340	340	#define TTY_DO_WRITE_WAKEUP 5 /* Call write_wakeup after queuing new */
341		-#define TTY_OTHER_DONE 6 /* Closed pty has completed input processing */
342	341	#define TTY_LDISC_OPEN 11 /* Line discipline is open */
343	342	#define TTY_PTY_LOCK 16 /* pty private */
344	343	#define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver */
...	...	@@ -469,6 +468,7 @@
469	468	extern void tty_buffer_set_lock_subclass(struct tty_port *port);
470	469	extern bool tty_buffer_restart_work(struct tty_port *port);
471	470	extern bool tty_buffer_cancel_work(struct tty_port *port);
	471	+extern void tty_buffer_flush_work(struct tty_port *port);
472	472	extern speed_t tty_termios_baud_rate(struct ktermios *termios);
473	473	extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
474	474	extern void tty_termios_encode_baud_rate(struct ktermios *termios,