Skip to contentSkip to conversation

n_tty fixes

PATCH Superseded Peter Hurley 3 participants 23 messages Mar 6, 2013 – Jun 17, 2013 TTY layer and serial drivers

lore
Share & tools
lore

Peter Hurley

author
#0 lore raw

cover n_tty fixes

This patch series combines a number of fixes for the N_TTY line discipline.

These address unsafe use of the foreground process group id:
n_tty: Fix unsafe driver-side signals
n_tty: Lock access to tty->pgrp for POSIX job control

These continue Jiri's work to separate ldisc from tty:
tty: Fix checkpatch errors in tty_ldisc.h
n_tty: Encapsulate minimum_to_wake within N_TTY

The rest are self-explanatory:
n_tty: Untangle read completion variables
n_tty: Fix unsafe update of available buffer space
n_tty: Buffer work should not reschedule itself

Peter Hurley (7):
n_tty: Fix unsafe driver-side signals
n_tty: Lock access to tty->pgrp for POSIX job control
tty: Fix checkpatch errors in tty_ldisc.h
n_tty: Encapsulate minimum_to_wake within N_TTY
n_tty: Untangle read completion variables
n_tty: Fix unsafe update of available buffer space
n_tty: Buffer work should not reschedule itself

 drivers/tty/n_tty.c       | 144 +++++++++++++++++++++++++---------------------
 drivers/tty/tty_io.c      |  17 +++---
 include/linux/tty.h       |   1 -
 include/linux/tty_ldisc.h | 138 +++++++++++++++++++++++---------------------
 4 files changed, 161 insertions(+), 139 deletions(-)
Signature · 1 line
1.8.1.2
author
#1 lore raw

PATCH 1/7 n_tty: Fix unsafe driver-side signals

An ldisc reference is insufficient guarantee the foreground process group is not in the process of being signalled from a hangup.

1) Reads of tty->pgrp must be locked with ctrl_lock
2) The group pid must be referenced for the duration of signalling. Because the driver-side is not process-context, a pid reference must be acquired.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)
drivers/tty/n_tty.c +14 -20
index 66ce178..cd9ba3d 100644
@@ -1038,23 +1038,19 @@ static void eraser(unsigned char c, struct tty_struct *tty)
  *	isig		-	handle the ISIG optio
  *	@sig: signal
  *	@tty: terminal
- *	@flush: force flush
  *
- *	Called when a signal is being sent due to terminal input. This
- *	may caus terminal flushing to take place according to the termios
- *	settings and character used. Called from the driver receive_buf
- *	path so serialized.
+ *	Called when a signal is being sent due to terminal input.
+ *	Called from the driver receive_buf path so serialized.
  *
- *	Locking: ctrl_lock, read_lock (both via flush buffer)
+ *	Locking: ctrl_lock
  */
 
-static inline void isig(int sig, struct tty_struct *tty, int flush)
+static inline void isig(int sig, struct tty_struct *tty)
 {
-	if (tty->pgrp)
-		kill_pgrp(tty->pgrp, sig, 1);
-	if (flush || !L_NOFLSH(tty)) {
-		n_tty_flush_buffer(tty);
-		tty_driver_flush_buffer(tty);
+	struct pid *tty_pgrp = tty_get_pgrp(tty);
+	if (tty_pgrp) {
+		kill_pgrp(tty_pgrp, sig, 1);
+		put_pid(tty_pgrp);
 	}
 }
 

@@ -1075,7 +1071,11 @@ static inline void n_tty_receive_break(struct tty_struct *tty)
 	if (I_IGNBRK(tty))
 		return;
 	if (I_BRKINT(tty)) {
-		isig(SIGINT, tty, 1);
+		isig(SIGINT, tty);
+		if (!L_NOFLSH(tty)) {
+			n_tty_flush_buffer(tty);
+			tty_driver_flush_buffer(tty);
+		}
 		return;
 	}
 	if (I_PARMRK(tty)) {

@@ -1242,11 +1242,6 @@ static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 		signal = SIGTSTP;
 		if (c == SUSP_CHAR(tty)) {
 send_signal:
-			/*
-			 * Note that we do not use isig() here because we want
-			 * the order to be:
-			 * 1) flush, 2) echo, 3) signal
-			 */
 			if (!L_NOFLSH(tty)) {
 				n_tty_flush_buffer(tty);
 				tty_driver_flush_buffer(tty);

@@ -1257,8 +1252,7 @@ send_signal:
 				echo_char(c, tty);
 				process_echoes(tty);
 			}
-			if (tty->pgrp)
-				kill_pgrp(tty->pgrp, signal, 1);
+			isig(signal, tty);
 			return;
 		}
 	}
Signature · 1 line
1.8.1.2
author
#2 lore raw

PATCH 2/7 n_tty: Lock access to tty->pgrp for POSIX job control

In reply to Peter Hurley

Concurrent access to tty->pgrp must be protected with tty->ctrl_lock. Also, as noted in the comments, reading current->signal->tty is safe because either,
1) current->signal->tty is assigned by current, or
2) current->signal->tty is set to NULL.

NB: for reference, tty_check_change() implements a similar POSIX check for the ioctls corresponding to tcflush(), tcdrain(), tcsetattr(), tcsetpgrp(), tcflow() and tcsendbreak().

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)
drivers/tty/n_tty.c +18 -16
index cd9ba3d..61797c4 100644
@@ -1744,10 +1744,9 @@ extern ssize_t redirected_tty_write(struct file *, const char __user *,
  *	and if appropriate send any needed signals and return a negative
  *	error code if action should be taken.
  *
- *	FIXME:
- *	Locking: None - redirected write test is safe, testing
- *	current->signal should possibly lock current->sighand
- *	pgrp locking ?
+ *	Locking: redirected write test is safe
+ *		 current->signal->tty check is safe
+ *		 ctrl_lock to safely reference tty->pgrp
  */
 
 static int job_control(struct tty_struct *tty, struct file *file)

@@ -1757,19 +1756,22 @@ static int job_control(struct tty_struct *tty, struct file *file)
 	/* NOTE: not yet done after every sleep pending a thorough
 	   check of the logic of this change. -- jlc */
 	/* don't stop on /dev/console */
-	if (file->f_op->write != redirected_tty_write &&
-	    current->signal->tty == tty) {
-		if (!tty->pgrp)
-			printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
-		else if (task_pgrp(current) != tty->pgrp) {
-			if (is_ignored(SIGTTIN) ||
-			    is_current_pgrp_orphaned())
-				return -EIO;
-			kill_pgrp(task_pgrp(current), SIGTTIN, 1);
-			set_thread_flag(TIF_SIGPENDING);
-			return -ERESTARTSYS;
-		}
+	if (file->f_op->write == redirected_tty_write ||
+	    current->signal->tty != tty)
+		return 0;
+
+	spin_lock_irq(&tty->ctrl_lock);
+	if (!tty->pgrp)
+		printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
+	else if (task_pgrp(current) != tty->pgrp) {
+		spin_unlock_irq(&tty->ctrl_lock);
+		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned())
+			return -EIO;
+		kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+		set_thread_flag(TIF_SIGPENDING);
+		return -ERESTARTSYS;
 	}
+	spin_unlock_irq(&tty->ctrl_lock);
 	return 0;
 }
