From d7a855bd6ab25d10d5e3b6aeb53d9c57fa17b808 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 10 Sep 2014 15:06:30 -0400 Subject: tty: Convert tty_struct bitfield to ints The stopped, hw_stopped, flow_stopped and packet bits are smp-unsafe and interrupt-unsafe. For example, CPU 0 | CPU 1 | tty->flow_stopped = 1 | tty->hw_stopped = 0 One of these updates will be corrupted, as the bitwise operation on the bitfield is non-atomic. Ensure each flag has a separate memory location, so concurrent updates do not corrupt orthogonal states. Because DEC Alpha EV4 and EV5 cpus (from 1995) perform RMW on smaller-than-machine-word storage, "separate memory location" must be int instead of byte. Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- include/linux/tty.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'include/linux/tty.h') diff --git a/include/linux/tty.h b/include/linux/tty.h index 84132942902..4cfd4a82fc3 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -261,7 +261,10 @@ struct tty_struct { unsigned long flags; int count; struct winsize winsize; /* winsize_mutex */ - unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1; + int stopped; + int flow_stopped; + int hw_stopped; + int packet; unsigned char ctrl_status; /* ctrl_lock */ unsigned int receive_room; /* Bytes free for queue */ int flow_change; -- cgit v1.2.3-70-g09d2 From f9e053dcfc02b0ad29daec8524fb1afe09774976 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 10 Sep 2014 15:06:31 -0400 Subject: tty: Serialize tty flow control changes with flow_lock Without serialization, the flow control state can become inverted wrt. the actual hardware state. For example, CPU 0 | CPU 1 stop_tty() | lock ctrl_lock | tty->stopped = 1 | unlock ctrl_lock | | start_tty() | lock ctrl_lock | tty->stopped = 0 | unlock ctrl_lock | driver->start() driver->stop() | In this case, the flow control state now indicates the tty has been started, but the actual hardware state has actually been stopped. Introduce tty->flow_lock spinlock to serialize tty flow control changes. Split out unlocked __start_tty()/__stop_tty() flavors for use by ioctl(TCXONC) in follow-on patch. Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- drivers/tty/tty_io.c | 39 ++++++++++++++++++++++++++++----------- include/linux/tty.h | 5 ++++- include/linux/tty_driver.h | 4 ++++ 3 files changed, 36 insertions(+), 12 deletions(-) (limited to 'include/linux/tty.h') diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index d4eb2a8b704..b8ddfef6b5d 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -919,18 +919,18 @@ void no_tty(void) * but not always. * * Locking: - * Uses the tty control lock internally + * ctrl_lock + * flow_lock */ -void stop_tty(struct tty_struct *tty) +void __stop_tty(struct tty_struct *tty) { unsigned long flags; - spin_lock_irqsave(&tty->ctrl_lock, flags); - if (tty->stopped) { - spin_unlock_irqrestore(&tty->ctrl_lock, flags); + + if (tty->stopped) return; - } tty->stopped = 1; + spin_lock_irqsave(&tty->ctrl_lock, flags); if (tty->link && tty->link->packet) { tty->ctrl_status &= ~TIOCPKT_START; tty->ctrl_status |= TIOCPKT_STOP; @@ -941,6 +941,14 @@ void stop_tty(struct tty_struct *tty) (tty->ops->stop)(tty); } +void stop_tty(struct tty_struct *tty) +{ + unsigned long flags; + + spin_lock_irqsave(&tty->flow_lock, flags); + __stop_tty(tty); + spin_unlock_irqrestore(&tty->flow_lock, flags); +} EXPORT_SYMBOL(stop_tty); /** @@ -954,17 +962,17 @@ EXPORT_SYMBOL(stop_tty); * * Locking: * ctrl_lock + * flow_lock */ -void start_tty(struct tty_struct *tty) +void __start_tty(struct tty_struct *tty) { unsigned long flags; - spin_lock_irqsave(&tty->ctrl_lock, flags); - if (!tty->stopped || tty->flow_stopped) { - spin_unlock_irqrestore(&tty->ctrl_lock, flags); + + if (!tty->stopped || tty->flow_stopped) return; - } tty->stopped = 0; + spin_lock_irqsave(&tty->ctrl_lock, flags); if (tty->link && tty->link->packet) { tty->ctrl_status &= ~TIOCPKT_STOP; tty->ctrl_status |= TIOCPKT_START; @@ -977,6 +985,14 @@ void start_tty(struct tty_struct *tty) tty_wakeup(tty); } +void start_tty(struct tty_struct *tty) +{ + unsigned long flags; + + spin_lock_irqsave(&tty->flow_lock, flags); + __start_tty(tty); + spin_unlock_irqrestore(&tty->flow_lock, flags); +} EXPORT_SYMBOL(start_tty); /* We limit tty time update visibility to every 8 seconds or so. */ @@ -3019,6 +3035,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx) INIT_WORK(&tty->hangup_work, do_tty_hangup); mutex_init(&tty->atomic_write_lock); spin_lock_init(&tty->ctrl_lock); + spin_lock_init(&tty->flow_lock); INIT_LIST_HEAD(&tty->tty_files); INIT_WORK(&tty->SAK_work, do_SAK_work); diff --git a/include/linux/tty.h b/include/linux/tty.h index 4cfd4a82fc3..fd4148d3a26 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -252,6 +252,7 @@ struct tty_struct { struct rw_semaphore termios_rwsem; struct mutex winsize_mutex; spinlock_t ctrl_lock; + spinlock_t flow_lock; /* Termios values are protected by the termios rwsem */ struct ktermios termios, termios_locked; struct termiox *termiox; /* May be NULL for unsupported */ @@ -261,7 +262,7 @@ struct tty_struct { unsigned long flags; int count; struct winsize winsize; /* winsize_mutex */ - int stopped; + int stopped; /* flow_lock */ int flow_stopped; int hw_stopped; int packet; @@ -400,7 +401,9 @@ extern int tty_paranoia_check(struct tty_struct *tty, struct inode *inode, extern char *tty_name(struct tty_struct *tty, char *buf); extern void tty_wait_until_sent(struct tty_struct *tty, long timeout); extern int tty_check_change(struct tty_struct *tty); +extern void __stop_tty(struct tty_struct *tty); extern void stop_tty(struct tty_struct *tty); +extern void __start_tty(struct tty_struct *tty); extern void start_tty(struct tty_struct *tty); extern int tty_register_driver(struct tty_driver *driver); extern int tty_unregister_driver(struct tty_driver *driver); diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h index e48c608a8fa..92e337c1883 100644 --- a/include/linux/tty_driver.h +++ b/include/linux/tty_driver.h @@ -152,6 +152,8 @@ * This routine notifies the tty driver that it should stop * outputting characters to the tty device. * + * Called with ->flow_lock held. Serialized with start() method. + * * Optional: * * Note: Call stop_tty not this method. @@ -161,6 +163,8 @@ * This routine notifies the tty driver that it resume sending * characters to the tty device. * + * Called with ->flow_lock held. Serialized with stop() method. + * * Optional: * * Note: Call start_tty not this method. -- cgit v1.2.3-70-g09d2 From c545b66c6922b002b5fe224a6eaec58c913650b5 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 10 Sep 2014 15:06:33 -0400 Subject: tty: Serialize tcflow() with other tty flow control changes Use newly-introduced tty->flow_lock to serialize updates to tty->flow_stopped (via tcflow()) and with concurrent tty flow control changes from other sources. Merge the storage for ->stopped and ->flow_stopped, now that both flags are serialized by ->flow_lock. The padding bits are necessary to force the compiler to allocate the type specified; otherwise, gcc will ignore the type specifier and allocate the minimum number of bytes necessary to store the bitfield. In turn, this would allow Alpha EV4 and EV5 cpus to corrupt adjacent byte storage because those cpus use RMW to store byte and short data. gcc versions < 4.7.2 will also corrupt storage adjacent to bitfields smaller than unsigned long on ia64, ppc64, hppa64 and sparc64, thus requiring more than unsigned int storage (which would otherwise be sufficient to workaround the Alpha non-atomic byte/short storage problem). Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- drivers/tty/tty_ioctl.c | 8 ++++++-- include/linux/tty.h | 5 +++-- 2 files changed, 9 insertions(+), 4 deletions(-) (limited to 'include/linux/tty.h') diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c index cd1285c3bfe..dcf5c0af7de 100644 --- a/drivers/tty/tty_ioctl.c +++ b/drivers/tty/tty_ioctl.c @@ -1177,16 +1177,20 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file, return retval; switch (arg) { case TCOOFF: + spin_lock_irq(&tty->flow_lock); if (!tty->flow_stopped) { tty->flow_stopped = 1; - stop_tty(tty); + __stop_tty(tty); } + spin_unlock_irq(&tty->flow_lock); break; case TCOON: + spin_lock_irq(&tty->flow_lock); if (tty->flow_stopped) { tty->flow_stopped = 0; - start_tty(tty); + __start_tty(tty); } + spin_unlock_irq(&tty->flow_lock); break; case TCIOFF: if (STOP_CHAR(tty) != __DISABLED_CHAR) diff --git a/include/linux/tty.h b/include/linux/tty.h index fd4148d3a26..d80b53642f2 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -262,8 +262,9 @@ struct tty_struct { unsigned long flags; int count; struct winsize winsize; /* winsize_mutex */ - int stopped; /* flow_lock */ - int flow_stopped; + unsigned long stopped:1, /* flow_lock */ + flow_stopped:1, + unused:62; int hw_stopped; int packet; unsigned char ctrl_status; /* ctrl_lock */ -- cgit v1.2.3-70-g09d2 From 136d5258b2bc4ffae99cb69874a76624c26fbfad Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 10 Sep 2014 15:06:34 -0400 Subject: tty: Move and rename send_prio_char() as tty_send_xchar() Relocate the file-scope function, send_prio_char(), as a global helper tty_send_xchar(). Remove the global declarations for tty_write_lock()/tty_write_unlock(), as these are file-scope only now. Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- drivers/tty/tty_io.c | 33 +++++++++++++++++++++++++++++++-- drivers/tty/tty_ioctl.c | 33 ++------------------------------- include/linux/tty.h | 3 +-- 3 files changed, 34 insertions(+), 35 deletions(-) (limited to 'include/linux/tty.h') diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index c249ff1d51c..2f6f9b5e489 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -1023,14 +1023,14 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count, return i; } -void tty_write_unlock(struct tty_struct *tty) +static void tty_write_unlock(struct tty_struct *tty) __releases(&tty->atomic_write_lock) { mutex_unlock(&tty->atomic_write_lock); wake_up_interruptible_poll(&tty->write_wait, POLLOUT); } -int tty_write_lock(struct tty_struct *tty, int ndelay) +static int tty_write_lock(struct tty_struct *tty, int ndelay) __acquires(&tty->atomic_write_lock) { if (!mutex_trylock(&tty->atomic_write_lock)) { @@ -1217,6 +1217,35 @@ ssize_t redirected_tty_write(struct file *file, const char __user *buf, return tty_write(file, buf, count, ppos); } +/** + * tty_send_xchar - send priority character + * + * Send a high priority character to the tty even if stopped + * + * Locking: none for xchar method, write ordering for write method. + */ + +int tty_send_xchar(struct tty_struct *tty, char ch) +{ + int was_stopped = tty->stopped; + + if (tty->ops->send_xchar) { + tty->ops->send_xchar(tty, ch); + return 0; + } + + if (tty_write_lock(tty, 0) < 0) + return -ERESTARTSYS; + + if (was_stopped) + start_tty(tty); + tty->ops->write(tty, &ch, 1); + if (was_stopped) + stop_tty(tty); + tty_write_unlock(tty); + return 0; +} + static char ptychar[] = "pqrstuvwxyzabcde"; /** diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c index dcf5c0af7de..ad9120d1c0f 100644 --- a/drivers/tty/tty_ioctl.c +++ b/drivers/tty/tty_ioctl.c @@ -911,35 +911,6 @@ static int set_ltchars(struct tty_struct *tty, struct ltchars __user *ltchars) } #endif -/** - * send_prio_char - send priority character - * - * Send a high priority character to the tty even if stopped - * - * Locking: none for xchar method, write ordering for write method. - */ - -static int send_prio_char(struct tty_struct *tty, char ch) -{ - int was_stopped = tty->stopped; - - if (tty->ops->send_xchar) { - tty->ops->send_xchar(tty, ch); - return 0; - } - - if (tty_write_lock(tty, 0) < 0) - return -ERESTARTSYS; - - if (was_stopped) - start_tty(tty); - tty->ops->write(tty, &ch, 1); - if (was_stopped) - stop_tty(tty); - tty_write_unlock(tty); - return 0; -} - /** * tty_change_softcar - carrier change ioctl helper * @tty: tty to update @@ -1194,11 +1165,11 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file, break; case TCIOFF: if (STOP_CHAR(tty) != __DISABLED_CHAR) - return send_prio_char(tty, STOP_CHAR(tty)); + return tty_send_xchar(tty, STOP_CHAR(tty)); break; case TCION: if (START_CHAR(tty) != __DISABLED_CHAR) - return send_prio_char(tty, START_CHAR(tty)); + return tty_send_xchar(tty, START_CHAR(tty)); break; default: return -EINVAL; diff --git a/include/linux/tty.h b/include/linux/tty.h index d80b53642f2..4c6b9fd3c75 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -418,6 +418,7 @@ extern void tty_unregister_device(struct tty_driver *driver, unsigned index); extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp, int buflen); extern void tty_write_message(struct tty_struct *tty, char *msg); +extern int tty_send_xchar(struct tty_struct *tty, char ch); extern int tty_put_char(struct tty_struct *tty, unsigned char c); extern int tty_chars_in_buffer(struct tty_struct *tty); extern int tty_write_room(struct tty_struct *tty); @@ -502,8 +503,6 @@ extern struct tty_struct *tty_pair_get_pty(struct tty_struct *tty); extern struct mutex tty_mutex; extern spinlock_t tty_files_lock; -extern void tty_write_unlock(struct tty_struct *tty); -extern int tty_write_lock(struct tty_struct *tty, int ndelay); #define tty_is_writelocked(tty) (mutex_is_locked(&tty->atomic_write_lock)) extern void tty_port_init(struct tty_port *port); -- cgit v1.2.3-70-g09d2 From 99416322dd16b810ba74098cc50ef2a844091d35 Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 10 Sep 2014 15:06:36 -0400 Subject: tty: Workaround Alpha non-atomic byte storage in tty_struct The Alpha EV4/EV5 cpus can corrupt adjacent byte and short data because those cpus use RMW to store byte and short data. Thus, concurrent adjacent byte stores could become corrupted, if serialized by a different lock. tty_struct uses different locks to protect certain fields within the structure, and thus is vulnerable to byte stores which are not atomic. Merge the ->ctrl_status byte and packet mode bit, both protected by the ->ctrl_lock, into an unsigned long. The padding bits are necessary to force the compiler to allocate the type specified; otherwise, gcc will ignore the type specifier and allocate the minimum number of bytes required to store the bitfield. In turn, this would allow Alpha EV4/EV5 cpus to corrupt adjacent byte or short storage (because those cpus use RMW to store byte and short data). gcc versions < 4.7.2 will also corrupt storage adjacent to bitfields smaller than unsigned long on ia64, ppc64, hppa64, and sparc64, thus requiring more than unsigned int storage (which would otherwise be sufficient to fix the Alpha non-atomic storage problem). Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- include/linux/tty.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'include/linux/tty.h') diff --git a/include/linux/tty.h b/include/linux/tty.h index 4c6b9fd3c75..546e9455789 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -266,8 +266,9 @@ struct tty_struct { flow_stopped:1, unused:62; int hw_stopped; - int packet; - unsigned char ctrl_status; /* ctrl_lock */ + unsigned long ctrl_status:8, /* ctrl_lock */ + packet:1, + unused_ctrl:55; unsigned int receive_room; /* Bytes free for queue */ int flow_change; -- cgit v1.2.3-70-g09d2 From cc952e7017fa2e8871ee6a94f2c606ff5911f61e Mon Sep 17 00:00:00 2001 From: Peter Hurley Date: Wed, 24 Sep 2014 06:26:21 -0400 Subject: tty: Fix width of unsigned long bitfield padding Commit c545b66c6922b002b5fe224a6eaec58c913650b5, 'tty: Serialize tcflow() with other tty flow control changes' and commit 99416322dd16b810ba74098cc50ef2a844091d35, 'tty: Workaround Alpha non-atomic byte storage in tty_struct' work around compiler bugs and non-atomic storage on multiple arches by padding bitfields out to the declared type which is unsigned long. However, the width varies by arch. Pad bitfields to actual width of unsigned long (which is BITS_PER_LONG). Reported-by: Fengguang Wu Signed-off-by: Peter Hurley Signed-off-by: Greg Kroah-Hartman --- include/linux/tty.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include/linux/tty.h') diff --git a/include/linux/tty.h b/include/linux/tty.h index 546e9455789..5171ef8f7b8 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -264,11 +264,11 @@ struct tty_struct { struct winsize winsize; /* winsize_mutex */ unsigned long stopped:1, /* flow_lock */ flow_stopped:1, - unused:62; + unused:BITS_PER_LONG - 2; int hw_stopped; unsigned long ctrl_status:8, /* ctrl_lock */ packet:1, - unused_ctrl:55; + unused_ctrl:BITS_PER_LONG - 9; unsigned int receive_room; /* Bytes free for queue */ int flow_change; -- cgit v1.2.3-70-g09d2