Hi Greg,
This series addresses some pecularities of the pty driver, especially
set_termios() handling, which is moved into the pty driver proper, and
packet mode/ctrl_lock behavior.
Regards,
Peter Hurley (9):
tty: WARN for attempted set_termios() of pty master
tty: Move pty-specific set_termios() handling to pty driver
pty: Use spin_lock_irq() for pty_set_termios()
tty: Use spin_lock_irq() for ctrl_lock when interrupts enabled
pty: Don't claim slave's ctrl_lock for master's packet mode
pty: Fix packet mode setting race
pty: Hold ctrl_lock for packet mode updates
tty: Fix missed wakeup from packet mode status update
n_tty: Only process packet mode data in raw mode
drivers/tty/n_tty.c | 39 +++++++++++++++++++------------------
drivers/tty/pty.c | 51 ++++++++++++++++++++++++++++++++++++-------------
drivers/tty/tty_ioctl.c | 34 ++++-----------------------------
3 files changed, 62 insertions(+), 62 deletions(-)
--
2.1.1
Updates to the packet mode enable require holding the ctrl_lock;
the serialization prevents corruption of adjacent fields.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/pty.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index bcec4c7..7a1a538 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -47,7 +47,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
set_bit(TTY_IO_ERROR, &tty->flags);
wake_up_interruptible(&tty->read_wait);
wake_up_interruptible(&tty->write_wait);
+ spin_lock_irq(&tty->ctrl_lock);
tty->packet = 0;
+ spin_unlock_irq(&tty->ctrl_lock);
/* Review - krefs on tty_link ?? */
if (!tty->link)
return;
--
2.1.1
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 <[email protected]>
---
drivers/tty/n_tty.c | 4 ++--
drivers/tty/pty.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 53cdfbe..6a42204 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 29b5eeb..e554393 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);
}
/**
--
2.1.1
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 <[email protected]>
---
drivers/tty/n_tty.c | 5 ++---
drivers/tty/pty.c | 10 ++++------
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index f44f1ba..53cdfbe 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--;
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 3943747..29b5eeb 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -178,13 +178,12 @@ static int pty_get_lock(struct tty_struct *tty, int __user *arg)
/* Set the packet mode on a pty */
static int pty_set_pktmode(struct tty_struct *tty, int __user *arg)
{
- unsigned long flags;
int pktmode;
if (get_user(pktmode, arg))
return -EFAULT;
- spin_lock_irqsave(&tty->ctrl_lock, flags);
+ spin_lock_irq(&tty->ctrl_lock);
if (pktmode) {
if (!tty->packet) {
tty->packet = 1;
@@ -192,7 +191,7 @@ static int pty_set_pktmode(struct tty_struct *tty, int __user *arg)
}
} else
tty->packet = 0;
- spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+ spin_unlock_irq(&tty->ctrl_lock);
return 0;
}
@@ -221,16 +220,15 @@ static int pty_signal(struct tty_struct *tty, int sig)
static void pty_flush_buffer(struct tty_struct *tty)
{
struct tty_struct *to = tty->link;
- unsigned long flags;
if (!to)
return;
/* tty_buffer_flush(to); FIXME */
if (to->packet) {
- spin_lock_irqsave(&tty->ctrl_lock, flags);
+ spin_lock_irq(&tty->ctrl_lock);
tty->ctrl_status |= TIOCPKT_FLUSHWRITE;
wake_up_interruptible(&to->read_wait);
- spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+ spin_unlock_irq(&tty->ctrl_lock);
}
}
--
2.1.1
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 <[email protected]>
---
drivers/tty/n_tty.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 602dbc2..379f60c 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);
--
2.1.1
Because pty_set_pktmode() does not claim the slave's ctrl_lock
to clear ->ctrl_status (to avoid unnecessary lock nesting),
pty_set_pktmode() may accidentally erase new ->ctrl_status updates.
For example,
CPU 0 | CPU 1
pty_set_pktmode() | pty_start()
spin_lock(master's ctrl_lock) |
tty->packet = 1 |
| if (tty->link->packet)
| spin_lock(slave's ctrl_lock)
| tty->ctrl_status = TIOCPKT_START
tty->link->ctrl_status = 0 |
Ensure the clear of ->ctrl_status occurs before packet mode is set
(and observable on another cpu).
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/pty.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e554393..bcec4c7 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -186,8 +186,9 @@ static int pty_set_pktmode(struct tty_struct *tty, int __user *arg)
spin_lock_irq(&tty->ctrl_lock);
if (pktmode) {
if (!tty->packet) {
- tty->packet = 1;
tty->link->ctrl_status = 0;
+ smp_mb();
+ tty->packet = 1;
}
} else
tty->packet = 0;
--
2.1.1
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 <[email protected]>
---
drivers/tty/n_tty.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 6a42204..602dbc2 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))
--
2.1.1
The tty driver's set_termios() method is called with interrupts
enabled; there is no need to save and restore the local interrupt state.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/pty.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 7f612c5..3943747 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -259,8 +259,6 @@ out:
static void pty_set_termios(struct tty_struct *tty,
struct ktermios *old_termios)
{
- unsigned long flags;
-
/* See if packet mode change of state. */
if (tty->link && tty->link->packet) {
int extproc = (old_termios->c_lflag & EXTPROC) |
@@ -272,7 +270,7 @@ static void pty_set_termios(struct tty_struct *tty,
STOP_CHAR(tty) == '\023' &&
START_CHAR(tty) == '\021');
if ((old_flow != new_flow) || extproc) {
- spin_lock_irqsave(&tty->ctrl_lock, flags);
+ spin_lock_irq(&tty->ctrl_lock);
if (old_flow != new_flow) {
tty->ctrl_status &= ~(TIOCPKT_DOSTOP | TIOCPKT_NOSTOP);
if (new_flow)
@@ -282,7 +280,7 @@ static void pty_set_termios(struct tty_struct *tty,
}
if (extproc)
tty->ctrl_status |= TIOCPKT_IOCTL;
- spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+ spin_unlock_irq(&tty->ctrl_lock);
wake_up_interruptible(&tty->link->read_wait);
}
}
--
2.1.1
The pty master's termios should never be set; currently, all code
paths which call the driver's set_termios() method ensure that the
pty slave's termios is being set.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/pty.c | 2 --
drivers/tty/tty_ioctl.c | 4 ++++
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 4c0218d..6c76025 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -477,7 +477,6 @@ static const struct tty_operations master_pty_ops_bsd = {
.flush_buffer = pty_flush_buffer,
.chars_in_buffer = pty_chars_in_buffer,
.unthrottle = pty_unthrottle,
- .set_termios = pty_set_termios,
.ioctl = pty_bsd_ioctl,
.cleanup = pty_cleanup,
.resize = pty_resize,
@@ -654,7 +653,6 @@ static const struct tty_operations ptm_unix98_ops = {
.flush_buffer = pty_flush_buffer,
.chars_in_buffer = pty_chars_in_buffer,
.unthrottle = pty_unthrottle,
- .set_termios = pty_set_termios,
.ioctl = pty_unix98_ioctl,
.resize = pty_resize,
.shutdown = pty_unix98_shutdown,
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 32cee97..f29e3d1 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -528,6 +528,8 @@ EXPORT_SYMBOL(tty_termios_hw_change);
* is a bit of layering violation here with n_tty in terms of the
* internal knowledge of this function.
*
+ * A master pty's termios should never be set.
+ *
* Locking: termios_rwsem
*/
@@ -537,6 +539,8 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
struct tty_ldisc *ld;
unsigned long flags;
+ WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+ tty->driver->subtype == PTY_TYPE_MASTER);
/*
* Perform the actual termios internal changes under lock.
*/
--
2.1.1
Packet mode is unique to the pty driver; move the packet mode state
change code from the generic tty ioctl handler to the pty driver.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/pty.c | 28 ++++++++++++++++++++++++++++
drivers/tty/tty_ioctl.c | 32 +-------------------------------
2 files changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 6c76025..7f612c5 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -259,6 +259,34 @@ out:
static void pty_set_termios(struct tty_struct *tty,
struct ktermios *old_termios)
{
+ unsigned long flags;
+
+ /* See if packet mode change of state. */
+ if (tty->link && tty->link->packet) {
+ int extproc = (old_termios->c_lflag & EXTPROC) |
+ (tty->termios.c_lflag & EXTPROC);
+ int old_flow = ((old_termios->c_iflag & IXON) &&
+ (old_termios->c_cc[VSTOP] == '\023') &&
+ (old_termios->c_cc[VSTART] == '\021'));
+ int new_flow = (I_IXON(tty) &&
+ STOP_CHAR(tty) == '\023' &&
+ START_CHAR(tty) == '\021');
+ if ((old_flow != new_flow) || extproc) {
+ spin_lock_irqsave(&tty->ctrl_lock, flags);
+ if (old_flow != new_flow) {
+ tty->ctrl_status &= ~(TIOCPKT_DOSTOP | TIOCPKT_NOSTOP);
+ if (new_flow)
+ tty->ctrl_status |= TIOCPKT_DOSTOP;
+ else
+ tty->ctrl_status |= TIOCPKT_NOSTOP;
+ }
+ if (extproc)
+ tty->ctrl_status |= TIOCPKT_IOCTL;
+ spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+ wake_up_interruptible(&tty->link->read_wait);
+ }
+ }
+
tty->termios.c_cflag &= ~(CSIZE | PARENB);
tty->termios.c_cflag |= (CS8 | CREAD);
}
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index f29e3d1..e2484e4 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -524,10 +524,7 @@ EXPORT_SYMBOL(tty_termios_hw_change);
* @tty: tty to update
* @new_termios: desired new value
*
- * Perform updates to the termios values set on this terminal. There
- * is a bit of layering violation here with n_tty in terms of the
- * internal knowledge of this function.
- *
+ * Perform updates to the termios values set on this terminal.
* A master pty's termios should never be set.
*
* Locking: termios_rwsem
@@ -537,7 +534,6 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
{
struct ktermios old_termios;
struct tty_ldisc *ld;
- unsigned long flags;
WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
tty->driver->subtype == PTY_TYPE_MASTER);
@@ -553,32 +549,6 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
tty->termios = *new_termios;
unset_locked_termios(&tty->termios, &old_termios, &tty->termios_locked);
- /* See if packet mode change of state. */
- if (tty->link && tty->link->packet) {
- int extproc = (old_termios.c_lflag & EXTPROC) |
- (tty->termios.c_lflag & EXTPROC);
- int old_flow = ((old_termios.c_iflag & IXON) &&
- (old_termios.c_cc[VSTOP] == '\023') &&
- (old_termios.c_cc[VSTART] == '\021'));
- int new_flow = (I_IXON(tty) &&
- STOP_CHAR(tty) == '\023' &&
- START_CHAR(tty) == '\021');
- if ((old_flow != new_flow) || extproc) {
- spin_lock_irqsave(&tty->ctrl_lock, flags);
- if (old_flow != new_flow) {
- tty->ctrl_status &= ~(TIOCPKT_DOSTOP | TIOCPKT_NOSTOP);
- if (new_flow)
- tty->ctrl_status |= TIOCPKT_DOSTOP;
- else
- tty->ctrl_status |= TIOCPKT_NOSTOP;
- }
- if (extproc)
- tty->ctrl_status |= TIOCPKT_IOCTL;
- spin_unlock_irqrestore(&tty->ctrl_lock, flags);
- wake_up_interruptible(&tty->link->read_wait);
- }
- }
-
if (tty->ops->set_termios)
(*tty->ops->set_termios)(tty, &old_termios);
else
--
2.1.1
On Thu, 16 Oct 2014 15:33:21 -0400
Peter Hurley <[email protected]> wrote:
> Hi Greg,
>
> This series addresses some pecularities of the pty driver, especially
> set_termios() handling, which is moved into the pty driver proper, and
> packet mode/ctrl_lock behavior.
>
> Regards,
>
> Peter Hurley (9):
> tty: WARN for attempted set_termios() of pty master
> tty: Move pty-specific set_termios() handling to pty driver
> pty: Use spin_lock_irq() for pty_set_termios()
> tty: Use spin_lock_irq() for ctrl_lock when interrupts enabled
> pty: Don't claim slave's ctrl_lock for master's packet mode
> pty: Fix packet mode setting race
> pty: Hold ctrl_lock for packet mode updates
> tty: Fix missed wakeup from packet mode status update
> n_tty: Only process packet mode data in raw mode
>
> drivers/tty/n_tty.c | 39 +++++++++++++++++++------------------
> drivers/tty/pty.c | 51 ++++++++++++++++++++++++++++++++++++-------------
> drivers/tty/tty_ioctl.c | 34 ++++-----------------------------
> 3 files changed, 62 insertions(+), 62 deletions(-)
>
Reviewed-by: Alan Cox <[email protected]>