From eeb89d918c2fa2b809e464136bbafdaec2aacb30 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Mon, 30 Nov 2009 13:18:29 +0000 Subject: tty: push the BKL down into the handlers a bit Start trying to untangle the remaining BKL mess Updated to fix missing unlock_kernel noted by Dan Carpenter Signed-off-by: Alan "I must be out of my tree" Cox Signed-off-by: Greg Kroah-Hartman --- drivers/char/tty_io.c | 141 ++++++++++++++++++++++++++++---------------------- 1 file changed, 80 insertions(+), 61 deletions(-) (limited to 'drivers/char/tty_io.c') diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index 59499ee0fe6..1e241309361 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -142,7 +142,6 @@ ssize_t redirected_tty_write(struct file *, const char __user *, size_t, loff_t *); static unsigned int tty_poll(struct file *, poll_table *); static int tty_open(struct inode *, struct file *); -static int tty_release(struct inode *, struct file *); long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg); #ifdef CONFIG_COMPAT static long tty_compat_ioctl(struct file *file, unsigned int cmd, @@ -1017,14 +1016,16 @@ out: void tty_write_message(struct tty_struct *tty, char *msg) { - lock_kernel(); if (tty) { mutex_lock(&tty->atomic_write_lock); - if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) + lock_kernel(); + if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) { + unlock_kernel(); tty->ops->write(tty, msg, strlen(msg)); + } else + unlock_kernel(); tty_write_unlock(tty); } - unlock_kernel(); return; } @@ -1202,14 +1203,21 @@ static int tty_driver_install_tty(struct tty_driver *driver, struct tty_struct *tty) { int idx = tty->index; + int ret; - if (driver->ops->install) - return driver->ops->install(driver, tty); + if (driver->ops->install) { + lock_kernel(); + ret = driver->ops->install(driver, tty); + unlock_kernel(); + return ret; + } if (tty_init_termios(tty) == 0) { + lock_kernel(); tty_driver_kref_get(driver); tty->count++; driver->ttys[idx] = tty; + unlock_kernel(); return 0; } return -ENOMEM; @@ -1302,10 +1310,14 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx, struct tty_struct *tty; int retval; + lock_kernel(); /* Check if pty master is being opened multiple times */ if (driver->subtype == PTY_TYPE_MASTER && - (driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) + (driver->flags & TTY_DRIVER_DEVPTS_MEM) && !first_ok) { + unlock_kernel(); return ERR_PTR(-EIO); + } + unlock_kernel(); /* * First time open is complex, especially for PTY devices. @@ -1335,8 +1347,9 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx, * If we fail here just call release_tty to clean up. No need * to decrement the use counts, as release_tty doesn't care. */ - + lock_kernel(); retval = tty_ldisc_setup(tty, tty->link); + unlock_kernel(); if (retval) goto release_mem_out; return tty; @@ -1350,7 +1363,9 @@ release_mem_out: if (printk_ratelimit()) printk(KERN_INFO "tty_init_dev: ldisc open failed, " "clearing slot %d\n", idx); + lock_kernel(); release_tty(tty, idx); + unlock_kernel(); return ERR_PTR(retval); } @@ -1464,7 +1479,17 @@ static void release_tty(struct tty_struct *tty, int idx) tty_kref_put(tty); } -/* +/** + * tty_release - vfs callback for close + * @inode: inode of tty + * @filp: file pointer for handle to tty + * + * Called the last time each file handle is closed that references + * this tty. There may however be several such references. + * + * Locking: + * Takes bkl. See tty_release_dev + * * Even releasing the tty structures is a tricky business.. We have * to be very careful that the structures are all released at the * same time, as interrupts might otherwise get the wrong pointers. @@ -1472,20 +1497,20 @@ static void release_tty(struct tty_struct *tty, int idx) * WSH 09/09/97: rewritten to avoid some nasty race conditions that could * lead to double frees or releasing memory still in use. */ -void tty_release_dev(struct file *filp) + +int tty_release(struct inode *inode, struct file *filp) { struct tty_struct *tty, *o_tty; int pty_master, tty_closing, o_tty_closing, do_sleep; int devpts; int idx; char buf[64]; - struct inode *inode; - inode = filp->f_path.dentry->d_inode; tty = (struct tty_struct *)filp->private_data; if (tty_paranoia_check(tty, inode, "tty_release_dev")) - return; + return 0; + lock_kernel(); check_tty_count(tty, "tty_release_dev"); tty_fasync(-1, filp, 0); @@ -1500,19 +1525,22 @@ void tty_release_dev(struct file *filp) if (idx < 0 || idx >= tty->driver->num) { printk(KERN_DEBUG "tty_release_dev: bad idx when trying to " "free (%s)\n", tty->name); - return; + unlock_kernel(); + return 0; } if (!devpts) { if (tty != tty->driver->ttys[idx]) { + unlock_kernel(); printk(KERN_DEBUG "tty_release_dev: driver.table[%d] not tty " "for (%s)\n", idx, tty->name); - return; + return 0; } if (tty->termios != tty->driver->termios[idx]) { + unlock_kernel(); printk(KERN_DEBUG "tty_release_dev: driver.termios[%d] not termios " "for (%s)\n", idx, tty->name); - return; + return 0; } } #endif @@ -1526,26 +1554,30 @@ void tty_release_dev(struct file *filp) if (tty->driver->other && !(tty->driver->flags & TTY_DRIVER_DEVPTS_MEM)) { if (o_tty != tty->driver->other->ttys[idx]) { + unlock_kernel(); printk(KERN_DEBUG "tty_release_dev: other->table[%d] " "not o_tty for (%s)\n", idx, tty->name); - return; + return 0 ; } if (o_tty->termios != tty->driver->other->termios[idx]) { + unlock_kernel(); printk(KERN_DEBUG "tty_release_dev: other->termios[%d] " "not o_termios for (%s)\n", idx, tty->name); - return; + return 0; } if (o_tty->link != tty) { + unlock_kernel(); printk(KERN_DEBUG "tty_release_dev: bad pty pointers\n"); - return; + return 0; } } #endif if (tty->ops->close) tty->ops->close(tty, filp); + unlock_kernel(); /* * Sanity check: if tty->count is going to zero, there shouldn't be * any waiters on tty->read_wait or tty->write_wait. We test the @@ -1568,6 +1600,7 @@ void tty_release_dev(struct file *filp) opens on /dev/tty */ mutex_lock(&tty_mutex); + lock_kernel(); tty_closing = tty->count <= 1; o_tty_closing = o_tty && (o_tty->count <= (pty_master ? 1 : 0)); @@ -1598,6 +1631,7 @@ void tty_release_dev(struct file *filp) printk(KERN_WARNING "tty_release_dev: %s: read/write wait queue " "active!\n", tty_name(tty, buf)); + unlock_kernel(); mutex_unlock(&tty_mutex); schedule(); } @@ -1661,8 +1695,10 @@ void tty_release_dev(struct file *filp) mutex_unlock(&tty_mutex); /* check whether both sides are closing ... */ - if (!tty_closing || (o_tty && !o_tty_closing)) - return; + if (!tty_closing || (o_tty && !o_tty_closing)) { + unlock_kernel(); + return 0; + } #ifdef TTY_DEBUG_HANGUP printk(KERN_DEBUG "freeing tty structure..."); @@ -1680,10 +1716,12 @@ void tty_release_dev(struct file *filp) /* Make this pty number available for reallocation */ if (devpts) devpts_kill_index(inode, idx); + unlock_kernel(); + return 0; } /** - * __tty_open - open a tty device + * tty_open - open a tty device * @inode: inode of device file * @filp: file pointer to tty * @@ -1703,7 +1741,7 @@ void tty_release_dev(struct file *filp) * ->siglock protects ->signal/->sighand */ -static int __tty_open(struct inode *inode, struct file *filp) +static int tty_open(struct inode *inode, struct file *filp) { struct tty_struct *tty = NULL; int noctty, retval; @@ -1720,10 +1758,12 @@ retry_open: retval = 0; mutex_lock(&tty_mutex); + lock_kernel(); if (device == MKDEV(TTYAUX_MAJOR, 0)) { tty = get_current_tty(); if (!tty) { + unlock_kernel(); mutex_unlock(&tty_mutex); return -ENXIO; } @@ -1755,12 +1795,14 @@ retry_open: goto got_driver; } } + unlock_kernel(); mutex_unlock(&tty_mutex); return -ENODEV; } driver = get_tty_driver(device, &index); if (!driver) { + unlock_kernel(); mutex_unlock(&tty_mutex); return -ENODEV; } @@ -1770,6 +1812,7 @@ got_driver: tty = tty_driver_lookup_tty(driver, inode, index); if (IS_ERR(tty)) { + unlock_kernel(); mutex_unlock(&tty_mutex); return PTR_ERR(tty); } @@ -1784,8 +1827,10 @@ got_driver: mutex_unlock(&tty_mutex); tty_driver_kref_put(driver); - if (IS_ERR(tty)) + if (IS_ERR(tty)) { + unlock_kernel(); return PTR_ERR(tty); + } filp->private_data = tty; file_move(filp, &tty->tty_files); @@ -1813,11 +1858,15 @@ got_driver: printk(KERN_DEBUG "error %d in opening %s...", retval, tty->name); #endif - tty_release_dev(filp); - if (retval != -ERESTARTSYS) + tty_release(inode, filp); + if (retval != -ERESTARTSYS) { + unlock_kernel(); return retval; - if (signal_pending(current)) + } + if (signal_pending(current)) { + unlock_kernel(); return retval; + } schedule(); /* * Need to reset f_op in case a hangup happened. @@ -1826,8 +1875,11 @@ got_driver: filp->f_op = &tty_fops; goto retry_open; } + unlock_kernel(); + mutex_lock(&tty_mutex); + lock_kernel(); spin_lock_irq(¤t->sighand->siglock); if (!noctty && current->signal->leader && @@ -1835,44 +1887,13 @@ got_driver: tty->session == NULL) __proc_set_tty(current, tty); spin_unlock_irq(¤t->sighand->siglock); + unlock_kernel(); mutex_unlock(&tty_mutex); return 0; } -/* BKL pushdown: scary code avoidance wrapper */ -static int tty_open(struct inode *inode, struct file *filp) -{ - int ret; - - lock_kernel(); - ret = __tty_open(inode, filp); - unlock_kernel(); - return ret; -} - - -/** - * tty_release - vfs callback for close - * @inode: inode of tty - * @filp: file pointer for handle to tty - * - * Called the last time each file handle is closed that references - * this tty. There may however be several such references. - * - * Locking: - * Takes bkl. See tty_release_dev - */ - -static int tty_release(struct inode *inode, struct file *filp) -{ - lock_kernel(); - tty_release_dev(filp); - unlock_kernel(); - return 0; -} - /** * tty_poll - check tty status * @filp: file being polled @@ -2317,9 +2338,7 @@ static int tiocsetd(struct tty_struct *tty, int __user *p) if (get_user(ldisc, p)) return -EFAULT; - lock_kernel(); ret = tty_set_ldisc(tty, ldisc); - unlock_kernel(); return ret; } -- cgit v1.2.3-70-g09d2 From f18f9498e90327b9b0e245e191029e6e1996d203 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Mon, 30 Nov 2009 13:18:35 +0000 Subject: tty: Push the lock down further into the ldisc code Signed-off-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/char/tty_io.c | 2 -- drivers/char/tty_ldisc.c | 12 +++++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) (limited to 'drivers/char/tty_io.c') diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index 1e241309361..c408c81c06a 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -1347,9 +1347,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx, * If we fail here just call release_tty to clean up. No need * to decrement the use counts, as release_tty doesn't care. */ - lock_kernel(); retval = tty_ldisc_setup(tty, tty->link); - unlock_kernel(); if (retval) goto release_mem_out; return tty; diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c index d914e77f7f0..3f653f7d849 100644 --- a/drivers/char/tty_ldisc.c +++ b/drivers/char/tty_ldisc.c @@ -445,8 +445,14 @@ static void tty_set_termios_ldisc(struct tty_struct *tty, int num) static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld) { WARN_ON(test_and_set_bit(TTY_LDISC_OPEN, &tty->flags)); - if (ld->ops->open) - return ld->ops->open(tty); + if (ld->ops->open) { + int ret; + /* BKL here locks verus a hangup event */ + lock_kernel(); + ret = ld->ops->open(tty); + unlock_kernel(); + return ret; + } return 0; } @@ -566,6 +572,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) return 0; } + unlock_kernel(); /* * Problem: What do we do if this blocks ? * We could deadlock here @@ -573,7 +580,6 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) tty_wait_until_sent(tty, 0); - unlock_kernel(); mutex_lock(&tty->ldisc_mutex); /* -- cgit v1.2.3-70-g09d2 From 38c70b27f9502c31c1d0c29676275f7362cdb0d9 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Mon, 30 Nov 2009 13:18:40 +0000 Subject: tty: Push the bkl down a bit in the hangup code We know that the redirect field is handled via its own locking in all places Signed-off-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/char/tty_io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/char/tty_io.c') diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index c408c81c06a..cc941a3504d 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -505,8 +505,6 @@ static void do_tty_hangup(struct work_struct *work) if (!tty) return; - /* inuse_filps is protected by the single kernel lock */ - lock_kernel(); spin_lock(&redirect_lock); if (redirect && redirect->private_data == tty) { @@ -515,6 +513,8 @@ static void do_tty_hangup(struct work_struct *work) } spin_unlock(&redirect_lock); + /* inuse_filps is protected by the single kernel lock */ + lock_kernel(); check_tty_count(tty, "do_tty_hangup"); file_list_lock(); /* This breaks for file handles being sent over AF_UNIX sockets ? */ -- cgit v1.2.3-70-g09d2 From 5ec93d1154fd1e269162398f8e70efc7e004485d Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Mon, 30 Nov 2009 13:18:45 +0000 Subject: tty: Move the leader test in disassociate There are two call points, both want to check that tty->signal->leader is set. Move the test into disassociate_ctty() as that will make locking changes easier in a bit Signed-off-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/char/tty_io.c | 5 +++-- kernel/exit.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) (limited to 'drivers/char/tty_io.c') diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index cc941a3504d..a19fef2093f 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -707,6 +707,8 @@ void disassociate_ctty(int on_exit) struct tty_struct *tty; struct pid *tty_pgrp = NULL; + if (!current->signal->leader) + return; tty = get_current_tty(); if (tty) { @@ -772,8 +774,7 @@ void no_tty(void) { struct task_struct *tsk = current; lock_kernel(); - if (tsk->signal->leader) - disassociate_ctty(0); + disassociate_ctty(0); unlock_kernel(); proc_clear_tty(tsk); } diff --git a/kernel/exit.c b/kernel/exit.c index 1143012951e..6f50ef55a6f 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -971,7 +971,7 @@ NORET_TYPE void do_exit(long code) exit_thread(); cgroup_exit(tsk, 1); - if (group_dead && tsk->signal->leader) + if (group_dead) disassociate_ctty(1); module_put(task_thread_info(tsk)->exec_domain->module); -- cgit v1.2.3-70-g09d2 From 36ba782e9674cdc29ec7003757df0b375e99fa96 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Mon, 30 Nov 2009 13:18:51 +0000 Subject: tty: split the lock up a bit further The tty count sanity check may need the BKL, that isn't clear. However it is clear that the count use of the lock is internal and independant of the bigger use of the lock. Furthermore the file list locking is also separately locked already Signed-off-by: Alan Cox Signed-off-by: Greg Kroah-Hartman --- drivers/char/tty_io.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/char/tty_io.c') diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index a19fef2093f..684f0e0b175 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -516,6 +516,8 @@ static void do_tty_hangup(struct work_struct *work) /* inuse_filps is protected by the single kernel lock */ lock_kernel(); check_tty_count(tty, "do_tty_hangup"); + unlock_kernel(); + file_list_lock(); /* This breaks for file handles being sent over AF_UNIX sockets ? */ list_for_each_entry(filp, &tty->tty_files, f_u.fu_list) { @@ -529,6 +531,7 @@ static void do_tty_hangup(struct work_struct *work) } file_list_unlock(); + lock_kernel(); tty_ldisc_hangup(tty); read_lock(&tasklist_lock); -- cgit v1.2.3-70-g09d2