Signature · 1 line
1.8.1.2
author
#3 lore raw

PATCH 3/7 tty: Fix checkpatch errors in tty_ldisc.h

In reply to Peter Hurley
Signed-off-by Peter Hurley <[email protected]>
---
 include/linux/tty_ldisc.h | 132 +++++++++++++++++++++++-----------------------
 1 file changed, 66 insertions(+), 66 deletions(-)
include/linux/tty_ldisc.h +66 -66
index a8c58c2..1ef449e 100644
@@ -9,89 +9,89 @@
  *
  * int	(*open)(struct tty_struct *);
  *
- * 	This function is called when the line discipline is associated
- * 	with the tty.  The line discipline can use this as an
- * 	opportunity to initialize any state needed by the ldisc routines.
- * 
+ *	This function is called when the line discipline is associated
+ *	with the tty.  The line discipline can use this as an
+ *	opportunity to initialize any state needed by the ldisc routines.
+ *
  * void	(*close)(struct tty_struct *);
  *
  *	This function is called when the line discipline is being
- * 	shutdown, either because the tty is being closed or because
- * 	the tty is being changed to use a new line discipline
- * 
+ *	shutdown, either because the tty is being closed or because
+ *	the tty is being changed to use a new line discipline
+ *
  * void	(*flush_buffer)(struct tty_struct *tty);
  *
- * 	This function instructs the line discipline to clear its
- * 	buffers of any input characters it may have queued to be
- * 	delivered to the user mode process.
- * 
+ *	This function instructs the line discipline to clear its
+ *	buffers of any input characters it may have queued to be
+ *	delivered to the user mode process.
+ *
  * ssize_t (*chars_in_buffer)(struct tty_struct *tty);
  *
- * 	This function returns the number of input characters the line
+ *	This function returns the number of input characters the line
  *	discipline may have queued up to be delivered to the user mode
  *	process.
- * 
+ *
  * ssize_t (*read)(struct tty_struct * tty, struct file * file,
  *		   unsigned char * buf, size_t nr);
  *
- * 	This function is called when the user requests to read from
- * 	the tty.  The line discipline will return whatever characters
- * 	it has buffered up for the user.  If this function is not
- * 	defined, the user will receive an EIO error.
- * 
+ *	This function is called when the user requests to read from
+ *	the tty.  The line discipline will return whatever characters
+ *	it has buffered up for the user.  If this function is not
+ *	defined, the user will receive an EIO error.
+ *
  * ssize_t (*write)(struct tty_struct * tty, struct file * file,
- * 		    const unsigned char * buf, size_t nr);
- *
- * 	This function is called when the user requests to write to the
- * 	tty.  The line discipline will deliver the characters to the
- * 	low-level tty device for transmission, optionally performing
- * 	some processing on the characters first.  If this function is
- * 	not defined, the user will receive an EIO error.
- * 
+ *		    const unsigned char * buf, size_t nr);
+ *
+ *	This function is called when the user requests to write to the
+ *	tty.  The line discipline will deliver the characters to the
+ *	low-level tty device for transmission, optionally performing
+ *	some processing on the characters first.  If this function is
+ *	not defined, the user will receive an EIO error.
+ *
  * int	(*ioctl)(struct tty_struct * tty, struct file * file,
- * 		 unsigned int cmd, unsigned long arg);
+ *		 unsigned int cmd, unsigned long arg);
  *
  *	This function is called when the user requests an ioctl which
- * 	is not handled by the tty layer or the low-level tty driver.
- * 	It is intended for ioctls which affect line discpline
- * 	operation.  Note that the search order for ioctls is (1) tty
- * 	layer, (2) tty low-level driver, (3) line discpline.  So a
- * 	low-level driver can "grab" an ioctl request before the line
- * 	discpline has a chance to see it.
- * 
+ *	is not handled by the tty layer or the low-level tty driver.
+ *	It is intended for ioctls which affect line discpline
+ *	operation.  Note that the search order for ioctls is (1) tty
+ *	layer, (2) tty low-level driver, (3) line discpline.  So a
+ *	low-level driver can "grab" an ioctl request before the line
+ *	discpline has a chance to see it.
+ *
  * long	(*compat_ioctl)(struct tty_struct * tty, struct file * file,
- * 		        unsigned int cmd, unsigned long arg);
+ *		        unsigned int cmd, unsigned long arg);
  *
- *      Process ioctl calls from 32-bit process on 64-bit system
+ *	Process ioctl calls from 32-bit process on 64-bit system
  *
  * void	(*set_termios)(struct tty_struct *tty, struct ktermios * old);
  *
- * 	This function notifies the line discpline that a change has
- * 	been made to the termios structure.
- * 
+ *	This function notifies the line discpline that a change has
+ *	been made to the termios structure.
+ *
  * int	(*poll)(struct tty_struct * tty, struct file * file,
- * 		  poll_table *wait);
+ *		  poll_table *wait);
  *
- * 	This function is called when a user attempts to select/poll on a
- * 	tty device.  It is solely the responsibility of the line
- * 	discipline to handle poll requests.
+ *	This function is called when a user attempts to select/poll on a
+ *	tty device.  It is solely the responsibility of the line
+ *	discipline to handle poll requests.
  *
  * void	(*receive_buf)(struct tty_struct *, const unsigned char *cp,
- * 		       char *fp, int count);
- *
- * 	This function is called by the low-level tty driver to send
- * 	characters received by the hardware to the line discpline for
- * 	processing.  <cp> is a pointer to the buffer of input
- * 	character received by the device.  <fp> is a pointer to a
- * 	pointer of flag bytes which indicate whether a character was
- * 	received with a parity error, etc.
- * 
+ *		       char *fp, int count);
+ *
+ *	This function is called by the low-level tty driver to send
+ *	characters received by the hardware to the line discpline for
+ *	processing.  <cp> is a pointer to the buffer of input
+ *	character received by the device.  <fp> is a pointer to a
+ *	pointer of flag bytes which indicate whether a character was
+ *	received with a parity error, etc.
+ *
  * void	(*write_wakeup)(struct tty_struct *);
  *
- * 	This function is called by the low-level tty driver to signal
- * 	that line discpline should try to send more characters to the
- * 	low-level driver for transmission.  If the line discpline does
- * 	not have any more data to send, it can just return.
+ *	This function is called by the low-level tty driver to signal
+ *	that line discpline should try to send more characters to the
+ *	low-level driver for transmission.  If the line discpline does
+ *	not have any more data to send, it can just return.
  *
  * int (*hangup)(struct tty_struct *)
  *

