From 6054c16e80318d32ee084a0f5620a863ae8cea33 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Thu, 16 Oct 2014 15:33:25 -0400 Subject: tty: Use spin_lock_irq() for ctrl_lock when interrupts enabled Interrupts are enabled in the n_tty_read() loop, ioctl(TIOCPKT) and pty driver flush_buffer() routine; no need to save and restore local interrupt state. Signed-off-by: Peter Hurley Reviewed-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 89c4cee253e3..d521058ee55a 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2128,7 +2128,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, int minimum, time; ssize_t retval = 0; long timeout; - unsigned long flags; int packet; c = job_control(tty, file); @@ -2174,10 +2173,10 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, unsigned char cs; if (b != buf) break; - spin_lock_irqsave(&tty->link->ctrl_lock, flags); + spin_lock_irq(&tty->link->ctrl_lock); cs = tty->link->ctrl_status; tty->link->ctrl_status = 0; - spin_unlock_irqrestore(&tty->link->ctrl_lock, flags); + spin_unlock_irq(&tty->link->ctrl_lock); if (tty_put_user(tty, cs, b++)) { retval = -EFAULT; b--; -- cgit v1.2.3 From 54e8e5fcaae109f0303f52efb24f29bfac79ca86 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Thu, 16 Oct 2014 15:33:26 -0400 Subject: pty: Don't claim slave's ctrl_lock for master's packet mode The slave's ctrl_lock serializes updates to the ctrl_status field only, whereas the master's ctrl_lock serializes updates to the packet mode enable (ie., the master does not have ctrl_status and the slave does not have packet mode). Thus, claiming the slave's ctrl_lock to access ->packet is useless. Unlocked reads of ->packet are already smp-safe. Signed-off-by: Peter Hurley Reviewed-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 4 ++-- drivers/tty/pty.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index d521058ee55a..cd725cc6e21e 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -351,13 +351,13 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty) { unsigned long flags; - spin_lock_irqsave(&tty->ctrl_lock, flags); if (tty->link->packet) { + spin_lock_irqsave(&tty->ctrl_lock, flags); tty->ctrl_status |= TIOCPKT_FLUSHREAD; + spin_unlock_irqrestore(&tty->ctrl_lock, flags); if (waitqueue_active(&tty->link->read_wait)) wake_up_interruptible(&tty->link->read_wait); } - spin_unlock_irqrestore(&tty->ctrl_lock, flags); } /** diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 29b5eeb01856..e554393d5551 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -339,26 +339,26 @@ static void pty_start(struct tty_struct *tty) { unsigned long flags; - spin_lock_irqsave(&tty->ctrl_lock, flags); if (tty->link && tty->link->packet) { + spin_lock_irqsave(&tty->ctrl_lock, flags); tty->ctrl_status &= ~TIOCPKT_STOP; tty->ctrl_status |= TIOCPKT_START; + spin_unlock_irqrestore(&tty->ctrl_lock, flags); wake_up_interruptible_poll(&tty->link->read_wait, POLLIN); } - spin_unlock_irqrestore(&tty->ctrl_lock, flags); } static void pty_stop(struct tty_struct *tty) { unsigned long flags; - spin_lock_irqsave(&tty->ctrl_lock, flags); if (tty->link && tty->link->packet) { + spin_lock_irqsave(&tty->ctrl_lock, flags); tty->ctrl_status &= ~TIOCPKT_START; tty->ctrl_status |= TIOCPKT_STOP; + spin_unlock_irqrestore(&tty->ctrl_lock, flags); wake_up_interruptible_poll(&tty->link->read_wait, POLLIN); } - spin_unlock_irqrestore(&tty->ctrl_lock, flags); } /** -- cgit v1.2.3 From 1aa1bf1115272d22fc4c758ee8769b73d8079a79 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Thu, 16 Oct 2014 15:33:29 -0400 Subject: tty: Fix missed wakeup from packet mode status update The pty master read() can miss the wake up for a packet mode status change. For example, CPU 0 | CPU 1 n_tty_read() | n_tty_packet_mode_flush() ... | . if (packet & link->ctrl_status) { | . /* no new ctrl_status ATM */ | . | spin_lock | ctrl_status |= TIOCPKT_FLUSHREAD | spin_unlock | wake_up(link->read_wait) } | set_current_state(TASK_INTERRUPTIBLE) | ... | The pty master read() will now sleep (assuming there is no input) having missed the read_wait wakeup. Set the task state before the condition test. Signed-off-by: Peter Hurley Reviewed-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index cd725cc6e21e..7f134b977c36 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2168,6 +2168,11 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, add_wait_queue(&tty->read_wait, &wait); while (nr) { + /* This statement must be first before checking for input + so that any interrupt will set the state back to + TASK_RUNNING. */ + set_current_state(TASK_INTERRUPTIBLE); + /* First test for status change. */ if (packet && tty->link->ctrl_status) { unsigned char cs; @@ -2185,10 +2190,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, nr--; break; } - /* This statement must be first before checking for input - so that any interrupt will set the state back to - TASK_RUNNING. */ - set_current_state(TASK_INTERRUPTIBLE); if (((minimum - (b - buf)) < ldata->minimum_to_wake) && ((minimum - (b - buf)) >= 1)) -- cgit v1.2.3 From 95ea90db019561450826ac32d8341f5735c78eee Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Thu, 16 Oct 2014 15:33:30 -0400 Subject: n_tty: Only process packet mode data in raw mode Packet mode can only be set for a pty master, and a pty master is always in raw mode since its termios cannot be changed. Signed-off-by: Peter Hurley Reviewed-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 7f134b977c36..6dcaa5294f1e 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2228,16 +2228,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, } __set_current_state(TASK_RUNNING); - /* Deal with packet mode. */ - if (packet && b == buf) { - if (tty_put_user(tty, TIOCPKT_DATA, b++)) { - retval = -EFAULT; - b--; - break; - } - nr--; - } - if (ldata->icanon && !L_EXTPROC(tty)) { retval = canon_copy_from_read_buf(tty, &b, &nr); if (retval == -EAGAIN) { @@ -2247,6 +2237,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, break; } else { int uncopied; + + /* Deal with packet mode. */ + if (packet && b == buf) { + if (tty_put_user(tty, TIOCPKT_DATA, b++)) { + retval = -EFAULT; + b--; + break; + } + nr--; + } + /* The copy function takes the read lock and handles locking internally for this case */ uncopied = copy_from_read_buf(tty, &b, &nr); -- cgit v1.2.3 From fa59e25664fc2a9e0fec494e921480e37b9e3c1c Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Thu, 16 Oct 2014 15:36:38 -0400 Subject: n_tty: Remove stale read lock comment The stale comment refers to lock behavior which was eliminated in commit 6d76bd2618535c581f1673047b8341fd291abc67, n_tty: Make N_TTY ldisc receive path lockless. Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 6dcaa5294f1e..0cb0e12f87c7 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2248,8 +2248,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, nr--; } - /* The copy function takes the read lock and handles - locking internally for this case */ uncopied = copy_from_read_buf(tty, &b, &nr); uncopied += copy_from_read_buf(tty, &b, &nr); if (uncopied) { -- cgit v1.2.3 From 52bce7f8d4fc633c9a9d0646eef58ba6ae9a3b73 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 5 Nov 2014 12:13:05 -0500 Subject: pty, n_tty: Simplify input processing on final close When releasing one end of a pty pair, that end may just have written to the other, which the input processing worker, flush_to_ldisc(), is still working on but has not completed the copy to the other end's read buffer. So input may not appear to be available to a waiting reader but yet TTY_OTHER_CLOSED is now observed. The n_tty line discipline has worked around this by waiting for input processing to complete and then re-checking if input is available before exiting with -EIO. Since the tty/ldisc lock reordering, the wait for input processing to complete can now occur during final close before setting TTY_OTHER_CLOSED. In this way, a waiting reader is guaranteed to see input available (if any) before observing TTY_OTHER_CLOSED. Reviewed-by: Alan Cox Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 46 ++++++++++++++++++++-------------------------- drivers/tty/pty.c | 1 + 2 files changed, 21 insertions(+), 26 deletions(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 0cb0e12f87c7..112eda7c56bc 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2197,34 +2197,28 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, if (!input_available_p(tty, 0)) { if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) { - up_read(&tty->termios_rwsem); - tty_flush_to_ldisc(tty); - down_read(&tty->termios_rwsem); - if (!input_available_p(tty, 0)) { - retval = -EIO; - break; - } - } else { - 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; - } - n_tty_set_room(tty); - up_read(&tty->termios_rwsem); + 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; + } + n_tty_set_room(tty); + up_read(&tty->termios_rwsem); - timeout = schedule_timeout(timeout); + timeout = schedule_timeout(timeout); - down_read(&tty->termios_rwsem); - continue; - } + down_read(&tty->termios_rwsem); + continue; } __set_current_state(TASK_RUNNING); diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index bee9776730fd..a9d256d6e909 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -53,6 +53,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp) /* Review - krefs on tty_link ?? */ if (!tty->link) return; + tty_flush_to_ldisc(tty->link); set_bit(TTY_OTHER_CLOSED, &tty->link->flags); wake_up_interruptible(&tty->link->read_wait); wake_up_interruptible(&tty->link->write_wait); -- cgit v1.2.3 From 8bfbe2de769afda051c56aba5450391670e769fc Mon Sep 17 00:00:00 2001 From: Christian Riesch Date: Thu, 13 Nov 2014 05:53:26 +0100 Subject: n_tty: Fix read_buf race condition, increment read_head after pushing data Commit 19e2ad6a09f0c06dbca19c98e5f4584269d913dd ("n_tty: Remove overflow tests from receive_buf() path") moved the increment of read_head into the arguments list of read_buf_addr(). Function calls represent a sequence point in C. Therefore read_head is incremented before the character c is placed in the buffer. Since the circular read buffer is a lock-less design since commit 6d76bd2618535c581f1673047b8341fd291abc67 ("n_tty: Make N_TTY ldisc receive path lockless"), this creates a race condition that leads to communication errors. This patch modifies the code to increment read_head _after_ the data is placed in the buffer and thus fixes the race for non-SMP machines. To fix the problem for SMP machines, memory barriers must be added in a separate patch. Signed-off-by: Christian Riesch Cc: Signed-off-by: Greg Kroah-Hartman --- drivers/tty/n_tty.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/tty/n_tty.c') diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 8358daa865e5..9e3b21624c82 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -321,7 +321,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty) static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata) { - *read_buf_addr(ldata, ldata->read_head++) = c; + *read_buf_addr(ldata, ldata->read_head) = c; + ldata->read_head++; } /** -- cgit v1.2.3