From 38db89799bdf11625a831c5af33938dcb11908b6 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Thu, 11 Jun 2009 12:44:17 +0100 Subject: tty: throttling race fix The tty throttling code can race due to the lock drops. It takes very high loads but this has been observed and verified by Rob Duncan. The basic problem is that on an SMP box we can go CPU #1 CPU #2 need to throttle ? suppose we should buffer space cleared are we throttled yes ? - unthrottle call throttle method This changeet take the termios lock to protect against this. The termios lock isn't the initial obvious candidate but many implementations of throttle methods already need to poke around their own termios structures (and nobody really locks them against a racing change of flow control). This does mean that anyone who is setting tty->low_latency = 1 and then calling tty_flip_buffer_push from their unthrottle method is going to end up collapsing in a pile of locks. However we've removed all the known bogus users of low_latency = 1 and such use isn't safe anyway for other reasons so catching it would be an improvement. Signed-off-by: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/tty_ioctl.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'drivers/char/tty_ioctl.c') diff --git a/drivers/char/tty_ioctl.c b/drivers/char/tty_ioctl.c index 6f4c7d0a53b..2401dbcbee9 100644 --- a/drivers/char/tty_ioctl.c +++ b/drivers/char/tty_ioctl.c @@ -97,14 +97,19 @@ EXPORT_SYMBOL(tty_driver_flush_buffer); * @tty: terminal * * Indicate that a tty should stop transmitting data down the stack. + * Takes the termios mutex to protect against parallel throttle/unthrottle + * and also to ensure the driver can consistently reference its own + * termios data at this point when implementing software flow control. */ void tty_throttle(struct tty_struct *tty) { + mutex_lock(&tty->termios_mutex); /* check TTY_THROTTLED first so it indicates our state */ if (!test_and_set_bit(TTY_THROTTLED, &tty->flags) && tty->ops->throttle) tty->ops->throttle(tty); + mutex_unlock(&tty->termios_mutex); } EXPORT_SYMBOL(tty_throttle); @@ -113,13 +118,21 @@ EXPORT_SYMBOL(tty_throttle); * @tty: terminal * * Indicate that a tty may continue transmitting data down the stack. + * Takes the termios mutex to protect against parallel throttle/unthrottle + * and also to ensure the driver can consistently reference its own + * termios data at this point when implementing software flow control. + * + * Drivers should however remember that the stack can issue a throttle, + * then change flow control method, then unthrottle. */ void tty_unthrottle(struct tty_struct *tty) { + mutex_lock(&tty->termios_mutex); if (test_and_clear_bit(TTY_THROTTLED, &tty->flags) && tty->ops->unthrottle) tty->ops->unthrottle(tty); + mutex_unlock(&tty->termios_mutex); } EXPORT_SYMBOL(tty_unthrottle); -- cgit v1.2.3-70-g09d2 From 26a2e20f4a966bebc312836aeb4423fbfd4e33e2 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Thu, 11 Jun 2009 14:03:13 +0100 Subject: tty: Untangle termios and mm mutex dependencies Although this doesn't cause any problems it could potentially do so for future mmap using devices. No real work is needed to sort it out so untangle it before it causes problems Signed-off-by: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/tty_ioctl.c | 75 +++++++++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 29 deletions(-) (limited to 'drivers/char/tty_ioctl.c') diff --git a/drivers/char/tty_ioctl.c b/drivers/char/tty_ioctl.c index 2401dbcbee9..8116bb1c8f8 100644 --- a/drivers/char/tty_ioctl.c +++ b/drivers/char/tty_ioctl.c @@ -626,9 +626,25 @@ static int set_termios(struct tty_struct *tty, void __user *arg, int opt) return 0; } +static void copy_termios(struct tty_struct *tty, struct ktermios *kterm) +{ + mutex_lock(&tty->termios_mutex); + memcpy(kterm, tty->termios, sizeof(struct ktermios)); + mutex_unlock(&tty->termios_mutex); +} + +static void copy_termios_locked(struct tty_struct *tty, struct ktermios *kterm) +{ + mutex_lock(&tty->termios_mutex); + memcpy(kterm, tty->termios_locked, sizeof(struct ktermios)); + mutex_unlock(&tty->termios_mutex); +} + static int get_termio(struct tty_struct *tty, struct termio __user *termio) { - if (kernel_termios_to_user_termio(termio, tty->termios)) + struct ktermios kterm; + copy_termios(tty, &kterm); + if (kernel_termios_to_user_termio(termio, &kterm)) return -EFAULT; return 0; } @@ -930,6 +946,8 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, struct tty_struct *real_tty; void __user *p = (void __user *)arg; int ret = 0; + struct ktermios kterm; + struct termiox ktermx; if (tty->driver->type == TTY_DRIVER_TYPE_PTY && tty->driver->subtype == PTY_TYPE_MASTER) @@ -965,23 +983,20 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, return set_termios(real_tty, p, TERMIOS_OLD); #ifndef TCGETS2 case TCGETS: - mutex_lock(&real_tty->termios_mutex); - if (kernel_termios_to_user_termios((struct termios __user *)arg, real_tty->termios)) + copy_termios(real_tty, &kterm); + if (kernel_termios_to_user_termios((struct termios __user *)arg, &kterm)) ret = -EFAULT; - mutex_unlock(&real_tty->termios_mutex); return ret; #else case TCGETS: - mutex_lock(&real_tty->termios_mutex); - if (kernel_termios_to_user_termios_1((struct termios __user *)arg, real_tty->termios)) + copy_termios(real_tty, &kterm); + if (kernel_termios_to_user_termios_1((struct termios __user *)arg, &kterm)) ret = -EFAULT; - mutex_unlock(&real_tty->termios_mutex); return ret; case TCGETS2: - mutex_lock(&real_tty->termios_mutex); - if (kernel_termios_to_user_termios((struct termios2 __user *)arg, real_tty->termios)) + copy_termios(real_tty, &kterm); + if (kernel_termios_to_user_termios((struct termios2 __user *)arg, &kterm)) ret = -EFAULT; - mutex_unlock(&real_tty->termios_mutex); return ret; case TCSETSF2: return set_termios(real_tty, p, TERMIOS_FLUSH | TERMIOS_WAIT); @@ -1000,34 +1015,36 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, return set_termios(real_tty, p, TERMIOS_TERMIO); #ifndef TCGETS2 case TIOCGLCKTRMIOS: - mutex_lock(&real_tty->termios_mutex); - if (kernel_termios_to_user_termios((struct termios __user *)arg, real_tty->termios_locked)) + copy_termios_locked(real_tty, &kterm); + if (kernel_termios_to_user_termios((struct termios __user *)arg, &kterm)) ret = -EFAULT; - mutex_unlock(&real_tty->termios_mutex); return ret; case TIOCSLCKTRMIOS: if (!capable(CAP_SYS_ADMIN)) return -EPERM; - mutex_lock(&real_tty->termios_mutex); - if (user_termios_to_kernel_termios(real_tty->termios_locked, + copy_termios_locked(real_tty, &kterm); + if (user_termios_to_kernel_termios(&kterm, (struct termios __user *) arg)) - ret = -EFAULT; + return -EFAULT; + mutex_lock(&real_tty->termios_mutex); + memcpy(real_tty->termios_locked, &kterm, sizeof(struct ktermios)); mutex_unlock(&real_tty->termios_mutex); - return ret; + return 0; #else case TIOCGLCKTRMIOS: - mutex_lock(&real_tty->termios_mutex); - if (kernel_termios_to_user_termios_1((struct termios __user *)arg, real_tty->termios_locked)) + copy_termios_locked(real_tty, &kterm); + if (kernel_termios_to_user_termios_1((struct termios __user *)arg, &kterm)) ret = -EFAULT; - mutex_unlock(&real_tty->termios_mutex); return ret; case TIOCSLCKTRMIOS: if (!capable(CAP_SYS_ADMIN)) - ret = -EPERM; - mutex_lock(&real_tty->termios_mutex); - if (user_termios_to_kernel_termios_1(real_tty->termios_locked, + return -EPERM; + copy_termios_locked(real_tty, &kterm); + if (user_termios_to_kernel_termios_1(&kterm, (struct termios __user *) arg)) - ret = -EFAULT; + return -EFAULT; + mutex_lock(&real_tty->termios_mutex); + memcpy(real_tty->termios_locked, &kterm, sizeof(struct ktermios)); mutex_unlock(&real_tty->termios_mutex); return ret; #endif @@ -1036,9 +1053,10 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, if (real_tty->termiox == NULL) return -EINVAL; mutex_lock(&real_tty->termios_mutex); - if (copy_to_user(p, real_tty->termiox, sizeof(struct termiox))) - ret = -EFAULT; + memcpy(&ktermx, real_tty->termiox, sizeof(struct termiox)); mutex_unlock(&real_tty->termios_mutex); + if (copy_to_user(p, &ktermx, sizeof(struct termiox))) + ret = -EFAULT; return ret; case TCSETX: return set_termiox(real_tty, p, 0); @@ -1048,10 +1066,9 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, return set_termiox(real_tty, p, TERMIOS_FLUSH); #endif case TIOCGSOFTCAR: - mutex_lock(&real_tty->termios_mutex); - ret = put_user(C_CLOCAL(real_tty) ? 1 : 0, + copy_termios(real_tty, &kterm); + ret = put_user((kterm.c_cflag & CLOCAL) ? 1 : 0, (int __user *)arg); - mutex_unlock(&real_tty->termios_mutex); return ret; case TIOCSSOFTCAR: if (get_user(arg, (unsigned int __user *) arg)) -- cgit v1.2.3-70-g09d2 From 5dca607bcf10d3f08d07ffeac664c6769c336145 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Tue, 16 Jun 2009 17:01:02 +0100 Subject: tty: fix unused warning when TCGETX is not defined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If TCGETX is not defined, we end up with this warning: drivers/char/tty_ioctl.c: In function ‘tty_mode_ioctl’: drivers/char/tty_ioctl.c:950: warning: unused variable ‘ktermx’ Since the variable is only used in one case statement, push it down to the local case scope. Signed-off-by: Mike Frysinger Signed-off-by: Alan Cox Signed-off-by: Linus Torvalds --- drivers/char/tty_ioctl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers/char/tty_ioctl.c') diff --git a/drivers/char/tty_ioctl.c b/drivers/char/tty_ioctl.c index 8116bb1c8f8..b24f6c6a1ea 100644 --- a/drivers/char/tty_ioctl.c +++ b/drivers/char/tty_ioctl.c @@ -947,7 +947,6 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, void __user *p = (void __user *)arg; int ret = 0; struct ktermios kterm; - struct termiox ktermx; if (tty->driver->type == TTY_DRIVER_TYPE_PTY && tty->driver->subtype == PTY_TYPE_MASTER) @@ -1049,7 +1048,8 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, return ret; #endif #ifdef TCGETX - case TCGETX: + case TCGETX: { + struct termiox ktermx; if (real_tty->termiox == NULL) return -EINVAL; mutex_lock(&real_tty->termios_mutex); @@ -1058,6 +1058,7 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file, if (copy_to_user(p, &ktermx, sizeof(struct termiox))) ret = -EFAULT; return ret; + } case TCSETX: return set_termiox(real_tty, p, 0); case TCSETXW: -- cgit v1.2.3-70-g09d2