@@ -162,7 +162,7 @@ struct tty_ldisc_ops {
 	char	*name;
 	int	num;
 	int	flags;
-	
+
 	/*
 	 * The following routines are called from above.
 	 */

@@ -170,19 +170,19 @@ struct tty_ldisc_ops {
 	void	(*close)(struct tty_struct *);
 	void	(*flush_buffer)(struct tty_struct *tty);
 	ssize_t	(*chars_in_buffer)(struct tty_struct *tty);
-	ssize_t	(*read)(struct tty_struct * tty, struct file * file,
-			unsigned char __user * buf, size_t nr);
-	ssize_t	(*write)(struct tty_struct * tty, struct file * file,
-			 const unsigned char * buf, size_t nr);	
-	int	(*ioctl)(struct tty_struct * tty, struct file * file,
+	ssize_t	(*read)(struct tty_struct *tty, struct file *file,
+			unsigned char __user *buf, size_t nr);
+	ssize_t	(*write)(struct tty_struct *tty, struct file *file,
+			 const unsigned char *buf, size_t nr);
+	int	(*ioctl)(struct tty_struct *tty, struct file *file,
 			 unsigned int cmd, unsigned long arg);
-	long	(*compat_ioctl)(struct tty_struct * tty, struct file * file,
+	long	(*compat_ioctl)(struct tty_struct *tty, struct file *file,
 				unsigned int cmd, unsigned long arg);
-	void	(*set_termios)(struct tty_struct *tty, struct ktermios * old);
+	void	(*set_termios)(struct tty_struct *tty, struct ktermios *old);
 	unsigned int (*poll)(struct tty_struct *, struct file *,
 			     struct poll_table_struct *);
 	int	(*hangup)(struct tty_struct *tty);
-	
+
 	/*
 	 * The following routines are called from below.
 	 */

@@ -192,7 +192,7 @@ struct tty_ldisc_ops {
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
 
 	struct  module *owner;
-	
+
 	int refcount;
 };
Signature · 1 line
1.8.1.2
author
#4 lore raw

PATCH 4/7 n_tty: Encapsulate minimum_to_wake within N_TTY

In reply to Peter Hurley

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c       | 39 +++++++++++++++++++++++++++------------
 drivers/tty/tty_io.c      | 17 +++++++++--------
 include/linux/tty.h       |  1 -
 include/linux/tty_ldisc.h |  6 ++++++
 4 files changed, 42 insertions(+), 21 deletions(-)
drivers/tty/n_tty.c +27 -12
index 61797c4..3fd657b 100644
@@ -89,6 +89,7 @@ struct n_tty_data {
 	int read_head;
 	int read_tail;
 	int read_cnt;
+	int minimum_to_wake;
 
 	unsigned char *echo_buf;
 	unsigned int echo_pos;

@@ -1471,7 +1472,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 
 	n_tty_set_room(tty);
 
-	if ((!ldata->icanon && (ldata->read_cnt >= tty->minimum_to_wake)) ||
+	if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		if (waitqueue_active(&tty->read_wait))

@@ -1649,7 +1650,7 @@ static int n_tty_open(struct tty_struct *tty)
 	tty->disc_data = ldata;
 	reset_buffer_flags(tty->disc_data);
 	ldata->column = 0;
-	tty->minimum_to_wake = 1;
+	ldata->minimum_to_wake = 1;
 	tty->closing = 0;
 	/* indicate buffer work may resume */
 	clear_bit(TTY_LDISC_HALTED, &tty->flags);

@@ -1817,17 +1818,17 @@ do_it_again:
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
 			if (time)
-				tty->minimum_to_wake = 1;
+				ldata->minimum_to_wake = 1;
 			else if (!waitqueue_active(&tty->read_wait) ||
-				 (tty->minimum_to_wake > minimum))
-				tty->minimum_to_wake = minimum;
+				 (ldata->minimum_to_wake > minimum))
+				ldata->minimum_to_wake = minimum;
 		} else {
 			timeout = 0;
 			if (time) {
 				timeout = time;
 				time = 0;
 			}
-			tty->minimum_to_wake = minimum = 1;
+			ldata->minimum_to_wake = minimum = 1;
 		}
 	}
 

@@ -1867,9 +1868,9 @@ do_it_again:
 		   TASK_RUNNING. */
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		if (((minimum - (b - buf)) < tty->minimum_to_wake) &&
+		if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
 		    ((minimum - (b - buf)) >= 1))
-			tty->minimum_to_wake = (minimum - (b - buf));
+			ldata->minimum_to_wake = (minimum - (b - buf));
 
 		if (!input_available_p(tty, 0)) {
 			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {

@@ -1979,7 +1980,7 @@ do_it_again:
 	remove_wait_queue(&tty->read_wait, &wait);
 
 	if (!waitqueue_active(&tty->read_wait))
-		tty->minimum_to_wake = minimum;
+		ldata->minimum_to_wake = minimum;
 
 	__set_current_state(TASK_RUNNING);
 	size = b - buf;

@@ -2111,6 +2112,7 @@ break_out:
 static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 							poll_table *wait)
 {
+	struct n_tty_data *ldata = tty->disc_data;
 	unsigned int mask = 0;
 
 	poll_wait(file, &tty->read_wait, wait);

@@ -2125,9 +2127,9 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 		mask |= POLLHUP;
 	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
 		if (MIN_CHAR(tty) && !TIME_CHAR(tty))
-			tty->minimum_to_wake = MIN_CHAR(tty);
+			ldata->minimum_to_wake = MIN_CHAR(tty);
 		else
-			tty->minimum_to_wake = 1;
+			ldata->minimum_to_wake = 1;
 	}
 	if (tty->ops->write && !tty_is_writelocked(tty) &&
 			tty_chars_in_buffer(tty) < WAKEUP_CHARS &&

@@ -2175,6 +2177,18 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
+static void n_tty_fasync(struct tty_struct *tty, int on)
+{
+	struct n_tty_data *ldata = tty->disc_data;
+
+	if (!waitqueue_active(&tty->read_wait)) {
+		if (on)
+			ldata->minimum_to_wake = 1;
+		else if (!tty->fasync)
+			ldata->minimum_to_wake = N_TTY_BUF_SIZE;
+	}
+}
+
 struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",

@@ -2188,7 +2202,8 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.set_termios     = n_tty_set_termios,
 	.poll            = n_tty_poll,
 	.receive_buf     = n_tty_receive_buf,
-	.write_wakeup    = n_tty_write_wakeup
+	.write_wakeup    = n_tty_write_wakeup,
+	.fasync		 = n_tty_fasync,
 };
 
 /**
drivers/tty/tty_io.c +9 -8
index adc1d01..525c915 100644
@@ -2077,6 +2077,7 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
+	struct tty_ldisc *ldisc;
 	unsigned long flags;
 	int retval = 0;
 

@@ -2087,11 +2088,17 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 	if (retval <= 0)
 		goto out;
 
+	ldisc = tty_ldisc_ref(tty);
+	if (ldisc) {
+		if (ldisc->ops->fasync)
+			ldisc->ops->fasync(tty, on);
+		tty_ldisc_deref(ldisc);
+	}
+
 	if (on) {
 		enum pid_type type;
 		struct pid *pid;
-		if (!waitqueue_active(&tty->read_wait))
-			tty->minimum_to_wake = 1;
+
 		spin_lock_irqsave(&tty->ctrl_lock, flags);
 		if (tty->pgrp) {
 			pid = tty->pgrp;

@@ -2104,13 +2111,7 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 		retval = __f_setown(filp, pid, type, 0);
 		put_pid(pid);
-		if (retval)
-			goto out;
-	} else {
-		if (!tty->fasync && !waitqueue_active(&tty->read_wait))
-			tty->minimum_to_wake = N_TTY_BUF_SIZE;
 	}
-	retval = 0;
 out:
 	return retval;
 }
include/linux/tty.h +0 -1
index 2c109a3..39f0317 100644
@@ -272,7 +272,6 @@ struct tty_struct {
 #define N_TTY_BUF_SIZE 4096
 
 	unsigned char closing:1;
-	unsigned short minimum_to_wake;
 	unsigned char *write_buf;
 	int write_cnt;
 	/* If the tty has a pending do_SAK, queue it here - akpm */
include/linux/tty_ldisc.h +6 -0
index 1ef449e..44584d9 100644
@@ -100,6 +100,11 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
+ * void (*fasync)(struct tty_struct *, int on)
+ *
+ *	Notify line discipline when signal-driven I/O is enabled or
+ *	disabled.
+ *
  * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
  *	Tells the discipline that the DCD pin has changed its status.

@@ -190,6 +195,7 @@ struct tty_ldisc_ops {
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
+	void	(*fasync)(struct tty_struct *tty, int on);
 
 	struct  module *owner;
Signature · 1 line
1.8.1.2
maintainer
#5 lore raw

Re: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

Quoted 9 lines · Peter Hurley

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Quoted 4 lines

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>

For some reason, this patch doesn't apply. Care to refresh this one, and the rest in this series, and resend?

thanks,

greg k-h

author
#6 lore raw

Re: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

Quoted 10 lines · Greg Kroah-Hartman
Quoted 9 lines · Peter Hurley

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Quoted 7 lines
Quoted 4 lines

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>

For some reason, this patch doesn't apply. Care to refresh this one, and the rest in this series, and resend?

Sorry. There was probably some accidental dependency on one of the other patchsets of mine you did apply.

This and patch 5 now apply without error to tty-next.

maintainer
#7 lore raw

Re: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

Quoted 16 lines · Peter Hurley
Quoted 15 lines · Greg Kroah-Hartman
Quoted 13 lines · Peter Hurley

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>
Quoted 7 lines
Quoted 2 lines

For some reason, this patch doesn't apply. Care to refresh this one, and the rest in this series, and resend?

Sorry. There was probably some accidental dependency on one of the other patchsets of mine you did apply.

This and patch 5 now apply without error to tty-next.

Ok, but they are now long gone from my queue. Can you please resend what I haven't applied?

thanks,

greg k-h

author
#8 lore raw

PATCH 4/7 n_tty: Encapsulate minimum_to_wake within N_TTY

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c       | 39 +++++++++++++++++++++++++++------------
 drivers/tty/tty_io.c      | 17 +++++++++--------
 include/linux/tty.h       |  1 -
 include/linux/tty_ldisc.h |  6 ++++++
 4 files changed, 42 insertions(+), 21 deletions(-)
drivers/tty/n_tty.c +27 -12
index 61797c4..3fd657b 100644
@@ -89,6 +89,7 @@ struct n_tty_data {
 	int read_head;
 	int read_tail;
 	int read_cnt;
+	int minimum_to_wake;
 
 	unsigned char *echo_buf;
 	unsigned int echo_pos;

@@ -1471,7 +1472,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 
 	n_tty_set_room(tty);
 
-	if ((!ldata->icanon && (ldata->read_cnt >= tty->minimum_to_wake)) ||
+	if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		if (waitqueue_active(&tty->read_wait))

@@ -1649,7 +1650,7 @@ static int n_tty_open(struct tty_struct *tty)
 	tty->disc_data = ldata;
 	reset_buffer_flags(tty->disc_data);
 	ldata->column = 0;
-	tty->minimum_to_wake = 1;
+	ldata->minimum_to_wake = 1;
 	tty->closing = 0;
 	/* indicate buffer work may resume */
 	clear_bit(TTY_LDISC_HALTED, &tty->flags);

@@ -1817,17 +1818,17 @@ do_it_again:
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
 			if (time)
-				tty->minimum_to_wake = 1;
+				ldata->minimum_to_wake = 1;
 			else if (!waitqueue_active(&tty->read_wait) ||
-				 (tty->minimum_to_wake > minimum))
-				tty->minimum_to_wake = minimum;
+				 (ldata->minimum_to_wake > minimum))
+				ldata->minimum_to_wake = minimum;
 		} else {
 			timeout = 0;
 			if (time) {
 				timeout = time;
 				time = 0;
 			}
-			tty->minimum_to_wake = minimum = 1;
+			ldata->minimum_to_wake = minimum = 1;
 		}
 	}
 

@@ -1867,9 +1868,9 @@ do_it_again:
 		   TASK_RUNNING. */
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		if (((minimum - (b - buf)) < tty->minimum_to_wake) &&
+		if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
 		    ((minimum - (b - buf)) >= 1))
-			tty->minimum_to_wake = (minimum - (b - buf));
+			ldata->minimum_to_wake = (minimum - (b - buf));
 
 		if (!input_available_p(tty, 0)) {
 			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {

@@ -1979,7 +1980,7 @@ do_it_again:
 	remove_wait_queue(&tty->read_wait, &wait);
 
 	if (!waitqueue_active(&tty->read_wait))
-		tty->minimum_to_wake = minimum;
+		ldata->minimum_to_wake = minimum;
 
 	__set_current_state(TASK_RUNNING);
 	size = b - buf;

@@ -2111,6 +2112,7 @@ break_out:
 static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 							poll_table *wait)
 {
+	struct n_tty_data *ldata = tty->disc_data;
 	unsigned int mask = 0;
 
 	poll_wait(file, &tty->read_wait, wait);

@@ -2125,9 +2127,9 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 		mask |= POLLHUP;
 	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
 		if (MIN_CHAR(tty) && !TIME_CHAR(tty))
-			tty->minimum_to_wake = MIN_CHAR(tty);
+			ldata->minimum_to_wake = MIN_CHAR(tty);
 		else
-			tty->minimum_to_wake = 1;
+			ldata->minimum_to_wake = 1;
 	}
 	if (tty->ops->write && !tty_is_writelocked(tty) &&
 			tty_chars_in_buffer(tty) < WAKEUP_CHARS &&

@@ -2175,6 +2177,18 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
+static void n_tty_fasync(struct tty_struct *tty, int on)
+{
+	struct n_tty_data *ldata = tty->disc_data;
+
+	if (!waitqueue_active(&tty->read_wait)) {
+		if (on)
+			ldata->minimum_to_wake = 1;
+		else if (!tty->fasync)
+			ldata->minimum_to_wake = N_TTY_BUF_SIZE;
+	}
+}
+
 struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",

@@ -2188,7 +2202,8 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.set_termios     = n_tty_set_termios,
 	.poll            = n_tty_poll,
 	.receive_buf     = n_tty_receive_buf,
-	.write_wakeup    = n_tty_write_wakeup
+	.write_wakeup    = n_tty_write_wakeup,
+	.fasync		 = n_tty_fasync,
 };
 
 /**
drivers/tty/tty_io.c +9 -8
index adc1d01..525c915 100644
@@ -2077,6 +2077,7 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
+	struct tty_ldisc *ldisc;
 	unsigned long flags;
 	int retval = 0;
 

@@ -2087,11 +2088,17 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 	if (retval <= 0)
 		goto out;
 
+	ldisc = tty_ldisc_ref(tty);
+	if (ldisc) {
+		if (ldisc->ops->fasync)
+			ldisc->ops->fasync(tty, on);
+		tty_ldisc_deref(ldisc);
+	}
+
 	if (on) {
 		enum pid_type type;
 		struct pid *pid;
-		if (!waitqueue_active(&tty->read_wait))
-			tty->minimum_to_wake = 1;
+
 		spin_lock_irqsave(&tty->ctrl_lock, flags);
 		if (tty->pgrp) {
 			pid = tty->pgrp;

@@ -2104,13 +2111,7 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 		retval = __f_setown(filp, pid, type, 0);
 		put_pid(pid);
-		if (retval)
-			goto out;
-	} else {
-		if (!tty->fasync && !waitqueue_active(&tty->read_wait))
-			tty->minimum_to_wake = N_TTY_BUF_SIZE;
 	}
-	retval = 0;
 out:
 	return retval;
 }
include/linux/tty.h +0 -1
index 2c109a3..39f0317 100644
@@ -272,7 +272,6 @@ struct tty_struct {
 #define N_TTY_BUF_SIZE 4096
 
 	unsigned char closing:1;
-	unsigned short minimum_to_wake;
 	unsigned char *write_buf;
 	int write_cnt;
 	/* If the tty has a pending do_SAK, queue it here - akpm */
include/linux/tty_ldisc.h +6 -0
index 1ef449e..44584d9 100644
@@ -100,6 +100,11 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
+ * void (*fasync)(struct tty_struct *, int on)
+ *
+ *	Notify line discipline when signal-driven I/O is enabled or
+ *	disabled.
+ *
  * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
  *	Tells the discipline that the DCD pin has changed its status.

@@ -190,6 +195,7 @@ struct tty_ldisc_ops {
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
+	void	(*fasync)(struct tty_struct *tty, int on);
 
 	struct  module *owner;
Signature · 1 line
1.8.1.2
author
#9 lore raw

PATCH 5/7 n_tty: Untangle read completion variables

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/tty/n_tty.c +2 -6
index 3fd657b..b2f621f 100644
@@ -1814,20 +1814,16 @@ do_it_again:
 	minimum = time = 0;
 	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (!ldata->icanon) {
-		time = (HZ / 10) * TIME_CHAR(tty);
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
+			time = (HZ / 10) * TIME_CHAR(tty);
 			if (time)
 				ldata->minimum_to_wake = 1;
 			else if (!waitqueue_active(&tty->read_wait) ||
 				 (ldata->minimum_to_wake > minimum))
 				ldata->minimum_to_wake = minimum;
 		} else {
-			timeout = 0;
-			if (time) {
-				timeout = time;
-				time = 0;
-			}
+			timeout = (HZ / 10) * TIME_CHAR(tty);
 			ldata->minimum_to_wake = minimum = 1;
 		}
 	}
Signature · 1 line
1.8.1.2
author
#10 lore raw

PATCH 6/7 n_tty: Fix unsafe update of available buffer space

In reply to Peter Hurley

receive_room is used to control the amount of data the flip buffer work can push to the read buffer. This update is unsafe:

  CPU 0                        |  CPU 1
                               |
                               | n_tty_read()
                               |   n_tty_set_room()
                               |     left = <calc of space>
n_tty_receive_buf()            |
  <push data to buffer>        |
  n_tty_set_room()             |
    left = <calc of space>     |
    tty->receive_room = left   |
                               |     tty->receive_room = left

receive_room is now updated with a stale calculation of the available buffer space, and the subsequent work loop will likely overwrite unread data in the input buffer.

Update receive_room atomically with the calculation of the available buffer space.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/tty/n_tty.c +11 -7
index b2f621f..61a55f4 100644
@@ -115,13 +115,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
 }
 
 /**
- *	n_tty_set__room	-	receive space
+ *	n_tty_set_room	-	receive space
  *	@tty: terminal
  *
- *	Called by the driver to find out how much data it is
- *	permitted to feed to the line discipline without any being lost
- *	and thus to manage flow control. Not serialized. Answers for the
- *	"instant".
+ *	Sets tty->receive_room to reflect the currently available space
+ *	in the input buffer, and re-schedules the flip buffer work if space
+ *	just became available.
+ *
+ *	Locks: Concurrent update is protected with read_lock
  */
 
 static void n_tty_set_room(struct tty_struct *tty)

@@ -129,8 +130,10 @@ static void n_tty_set_room(struct tty_struct *tty)
 	struct n_tty_data *ldata = tty->disc_data;
 	int left;
 	int old_left;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&ldata->read_lock, flags);
 
-	/* ldata->read_cnt is not read locked ? */
 	if (I_PARMRK(tty)) {
 		/* Multiply read_cnt by 3, since each byte might take up to
 		 * three times as many spaces when PARMRK is set (depending on

@@ -150,6 +153,8 @@ static void n_tty_set_room(struct tty_struct *tty)
 	old_left = tty->receive_room;
 	tty->receive_room = left;
 
+	raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+
 	/* Did this open up the receive buffer? We may need to flip */
 	if (left && !old_left) {
 		WARN_RATELIMIT(tty->port->itty == NULL,

@@ -1885,7 +1890,6 @@ do_it_again:
 				retval = -ERESTARTSYS;
 				break;
 			}
-			/* FIXME: does n_tty_set_room need locking ? */
 			n_tty_set_room(tty);
 			timeout = schedule_timeout(timeout);
 			continue;
Signature · 1 line
1.8.1.2
#11 lore raw

Re: [PATCH 6/7] n_tty: Fix unsafe update of available buffer space

Quoted 15 lines · Peter Hurley

receive_room is used to control the amount of data the flip buffer work can push to the read buffer. This update is unsafe:

  CPU 0                        |  CPU 1
                               |
                               | n_tty_read()
                               |   n_tty_set_room()
                               |     left = <calc of space>
n_tty_receive_buf()            |
  <push data to buffer>        |
  n_tty_set_room()             |
    left = <calc of space>     |
    tty->receive_room = left   |
                               |     tty->receive_room = left
Quoted 4 lines

receive_room is now updated with a stale calculation of the available buffer space, and the subsequent work loop will likely overwrite unread data in the input buffer.

Sounds reasonable to me.
Thank you.
Ilya Zykov <[email protected]>

author
#12 lore raw

PATCH 7/7 n_tty: Buffer work should not reschedule itself

In reply to Peter Hurley

Although the driver-side input path must update the available buffer space, it should not reschedule itself. If space is still available and the flip buffers are not empty, flush_to_ldisc() will loop again.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/tty/n_tty.c +9 -4
index 61a55f4..6f9694d 100644
@@ -118,14 +118,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
  *	n_tty_set_room	-	receive space
  *	@tty: terminal
  *
- *	Sets tty->receive_room to reflect the currently available space
+ *	Updates tty->receive_room to reflect the currently available space
  *	in the input buffer, and re-schedules the flip buffer work if space
  *	just became available.
  *
  *	Locks: Concurrent update is protected with read_lock
  */
 
-static void n_tty_set_room(struct tty_struct *tty)
+static int set_room(struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 	int left;

@@ -155,8 +155,13 @@ static void n_tty_set_room(struct tty_struct *tty)
 
 	raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
 
+	return left && !old_left;
+}
+
+static void n_tty_set_room(struct tty_struct *tty)
+{
 	/* Did this open up the receive buffer? We may need to flip */
-	if (left && !old_left) {
+	if (set_room(tty)) {
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
 		/* see if ldisc has been killed - if so, this means that

@@ -1475,7 +1480,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 			tty->ops->flush_chars(tty);
 	}
 
-	n_tty_set_room(tty);
+	set_room(tty);
 
 	if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
Signature · 1 line
1.8.1.2
#13 lore raw

Re: [PATCH 7/7] n_tty: Buffer work should not reschedule itself

Quoted 5 lines · Peter Hurley

Although the driver-side input path must update the available buffer space, it should not reschedule itself. If space is still available and the flip buffers are not empty, flush_to_ldisc() will loop again.

Sounds reasonable to me.
Thank you.
Ilya Zykov <[email protected]>

author
#14 lore raw

cover n_tty fixes

In reply to Greg Kroah-Hartman
Quoted 19 lines · Peter Hurley
Quoted 19 lines · Greg Kroah-Hartman
Quoted 17 lines · Peter Hurley
Quoted 9 lines

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Quoted 4 lines

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>

For some reason, this patch doesn't apply. Care to refresh this one, and the rest in this series, and resend?

Quoted 7 lines
Quoted 4 lines

Sorry. There was probably some accidental dependency on one of the other patchsets of mine you did apply.

This and patch 5 now apply without error to tty-next.

Ok, but they are now long gone from my queue. Can you please resend what I haven't applied?

Greg,

I resent these back on 19 Mar but they never got applied. (maybe because I resent them as 4,5,6 & 7/7 ??)

Anyway, these apply cleanly to tty-next.

Regards,
Peter Hurley

Peter Hurley (4):
n_tty: Encapsulate minimum_to_wake within N_TTY
n_tty: Untangle read completion variables
n_tty: Fix unsafe update of available buffer space
n_tty: Buffer work should not reschedule itself

 drivers/tty/n_tty.c       | 76 ++++++++++++++++++++++++++++++-----------------
 drivers/tty/tty_io.c      | 17 ++++++-----
 include/linux/tty.h       |  1 -
 include/linux/tty_ldisc.h |  6 ++++
 4 files changed, 63 insertions(+), 37 deletions(-)
Signature · 1 line
1.8.1.2
author
#15 lore raw

PATCH 1/4 n_tty: Encapsulate minimum_to_wake within N_TTY

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c       | 39 +++++++++++++++++++++++++++------------
 drivers/tty/tty_io.c      | 17 +++++++++--------
 include/linux/tty.h       |  1 -
 include/linux/tty_ldisc.h |  6 ++++++
 4 files changed, 42 insertions(+), 21 deletions(-)
drivers/tty/n_tty.c +27 -12
index cdcdb0e..f1806de 100644
@@ -89,6 +89,7 @@ struct n_tty_data {
 	int read_head;
 	int read_tail;
 	int read_cnt;
+	int minimum_to_wake;
 
 	unsigned char *echo_buf;
 	unsigned int echo_pos;

@@ -1455,7 +1456,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 
 	n_tty_set_room(tty);
 
-	if ((!ldata->icanon && (ldata->read_cnt >= tty->minimum_to_wake)) ||
+	if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
 		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
 		if (waitqueue_active(&tty->read_wait))

@@ -1636,7 +1637,7 @@ static int n_tty_open(struct tty_struct *tty)
 	tty->disc_data = ldata;
 	reset_buffer_flags(tty->disc_data);
 	ldata->column = 0;
-	tty->minimum_to_wake = 1;
+	ldata->minimum_to_wake = 1;
 	tty->closing = 0;
 	/* indicate buffer work may resume */
 	clear_bit(TTY_LDISC_HALTED, &tty->flags);

@@ -1804,17 +1805,17 @@ do_it_again:
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
 			if (time)
-				tty->minimum_to_wake = 1;
+				ldata->minimum_to_wake = 1;
 			else if (!waitqueue_active(&tty->read_wait) ||
-				 (tty->minimum_to_wake > minimum))
-				tty->minimum_to_wake = minimum;
+				 (ldata->minimum_to_wake > minimum))
+				ldata->minimum_to_wake = minimum;
 		} else {
 			timeout = 0;
 			if (time) {
 				timeout = time;
 				time = 0;
 			}
-			tty->minimum_to_wake = minimum = 1;
+			ldata->minimum_to_wake = minimum = 1;
 		}
 	}
 

@@ -1854,9 +1855,9 @@ do_it_again:
 		   TASK_RUNNING. */
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		if (((minimum - (b - buf)) < tty->minimum_to_wake) &&
+		if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
 		    ((minimum - (b - buf)) >= 1))
-			tty->minimum_to_wake = (minimum - (b - buf));
+			ldata->minimum_to_wake = (minimum - (b - buf));
 
 		if (!input_available_p(tty, 0)) {
 			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {

@@ -1973,7 +1974,7 @@ do_it_again:
 	remove_wait_queue(&tty->read_wait, &wait);
 
 	if (!waitqueue_active(&tty->read_wait))
-		tty->minimum_to_wake = minimum;
+		ldata->minimum_to_wake = minimum;
 
 	__set_current_state(TASK_RUNNING);
 	size = b - buf;

@@ -2105,6 +2106,7 @@ break_out:
 static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 							poll_table *wait)
 {
+	struct n_tty_data *ldata = tty->disc_data;
 	unsigned int mask = 0;
 
 	poll_wait(file, &tty->read_wait, wait);

@@ -2119,9 +2121,9 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
 		mask |= POLLHUP;
 	if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
 		if (MIN_CHAR(tty) && !TIME_CHAR(tty))
-			tty->minimum_to_wake = MIN_CHAR(tty);
+			ldata->minimum_to_wake = MIN_CHAR(tty);
 		else
-			tty->minimum_to_wake = 1;
+			ldata->minimum_to_wake = 1;
 	}
 	if (tty->ops->write && !tty_is_writelocked(tty) &&
 			tty_chars_in_buffer(tty) < WAKEUP_CHARS &&

@@ -2169,6 +2171,18 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 	}
 }
 
+static void n_tty_fasync(struct tty_struct *tty, int on)
+{
+	struct n_tty_data *ldata = tty->disc_data;
+
+	if (!waitqueue_active(&tty->read_wait)) {
+		if (on)
+			ldata->minimum_to_wake = 1;
+		else if (!tty->fasync)
+			ldata->minimum_to_wake = N_TTY_BUF_SIZE;
+	}
+}
+
 struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.magic           = TTY_LDISC_MAGIC,
 	.name            = "n_tty",

@@ -2182,7 +2196,8 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
 	.set_termios     = n_tty_set_termios,
 	.poll            = n_tty_poll,
 	.receive_buf     = n_tty_receive_buf,
-	.write_wakeup    = n_tty_write_wakeup
+	.write_wakeup    = n_tty_write_wakeup,
+	.fasync		 = n_tty_fasync,
 };
 
 /**
drivers/tty/tty_io.c +9 -8
index 5ee51ba..6cd08d9 100644
@@ -2137,6 +2137,7 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
 static int __tty_fasync(int fd, struct file *filp, int on)
 {
 	struct tty_struct *tty = file_tty(filp);
+	struct tty_ldisc *ldisc;
 	unsigned long flags;
 	int retval = 0;
 

@@ -2147,11 +2148,17 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 	if (retval <= 0)
 		goto out;
 
+	ldisc = tty_ldisc_ref(tty);
+	if (ldisc) {
+		if (ldisc->ops->fasync)
+			ldisc->ops->fasync(tty, on);
+		tty_ldisc_deref(ldisc);
+	}
+
 	if (on) {
 		enum pid_type type;
 		struct pid *pid;
-		if (!waitqueue_active(&tty->read_wait))
-			tty->minimum_to_wake = 1;
+
 		spin_lock_irqsave(&tty->ctrl_lock, flags);
 		if (tty->pgrp) {
 			pid = tty->pgrp;

@@ -2164,13 +2171,7 @@ static int __tty_fasync(int fd, struct file *filp, int on)
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 		retval = __f_setown(filp, pid, type, 0);
 		put_pid(pid);
-		if (retval)
-			goto out;
-	} else {
-		if (!tty->fasync && !waitqueue_active(&tty->read_wait))
-			tty->minimum_to_wake = N_TTY_BUF_SIZE;
 	}
-	retval = 0;
 out:
 	return retval;
 }
include/linux/tty.h +0 -1
index a665c7e..7269daf 100644
@@ -272,7 +272,6 @@ struct tty_struct {
 #define N_TTY_BUF_SIZE 4096
 
 	unsigned char closing:1;
-	unsigned short minimum_to_wake;
 	unsigned char *write_buf;
 	int write_cnt;
 	/* If the tty has a pending do_SAK, queue it here - akpm */
include/linux/tty_ldisc.h +6 -0
index 22ee107..23bdd9d 100644
@@ -100,6 +100,11 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
+ * void (*fasync)(struct tty_struct *, int on)
+ *
+ *	Notify line discipline when signal-driven I/O is enabled or
+ *	disabled.
+ *
  * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
  *	Tells the discipline that the DCD pin has changed its status.

@@ -189,6 +194,7 @@ struct tty_ldisc_ops {
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
+	void	(*fasync)(struct tty_struct *tty, int on);
 
 	struct  module *owner;
Signature · 1 line
1.8.1.2
author
#16 lore raw

PATCH 2/4 n_tty: Untangle read completion variables

In reply to Peter Hurley
Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/tty/n_tty.c +2 -6
index f1806de..fa5cb46 100644
@@ -1801,20 +1801,16 @@ do_it_again:
 	minimum = time = 0;
 	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (!ldata->icanon) {
-		time = (HZ / 10) * TIME_CHAR(tty);
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
+			time = (HZ / 10) * TIME_CHAR(tty);
 			if (time)
 				ldata->minimum_to_wake = 1;
 			else if (!waitqueue_active(&tty->read_wait) ||
 				 (ldata->minimum_to_wake > minimum))
 				ldata->minimum_to_wake = minimum;
 		} else {
-			timeout = 0;
-			if (time) {
-				timeout = time;
-				time = 0;
-			}
+			timeout = (HZ / 10) * TIME_CHAR(tty);
 			ldata->minimum_to_wake = minimum = 1;
 		}
 	}
Signature · 1 line
1.8.1.2
author
#17 lore raw

PATCH 3/4 n_tty: Fix unsafe update of available buffer space

In reply to Peter Hurley

receive_room is used to control the amount of data the flip buffer work can push to the read buffer. This update is unsafe:

  CPU 0                        |  CPU 1
                               |
                               | n_tty_read()
                               |   n_tty_set_room()
                               |     left = <calc of space>
n_tty_receive_buf()            |
  <push data to buffer>        |
  n_tty_set_room()             |
    left = <calc of space>     |
    tty->receive_room = left   |
                               |     tty->receive_room = left

receive_room is now updated with a stale calculation of the available buffer space, and the subsequent work loop will likely overwrite unread data in the input buffer.

Update receive_room atomically with the calculation of the available buffer space.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/tty/n_tty.c +11 -7
index fa5cb46..0646165 100644
@@ -115,13 +115,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
 }
 
 /**
- *	n_tty_set__room	-	receive space
+ *	n_tty_set_room	-	receive space
  *	@tty: terminal
  *
- *	Called by the driver to find out how much data it is
- *	permitted to feed to the line discipline without any being lost
- *	and thus to manage flow control. Not serialized. Answers for the
- *	"instant".
+ *	Sets tty->receive_room to reflect the currently available space
+ *	in the input buffer, and re-schedules the flip buffer work if space
+ *	just became available.
+ *
+ *	Locks: Concurrent update is protected with read_lock
  */
 
 static void n_tty_set_room(struct tty_struct *tty)

@@ -129,8 +130,10 @@ static void n_tty_set_room(struct tty_struct *tty)
 	struct n_tty_data *ldata = tty->disc_data;
 	int left;
 	int old_left;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&ldata->read_lock, flags);
 
-	/* ldata->read_cnt is not read locked ? */
 	if (I_PARMRK(tty)) {
 		/* Multiply read_cnt by 3, since each byte might take up to
 		 * three times as many spaces when PARMRK is set (depending on

@@ -150,6 +153,8 @@ static void n_tty_set_room(struct tty_struct *tty)
 	old_left = tty->receive_room;
 	tty->receive_room = left;
 
+	raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+
 	/* Did this open up the receive buffer? We may need to flip */
 	if (left && !old_left) {
 		WARN_RATELIMIT(tty->port->itty == NULL,

@@ -1872,7 +1877,6 @@ do_it_again:
 				retval = -ERESTARTSYS;
 				break;
 			}
-			/* FIXME: does n_tty_set_room need locking ? */
 			n_tty_set_room(tty);
 			timeout = schedule_timeout(timeout);
 			continue;
Signature · 1 line
1.8.1.2
author
#18 lore raw

PATCH 4/4 n_tty: Buffer work should not reschedule itself

In reply to Peter Hurley

Although the driver-side input path must update the available buffer space, it should not reschedule itself. If space is still available and the flip buffers are not empty, flush_to_ldisc() will loop again.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/tty/n_tty.c +9 -4
index 0646165..4bf0fc0 100644
@@ -118,14 +118,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
  *	n_tty_set_room	-	receive space
  *	@tty: terminal
  *
- *	Sets tty->receive_room to reflect the currently available space
+ *	Updates tty->receive_room to reflect the currently available space
  *	in the input buffer, and re-schedules the flip buffer work if space
  *	just became available.
  *
  *	Locks: Concurrent update is protected with read_lock
  */
 
-static void n_tty_set_room(struct tty_struct *tty)
+static int set_room(struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 	int left;

@@ -155,8 +155,13 @@ static void n_tty_set_room(struct tty_struct *tty)
 
 	raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
 
+	return left && !old_left;
+}
+
+static void n_tty_set_room(struct tty_struct *tty)
+{
 	/* Did this open up the receive buffer? We may need to flip */
-	if (left && !old_left) {
+	if (set_room(tty)) {
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
 		/* see if ldisc has been killed - if so, this means that

@@ -1459,7 +1464,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 			tty->ops->flush_chars(tty);
 	}
 
-	n_tty_set_room(tty);
+	set_room(tty);
 
 	if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
Signature · 1 line
1.8.1.2
maintainer
#19 lore raw
In reply to Peter Hurley
Quoted 25 lines · Peter Hurley
Quoted 24 lines · Peter Hurley
Quoted 23 lines · Greg Kroah-Hartman
Quoted 10 lines · Peter Hurley
Quoted 9 lines

minimum_to_wake is unique to N_TTY processing, and belongs in per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines when signal-driven I/O is enabled or disabled. When enabled for N_TTY (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken for any readable input. When disabled, blocking reader/polls are not woken until the read buffer is full.

Quoted 7 lines
Quoted 4 lines

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by the minimum_to_wake setting.

Signed-off-by Peter Hurley <[email protected]>

For some reason, this patch doesn't apply. Care to refresh this one, and the rest in this series, and resend?

Sorry. There was probably some accidental dependency on one of the other patchsets of mine you did apply.

This and patch 5 now apply without error to tty-next.

Quoted 7 lines
Quoted 2 lines

Ok, but they are now long gone from my queue. Can you please resend what I haven't applied?

Greg,

I resent these back on 19 Mar but they never got applied. (maybe because I resent them as 4,5,6 & 7/7 ??)

Yes, they were burried, sorry about that :)

Quoted 1 line

Anyway, these apply cleanly to tty-next.

Now applied, thanks.

greg k-h

author
#20 lore raw

PATCH 5/7 n_tty: Untangle read completion variables

In reply to Peter Hurley
Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/tty/n_tty.c +2 -6
index 3fd657b..b2f621f 100644
@@ -1814,20 +1814,16 @@ do_it_again:
 	minimum = time = 0;
 	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (!ldata->icanon) {
-		time = (HZ / 10) * TIME_CHAR(tty);
 		minimum = MIN_CHAR(tty);
 		if (minimum) {
+			time = (HZ / 10) * TIME_CHAR(tty);
 			if (time)
 				ldata->minimum_to_wake = 1;
 			else if (!waitqueue_active(&tty->read_wait) ||
 				 (ldata->minimum_to_wake > minimum))
 				ldata->minimum_to_wake = minimum;
 		} else {
-			timeout = 0;
-			if (time) {
-				timeout = time;
-				time = 0;
-			}
+			timeout = (HZ / 10) * TIME_CHAR(tty);
 			ldata->minimum_to_wake = minimum = 1;
 		}
 	}
Signature · 1 line
1.8.1.2
author
#21 lore raw

PATCH 6/7 n_tty: Fix unsafe update of available buffer space

In reply to Peter Hurley

receive_room is used to control the amount of data the flip buffer work can push to the read buffer. This update is unsafe:

  CPU 0                        |  CPU 1
                               |
                               | n_tty_read()
                               |   n_tty_set_room()
                               |     left = <calc of space>
n_tty_receive_buf()            |
  <push data to buffer>        |
  n_tty_set_room()             |
    left = <calc of space>     |
    tty->receive_room = left   |
                               |     tty->receive_room = left

receive_room is now updated with a stale calculation of the available buffer space, and the subsequent work loop will likely overwrite unread data in the input buffer.

Update receive_room atomically with the calculation of the available buffer space.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/tty/n_tty.c +11 -7
index b2f621f..61a55f4 100644
@@ -115,13 +115,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
 }
 
 /**
- *	n_tty_set__room	-	receive space
+ *	n_tty_set_room	-	receive space
  *	@tty: terminal
  *
- *	Called by the driver to find out how much data it is
- *	permitted to feed to the line discipline without any being lost
- *	and thus to manage flow control. Not serialized. Answers for the
- *	"instant".
+ *	Sets tty->receive_room to reflect the currently available space
+ *	in the input buffer, and re-schedules the flip buffer work if space
+ *	just became available.
+ *
+ *	Locks: Concurrent update is protected with read_lock
  */
 
 static void n_tty_set_room(struct tty_struct *tty)

@@ -129,8 +130,10 @@ static void n_tty_set_room(struct tty_struct *tty)
 	struct n_tty_data *ldata = tty->disc_data;
 	int left;
 	int old_left;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&ldata->read_lock, flags);
 
-	/* ldata->read_cnt is not read locked ? */
 	if (I_PARMRK(tty)) {
 		/* Multiply read_cnt by 3, since each byte might take up to
 		 * three times as many spaces when PARMRK is set (depending on

@@ -150,6 +153,8 @@ static void n_tty_set_room(struct tty_struct *tty)
 	old_left = tty->receive_room;
 	tty->receive_room = left;
 
+	raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+
 	/* Did this open up the receive buffer? We may need to flip */
 	if (left && !old_left) {
 		WARN_RATELIMIT(tty->port->itty == NULL,

@@ -1885,7 +1890,6 @@ do_it_again:
 				retval = -ERESTARTSYS;
 				break;
 			}
-			/* FIXME: does n_tty_set_room need locking ? */
 			n_tty_set_room(tty);
 			timeout = schedule_timeout(timeout);
 			continue;
Signature · 1 line
1.8.1.2
author
#22 lore raw

PATCH 7/7 n_tty: Buffer work should not reschedule itself

In reply to Peter Hurley

Although the driver-side input path must update the available buffer space, it should not reschedule itself. If space is still available and the flip buffers are not empty, flush_to_ldisc() will loop again.

Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/n_tty.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/tty/n_tty.c +9 -4
index 61a55f4..6f9694d 100644
@@ -118,14 +118,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
  *	n_tty_set_room	-	receive space
  *	@tty: terminal
  *
- *	Sets tty->receive_room to reflect the currently available space
+ *	Updates tty->receive_room to reflect the currently available space
  *	in the input buffer, and re-schedules the flip buffer work if space
  *	just became available.
  *
  *	Locks: Concurrent update is protected with read_lock
  */
 
-static void n_tty_set_room(struct tty_struct *tty)
+static int set_room(struct tty_struct *tty)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 	int left;

@@ -155,8 +155,13 @@ static void n_tty_set_room(struct tty_struct *tty)
 
 	raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
 
+	return left && !old_left;
+}
+
+static void n_tty_set_room(struct tty_struct *tty)
+{
 	/* Did this open up the receive buffer? We may need to flip */
-	if (left && !old_left) {
+	if (set_room(tty)) {
 		WARN_RATELIMIT(tty->port->itty == NULL,
 				"scheduling with invalid itty\n");
 		/* see if ldisc has been killed - if so, this means that

@@ -1475,7 +1480,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 			tty->ops->flush_chars(tty);
 	}
 
-	n_tty_set_room(tty);
+	set_room(tty);
 
 	if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
 		L_EXTPROC(tty)) {
Signature · 1 line
1.8.1.2