* [PATCH 1/3] tty_lock: undo the old tty_lock use on the ctty @ 2012-05-03 21:21 Alan Cox 2012-05-03 21:22 ` [PATCH 2/3] pty: Lock the devpts bits privately Alan Cox 2012-05-03 21:24 ` [PATCH 3/3] tty_lock: Localise the lock Alan Cox 0 siblings, 2 replies; 21+ messages in thread From: Alan Cox @ 2012-05-03 21:21 UTC (permalink / raw) To: linux-kernel, linux-serial From: Alan Cox <alan@linux.intel.com> get_current_tty has its own consistent locking. That means a pile of the tty lock cases are not needed. As get_current_tty also keeps a reference the tty object lifetime means we can propogate the lock removal out. Signed-off-by: Alan Cox <alan@linux.intel.com> --- drivers/tty/tty_io.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index d939bd7..b425c79 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -855,10 +855,11 @@ void disassociate_ctty(int on_exit) */ void no_tty(void) { + /* FIXME: Review locking here. The tty_lock never covered any race + between a new association and proc_clear_tty but possible we need + to protect against this anyway */ struct task_struct *tsk = current; - tty_lock(); disassociate_ctty(0); - tty_unlock(); proc_clear_tty(tsk); } @@ -1800,6 +1801,9 @@ int tty_release(struct inode *inode, struct file *filp) * * We cannot return driver and index like for the other nodes because * devpts will not work then. It expects inodes to be from devpts FS. + * + * We need to move to returning a refcounted object from all the lookup + * paths including this one. */ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp) { @@ -1816,6 +1820,7 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp) /* noctty = 1; */ tty_kref_put(tty); /* FIXME: we put a reference and return a TTY! */ + /* This is only safe because the caller holds tty_mutex */ return tty; } ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] pty: Lock the devpts bits privately 2012-05-03 21:21 [PATCH 1/3] tty_lock: undo the old tty_lock use on the ctty Alan Cox @ 2012-05-03 21:22 ` Alan Cox 2012-05-08 18:18 ` H. Peter Anvin 2012-05-03 21:24 ` [PATCH 3/3] tty_lock: Localise the lock Alan Cox 1 sibling, 1 reply; 21+ messages in thread From: Alan Cox @ 2012-05-03 21:22 UTC (permalink / raw) To: linux-kernel, linux-serial From: Alan Cox <alan@linux.intel.com> This is a private pty affair, we don't want to tangle it with the tty_lock any more as we know all the other non tty locking is now handled by the vfs so we too can move. Signed-off-by: Alan Cox <alan@linux.intel.com> --- drivers/tty/pty.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index eeae7fa..5505ffc 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -26,11 +26,13 @@ #include <linux/bitops.h> #include <linux/devpts_fs.h> #include <linux/slab.h> +#include <linux/mutex.h> #ifdef CONFIG_UNIX98_PTYS static struct tty_driver *ptm_driver; static struct tty_driver *pts_driver; +static DEFINE_MUTEX(devpts_mutex); #endif static void pty_close(struct tty_struct *tty, struct file *filp) @@ -54,8 +56,11 @@ static void pty_close(struct tty_struct *tty, struct file *filp) if (tty->driver->subtype == PTY_TYPE_MASTER) { set_bit(TTY_OTHER_CLOSED, &tty->flags); #ifdef CONFIG_UNIX98_PTYS - if (tty->driver == ptm_driver) + if (tty->driver == ptm_driver) { + mutex_lock(&devpts_mutex); devpts_pty_kill(tty->link); + mutex_unlock(&devpts_mutex); + } #endif tty_unlock(); tty_vhangup(tty->link); @@ -475,13 +480,17 @@ static struct tty_struct *ptm_unix98_lookup(struct tty_driver *driver, * @idx: tty index * * Look up a pty master device. Called under the tty_mutex for now. - * This provides our locking. + * This provides our locking for the tty pointer. */ static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver, struct inode *pts_inode, int idx) { - struct tty_struct *tty = devpts_get_tty(pts_inode, idx); + struct tty_struct *tty; + + mutex_lock(&devpts_mutex); + tty = devpts_get_tty(pts_inode, idx); + mutex_unlock(&devpts_mutex); /* Master must be open before slave */ if (!tty) return ERR_PTR(-EIO); @@ -622,8 +631,10 @@ static int ptmx_open(struct inode *inode, struct file *filp) } mutex_lock(&tty_mutex); - tty_lock(); + mutex_lock(&devpts_mutex); tty = tty_init_dev(ptm_driver, index); + mutex_unlock(&devpts_mutex); + tty_lock(); mutex_unlock(&tty_mutex); if (IS_ERR(tty)) { ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] pty: Lock the devpts bits privately 2012-05-03 21:22 ` [PATCH 2/3] pty: Lock the devpts bits privately Alan Cox @ 2012-05-08 18:18 ` H. Peter Anvin 2012-05-08 20:43 ` Alan Cox 0 siblings, 1 reply; 21+ messages in thread From: H. Peter Anvin @ 2012-05-08 18:18 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial On 05/03/2012 02:22 PM, Alan Cox wrote: > From: Alan Cox <alan@linux.intel.com> > > This is a private pty affair, we don't want to tangle it with the tty_lock > any more as we know all the other non tty locking is now handled by the vfs > so we too can move. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > + mutex_lock(&devpts_mutex); > devpts_pty_kill(tty->link); > + mutex_unlock(&devpts_mutex); > + mutex_lock(&devpts_mutex); > + tty = devpts_get_tty(pts_inode, idx); > + mutex_unlock(&devpts_mutex); > + mutex_lock(&devpts_mutex); > tty = tty_init_dev(ptm_driver, index); > + mutex_unlock(&devpts_mutex); Conceptually this seems fine, but it would seem cleaner to me to push this mutex into the called functions in devpts; I suspect the lock could be eliminated or at least be made per instance there (which would make massive-container people happy...) -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] pty: Lock the devpts bits privately 2012-05-08 18:18 ` H. Peter Anvin @ 2012-05-08 20:43 ` Alan Cox 2012-05-08 20:41 ` H. Peter Anvin 0 siblings, 1 reply; 21+ messages in thread From: Alan Cox @ 2012-05-08 20:43 UTC (permalink / raw) To: H. Peter Anvin; +Cc: linux-kernel, linux-serial On Tue, 08 May 2012 11:18:35 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote: > On 05/03/2012 02:22 PM, Alan Cox wrote: > > From: Alan Cox <alan@linux.intel.com> > > > > This is a private pty affair, we don't want to tangle it with the tty_lock > > any more as we know all the other non tty locking is now handled by the vfs > > so we too can move. > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > > + mutex_lock(&devpts_mutex); > > devpts_pty_kill(tty->link); > > + mutex_unlock(&devpts_mutex); > > > + mutex_lock(&devpts_mutex); > > + tty = devpts_get_tty(pts_inode, idx); > > + mutex_unlock(&devpts_mutex); > > > + mutex_lock(&devpts_mutex); > > tty = tty_init_dev(ptm_driver, index); > > + mutex_unlock(&devpts_mutex); > > Conceptually this seems fine, but it would seem cleaner to me to push > this mutex into the called functions in devpts; I suspect the lock could > be eliminated or at least be made per instance there (which would make > massive-container people happy...) One step at a time. I agree entirely that the ideal case is that devpts_foo is internally locked and coherent. That is an exercise for someone who likes devpts 8) Alan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] pty: Lock the devpts bits privately 2012-05-08 20:43 ` Alan Cox @ 2012-05-08 20:41 ` H. Peter Anvin 0 siblings, 0 replies; 21+ messages in thread From: H. Peter Anvin @ 2012-05-08 20:41 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial On 05/08/2012 01:43 PM, Alan Cox wrote: > > One step at a time. I agree entirely that the ideal case is that > devpts_foo is internally locked and coherent. That is an exercise for > someone who likes devpts 8) > OK. I can look at that later; this should detach it enough from the tty locking that the rest is easy. -hpa ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] tty_lock: Localise the lock 2012-05-03 21:21 [PATCH 1/3] tty_lock: undo the old tty_lock use on the ctty Alan Cox 2012-05-03 21:22 ` [PATCH 2/3] pty: Lock the devpts bits privately Alan Cox @ 2012-05-03 21:24 ` Alan Cox 2012-05-04 20:48 ` Arnd Bergmann ` (2 more replies) 1 sibling, 3 replies; 21+ messages in thread From: Alan Cox @ 2012-05-03 21:24 UTC (permalink / raw) To: linux-kernel, linux-serial From: Alan Cox <alan@linux.intel.com> In each remaining case the tty_lock is associated with a specific tty. This means we can now lock on a per tty basis. We do need tty_lock_pair() for the pty case. Uglier but still a step in the right direction. Signed-off-by: Alan Cox <alan@linux.intel.com> --- drivers/tty/amiserial.c | 12 ++++---- drivers/tty/cyclades.c | 2 + drivers/tty/n_r3964.c | 11 ++++--- drivers/tty/pty.c | 23 +++++++++------ drivers/tty/synclink.c | 4 +-- drivers/tty/synclink_gt.c | 4 +-- drivers/tty/synclinkmp.c | 4 +-- drivers/tty/tty_io.c | 67 +++++++++++++++++++++++++++------------------ drivers/tty/tty_ldisc.c | 30 ++++++++++---------- drivers/tty/tty_mutex.c | 60 ++++++++++++++++++++++++++++++---------- drivers/tty/tty_port.c | 6 ++-- include/linux/tty.h | 23 +++++++++------ 12 files changed, 149 insertions(+), 97 deletions(-) diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c index 6cc4358..b88a65c 100644 --- a/drivers/tty/amiserial.c +++ b/drivers/tty/amiserial.c @@ -1033,7 +1033,7 @@ static int get_serial_info(struct tty_struct *tty, struct serial_state *state, if (!retinfo) return -EFAULT; memset(&tmp, 0, sizeof(tmp)); - tty_lock(); + tty_lock(tty); tmp.line = tty->index; tmp.port = state->port; tmp.flags = state->tport.flags; @@ -1042,7 +1042,7 @@ static int get_serial_info(struct tty_struct *tty, struct serial_state *state, tmp.close_delay = state->tport.close_delay; tmp.closing_wait = state->tport.closing_wait; tmp.custom_divisor = state->custom_divisor; - tty_unlock(); + tty_unlock(tty); if (copy_to_user(retinfo,&tmp,sizeof(*retinfo))) return -EFAULT; return 0; @@ -1059,12 +1059,12 @@ static int set_serial_info(struct tty_struct *tty, struct serial_state *state, if (copy_from_user(&new_serial,new_info,sizeof(new_serial))) return -EFAULT; - tty_lock(); + tty_lock(tty); change_spd = ((new_serial.flags ^ port->flags) & ASYNC_SPD_MASK) || new_serial.custom_divisor != state->custom_divisor; if (new_serial.irq || new_serial.port != state->port || new_serial.xmit_fifo_size != state->xmit_fifo_size) { - tty_unlock(); + tty_unlock(tty); return -EINVAL; } @@ -1084,7 +1084,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_state *state, } if (new_serial.baud_base < 9600) { - tty_unlock(); + tty_unlock(tty); return -EINVAL; } @@ -1116,7 +1116,7 @@ check_and_exit: } } else retval = startup(tty, state); - tty_unlock(); + tty_unlock(tty); return retval; } diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c index e61cabd..6984e1a 100644 --- a/drivers/tty/cyclades.c +++ b/drivers/tty/cyclades.c @@ -1599,7 +1599,7 @@ static int cy_open(struct tty_struct *tty, struct file *filp) * If the port is the middle of closing, bail out now */ if (tty_hung_up_p(filp) || (info->port.flags & ASYNC_CLOSING)) { - wait_event_interruptible_tty(info->port.close_wait, + wait_event_interruptible_tty(tty, info->port.close_wait, !(info->port.flags & ASYNC_CLOSING)); return (info->port.flags & ASYNC_HUP_NOTIFY) ? -EAGAIN: -ERESTARTSYS; } diff --git a/drivers/tty/n_r3964.c b/drivers/tty/n_r3964.c index 5c6c314..656ad93 100644 --- a/drivers/tty/n_r3964.c +++ b/drivers/tty/n_r3964.c @@ -1065,7 +1065,8 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file, TRACE_L("read()"); - tty_lock(); + /* FIXME: should use a private lock */ + tty_lock(tty); pClient = findClient(pInfo, task_pid(current)); if (pClient) { @@ -1077,7 +1078,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file, goto unlock; } /* block until there is a message: */ - wait_event_interruptible_tty(pInfo->read_wait, + wait_event_interruptible_tty(tty, pInfo->read_wait, (pMsg = remove_msg(pInfo, pClient))); } @@ -1107,7 +1108,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file, } ret = -EPERM; unlock: - tty_unlock(); + tty_unlock(tty); return ret; } @@ -1156,7 +1157,7 @@ static ssize_t r3964_write(struct tty_struct *tty, struct file *file, pHeader->locks = 0; pHeader->owner = NULL; - tty_lock(); + tty_lock(tty); pClient = findClient(pInfo, task_pid(current)); if (pClient) { @@ -1175,7 +1176,7 @@ static ssize_t r3964_write(struct tty_struct *tty, struct file *file, add_tx_queue(pInfo, pHeader); trigger_transmit(pInfo); - tty_unlock(); + tty_unlock(tty); return 0; } diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 5505ffc..d6fa842 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -47,6 +47,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp) wake_up_interruptible(&tty->read_wait); wake_up_interruptible(&tty->write_wait); tty->packet = 0; + /* Review - krefs on tty_link ?? */ if (!tty->link) return; tty->link->packet = 0; @@ -62,9 +63,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp) mutex_unlock(&devpts_mutex); } #endif - tty_unlock(); tty_vhangup(tty->link); - tty_lock(); } } @@ -622,26 +621,29 @@ static int ptmx_open(struct inode *inode, struct file *filp) return retval; /* find a device that is not in use. */ - tty_lock(); + mutex_lock(&devpts_mutex); index = devpts_new_index(inode); - tty_unlock(); if (index < 0) { retval = index; goto err_file; } + mutex_unlock(&devpts_mutex); + mutex_lock(&tty_mutex); mutex_lock(&devpts_mutex); tty = tty_init_dev(ptm_driver, index); - mutex_unlock(&devpts_mutex); - tty_lock(); - mutex_unlock(&tty_mutex); if (IS_ERR(tty)) { retval = PTR_ERR(tty); goto out; } + /* The tty returned here is locked so we can safely + drop the mutex */ + mutex_unlock(&devpts_mutex); + mutex_unlock(&tty_mutex); + set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */ tty_add_file(tty, filp); @@ -654,16 +656,17 @@ static int ptmx_open(struct inode *inode, struct file *filp) if (retval) goto err_release; - tty_unlock(); + tty_unlock(tty); return 0; err_release: - tty_unlock(); + tty_unlock(tty); tty_release(inode, filp); return retval; out: + mutex_unlock(&tty_mutex); devpts_kill_index(inode, index); - tty_unlock(); err_file: + mutex_unlock(&devpts_mutex); tty_free_file(filp); return retval; } diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c index d063b7d..bc1ae4a 100644 --- a/drivers/tty/synclink.c +++ b/drivers/tty/synclink.c @@ -3338,9 +3338,9 @@ static int block_til_ready(struct tty_struct *tty, struct file * filp, printk("%s(%d):block_til_ready blocking on %s count=%d\n", __FILE__,__LINE__, tty->driver->name, port->count ); - tty_unlock(); + tty_unlock(tty); schedule(); - tty_lock(); + tty_lock(tty); } set_current_state(TASK_RUNNING); diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c index fcde827..df4f0dc 100644 --- a/drivers/tty/synclink_gt.c +++ b/drivers/tty/synclink_gt.c @@ -3337,9 +3337,9 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp, } DBGINFO(("%s block_til_ready wait\n", tty->driver->name)); - tty_unlock(); + tty_unlock(tty); schedule(); - tty_lock(); + tty_lock(tty); } set_current_state(TASK_RUNNING); diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c index a040a46..845502f 100644 --- a/drivers/tty/synclinkmp.c +++ b/drivers/tty/synclinkmp.c @@ -3358,9 +3358,9 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp, printk("%s(%d):%s block_til_ready() count=%d\n", __FILE__,__LINE__, tty->driver->name, port->count ); - tty_unlock(); + tty_unlock(tty); schedule(); - tty_lock(); + tty_lock(tty); } set_current_state(TASK_RUNNING); diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index b425c79..9e930c0 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -185,6 +185,7 @@ void free_tty_struct(struct tty_struct *tty) put_device(tty->dev); kfree(tty->write_buf); tty_buffer_free_all(tty); + tty->magic = 0xDEADDEAD; kfree(tty); } @@ -573,7 +574,7 @@ void __tty_hangup(struct tty_struct *tty) } spin_unlock(&redirect_lock); - tty_lock(); + tty_lock(tty); /* some functions below drop BTM, so we need this bit */ set_bit(TTY_HUPPING, &tty->flags); @@ -666,7 +667,7 @@ void __tty_hangup(struct tty_struct *tty) clear_bit(TTY_HUPPING, &tty->flags); tty_ldisc_enable(tty); - tty_unlock(); + tty_unlock(tty); if (f) fput(f); @@ -1103,12 +1104,12 @@ void tty_write_message(struct tty_struct *tty, char *msg) { if (tty) { mutex_lock(&tty->atomic_write_lock); - tty_lock(); + tty_lock(tty); if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) { - tty_unlock(); + tty_unlock(tty); tty->ops->write(tty, msg, strlen(msg)); } else - tty_unlock(); + tty_unlock(tty); tty_write_unlock(tty); } return; @@ -1403,6 +1404,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx) } initialize_tty_struct(tty, driver, idx); + tty_lock(tty); retval = tty_driver_install_tty(driver, tty); if (retval < 0) goto err_deinit_tty; @@ -1415,9 +1417,11 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx) retval = tty_ldisc_setup(tty, tty->link); if (retval) goto err_release_tty; + /* Return the tty locked so that it cannot vanish under the caller */ return tty; err_deinit_tty: + tty_unlock(tty); deinitialize_tty_struct(tty); free_tty_struct(tty); err_module_put: @@ -1426,6 +1430,7 @@ err_module_put: /* call the tty release_tty routine to clean out this slot */ err_release_tty: + tty_unlock(tty); printk_ratelimited(KERN_INFO "tty_init_dev: ldisc open failed, " "clearing slot %d\n", idx); release_tty(tty, idx); @@ -1628,7 +1633,7 @@ int tty_release(struct inode *inode, struct file *filp) if (tty_paranoia_check(tty, inode, __func__)) return 0; - tty_lock(); + tty_lock(tty); check_tty_count(tty, __func__); __tty_fasync(-1, filp, 0); @@ -1637,10 +1642,11 @@ int tty_release(struct inode *inode, struct file *filp) pty_master = (tty->driver->type == TTY_DRIVER_TYPE_PTY && tty->driver->subtype == PTY_TYPE_MASTER); devpts = (tty->driver->flags & TTY_DRIVER_DEVPTS_MEM) != 0; + /* Review: parallel close */ o_tty = tty->link; if (tty_release_checks(tty, o_tty, idx)) { - tty_unlock(); + tty_unlock(tty); return 0; } @@ -1652,7 +1658,7 @@ int tty_release(struct inode *inode, struct file *filp) if (tty->ops->close) tty->ops->close(tty, filp); - tty_unlock(); + tty_unlock(tty); /* * 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 @@ -1675,7 +1681,7 @@ int tty_release(struct inode *inode, struct file *filp) opens on /dev/tty */ mutex_lock(&tty_mutex); - tty_lock(); + tty_lock_pair(tty, o_tty); tty_closing = tty->count <= 1; o_tty_closing = o_tty && (o_tty->count <= (pty_master ? 1 : 0)); @@ -1706,7 +1712,7 @@ int tty_release(struct inode *inode, struct file *filp) printk(KERN_WARNING "%s: %s: read/write wait queue active!\n", __func__, tty_name(tty, buf)); - tty_unlock(); + tty_unlock_pair(tty, o_tty); mutex_unlock(&tty_mutex); schedule(); } @@ -1769,7 +1775,7 @@ int tty_release(struct inode *inode, struct file *filp) /* check whether both sides are closing ... */ if (!tty_closing || (o_tty && !o_tty_closing)) { - tty_unlock(); + tty_unlock_pair(tty, o_tty); return 0; } @@ -1782,14 +1788,16 @@ int tty_release(struct inode *inode, struct file *filp) tty_ldisc_release(tty, o_tty); /* * The release_tty function takes care of the details of clearing - * the slots and preserving the termios structure. + * the slots and preserving the termios structure. The tty_unlock_pair + * should be safe as we keep a kref while the tty is locked (so the + * unlock never unlocks a freed tty). */ release_tty(tty, idx); + tty_unlock_pair(tty, o_tty); /* Make this pty number available for reallocation */ if (devpts) devpts_kill_index(inode, idx); - tty_unlock(); return 0; } @@ -1893,6 +1901,9 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp, * Locking: tty_mutex protects tty, tty_lookup_driver and tty_init_dev. * tty->count should protect the rest. * ->siglock protects ->signal/->sighand + * + * Note: the tty_unlock/lock cases without a ref are only safe due to + * tty_mutex */ static int tty_open(struct inode *inode, struct file *filp) @@ -1916,8 +1927,7 @@ retry_open: retval = 0; mutex_lock(&tty_mutex); - tty_lock(); - + /* This is protected by the tty_mutex */ tty = tty_open_current_tty(device, filp); if (IS_ERR(tty)) { retval = PTR_ERR(tty); @@ -1938,17 +1948,19 @@ retry_open: } if (tty) { + tty_lock(tty); retval = tty_reopen(tty); - if (retval) + if (retval < 0) { + tty_unlock(tty); tty = ERR_PTR(retval); - } else + } + } else /* Returns with the tty_lock held for now */ tty = tty_init_dev(driver, index); mutex_unlock(&tty_mutex); if (driver) tty_driver_kref_put(driver); if (IS_ERR(tty)) { - tty_unlock(); retval = PTR_ERR(tty); goto err_file; } @@ -1977,7 +1989,7 @@ retry_open: printk(KERN_DEBUG "%s: error %d in opening %s...\n", __func__, retval, tty->name); #endif - tty_unlock(); /* need to call tty_release without BTM */ + tty_unlock(tty); /* need to call tty_release without BTM */ tty_release(inode, filp); if (retval != -ERESTARTSYS) return retval; @@ -1989,17 +2001,15 @@ retry_open: /* * Need to reset f_op in case a hangup happened. */ - tty_lock(); if (filp->f_op == &hung_up_tty_fops) filp->f_op = &tty_fops; - tty_unlock(); goto retry_open; } - tty_unlock(); + tty_unlock(tty); mutex_lock(&tty_mutex); - tty_lock(); + tty_lock(tty); spin_lock_irq(¤t->sighand->siglock); if (!noctty && current->signal->leader && @@ -2007,11 +2017,10 @@ retry_open: tty->session == NULL) __proc_set_tty(current, tty); spin_unlock_irq(¤t->sighand->siglock); - tty_unlock(); + tty_unlock(tty); mutex_unlock(&tty_mutex); return 0; err_unlock: - tty_unlock(); mutex_unlock(&tty_mutex); /* after locks to avoid deadlock */ if (!IS_ERR_OR_NULL(driver)) @@ -2094,10 +2103,13 @@ out: static int tty_fasync(int fd, struct file *filp, int on) { + struct tty_struct *tty = file_tty(filp); int retval; - tty_lock(); + + tty_lock(tty); retval = __tty_fasync(fd, filp, on); - tty_unlock(); + tty_unlock(tty); + return retval; } @@ -2934,6 +2946,7 @@ void initialize_tty_struct(struct tty_struct *tty, tty->pgrp = NULL; tty->overrun_time = jiffies; tty_buffer_init(tty); + mutex_init(&tty->legacy_mutex); mutex_init(&tty->termios_mutex); mutex_init(&tty->ldisc_mutex); init_waitqueue_head(&tty->write_wait); diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index 24b95db..fa65cde 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -567,7 +567,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) if (IS_ERR(new_ldisc)) return PTR_ERR(new_ldisc); - tty_lock(); + tty_lock(tty); /* * We need to look at the tty locking here for pty/tty pairs * when both sides try to change in parallel. @@ -581,12 +581,12 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) */ if (tty->ldisc->ops->num == ldisc) { - tty_unlock(); + tty_unlock(tty); tty_ldisc_put(new_ldisc); return 0; } - tty_unlock(); + tty_unlock(tty); /* * Problem: What do we do if this blocks ? * We could deadlock here @@ -594,7 +594,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) tty_wait_until_sent(tty, 0); - tty_lock(); + tty_lock(tty); mutex_lock(&tty->ldisc_mutex); /* @@ -604,10 +604,10 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) while (test_bit(TTY_LDISC_CHANGING, &tty->flags)) { mutex_unlock(&tty->ldisc_mutex); - tty_unlock(); + tty_unlock(tty); wait_event(tty_ldisc_wait, test_bit(TTY_LDISC_CHANGING, &tty->flags) == 0); - tty_lock(); + tty_lock(tty); mutex_lock(&tty->ldisc_mutex); } @@ -622,7 +622,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) o_ldisc = tty->ldisc; - tty_unlock(); + tty_unlock(tty); /* * Make sure we don't change while someone holds a * reference to the line discipline. The TTY_LDISC bit @@ -649,7 +649,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) retval = tty_ldisc_wait_idle(tty, 5 * HZ); - tty_lock(); + tty_lock(tty); mutex_lock(&tty->ldisc_mutex); /* handle wait idle failure locked */ @@ -664,7 +664,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) clear_bit(TTY_LDISC_CHANGING, &tty->flags); mutex_unlock(&tty->ldisc_mutex); tty_ldisc_put(new_ldisc); - tty_unlock(); + tty_unlock(tty); return -EIO; } @@ -707,7 +707,7 @@ enable: if (o_work) schedule_work(&o_tty->buf.work); mutex_unlock(&tty->ldisc_mutex); - tty_unlock(); + tty_unlock(tty); return retval; } @@ -815,11 +815,11 @@ void tty_ldisc_hangup(struct tty_struct *tty) * need to wait for another function taking the BTM */ clear_bit(TTY_LDISC, &tty->flags); - tty_unlock(); + tty_unlock(tty); cancel_work_sync(&tty->buf.work); mutex_unlock(&tty->ldisc_mutex); retry: - tty_lock(); + tty_lock(tty); mutex_lock(&tty->ldisc_mutex); /* At this point we have a closed ldisc and we want to @@ -830,7 +830,7 @@ retry: if (atomic_read(&tty->ldisc->users) != 1) { char cur_n[TASK_COMM_LEN], tty_n[64]; long timeout = 3 * HZ; - tty_unlock(); + tty_unlock(tty); while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) { timeout = MAX_SCHEDULE_TIMEOUT; @@ -911,10 +911,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty) * race with the set_ldisc code path. */ - tty_unlock(); + tty_unlock(tty); tty_ldisc_halt(tty); tty_ldisc_flush_works(tty); - tty_lock(); + tty_lock(tty); mutex_lock(&tty->ldisc_mutex); /* diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c index 9ff986c..69adc80 100644 --- a/drivers/tty/tty_mutex.c +++ b/drivers/tty/tty_mutex.c @@ -4,29 +4,59 @@ #include <linux/semaphore.h> #include <linux/sched.h> -/* - * The 'big tty mutex' - * - * This mutex is taken and released by tty_lock() and tty_unlock(), - * replacing the older big kernel lock. - * It can no longer be taken recursively, and does not get - * released implicitly while sleeping. - * - * Don't use in new code. - */ -static DEFINE_MUTEX(big_tty_mutex); +/* Legacy tty mutex glue */ /* * Getting the big tty mutex. */ -void __lockfunc tty_lock(void) + +void __lockfunc tty_lock(struct tty_struct *tty) { - mutex_lock(&big_tty_mutex); + if (tty->magic != TTY_MAGIC) { + printk(KERN_ERR "L Bad %p\n", tty); + WARN_ON(1); + return; + } + tty_kref_get(tty); + mutex_lock(&tty->legacy_mutex); } EXPORT_SYMBOL(tty_lock); -void __lockfunc tty_unlock(void) +void __lockfunc tty_unlock(struct tty_struct *tty) { - mutex_unlock(&big_tty_mutex); + if (tty->magic != TTY_MAGIC) { + printk(KERN_ERR "U Bad %p\n", tty); + WARN_ON(1); + return; + } + mutex_unlock(&tty->legacy_mutex); + tty_kref_put(tty); } EXPORT_SYMBOL(tty_unlock); + +/* + * Getting the big tty mutex for a pair of ttys with lock ordering + * On a non pty/tty pair tty2 can be NULL which is just fine. + */ +void __lockfunc tty_lock_pair(struct tty_struct *tty, + struct tty_struct *tty2) +{ + if (tty < tty2) { + tty_lock(tty); + tty_lock(tty2); + } else { + if (tty2 && tty2 != tty) + tty_lock(tty2); + tty_lock(tty); + } +} +EXPORT_SYMBOL(tty_lock_pair); + +void __lockfunc tty_unlock_pair(struct tty_struct *tty, + struct tty_struct *tty2) +{ + tty_unlock(tty); + if (tty2 && tty2 != tty) + tty_unlock(tty2); +} +EXPORT_SYMBOL(tty_unlock_pair); diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c index d24807a..64f0975 100644 --- a/drivers/tty/tty_port.c +++ b/drivers/tty/tty_port.c @@ -237,7 +237,7 @@ int tty_port_block_til_ready(struct tty_port *port, /* block if port is in the process of being closed */ if (tty_hung_up_p(filp) || port->flags & ASYNC_CLOSING) { - wait_event_interruptible_tty(port->close_wait, + wait_event_interruptible_tty(tty, port->close_wait, !(port->flags & ASYNC_CLOSING)); if (port->flags & ASYNC_HUP_NOTIFY) return -EAGAIN; @@ -303,9 +303,9 @@ int tty_port_block_til_ready(struct tty_port *port, retval = -ERESTARTSYS; break; } - tty_unlock(); + tty_unlock(tty); schedule(); - tty_lock(); + tty_lock(tty); } finish_wait(&port->open_wait, &wait); diff --git a/include/linux/tty.h b/include/linux/tty.h index 9f47ab5..4990ef2 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -268,6 +268,7 @@ struct tty_struct { struct mutex ldisc_mutex; struct tty_ldisc *ldisc; + struct mutex legacy_mutex; struct mutex termios_mutex; spinlock_t ctrl_lock; /* Termios values are protected by the termios mutex */ @@ -605,8 +606,12 @@ extern long vt_compat_ioctl(struct tty_struct *tty, /* tty_mutex.c */ /* functions for preparation of BKL removal */ -extern void __lockfunc tty_lock(void) __acquires(tty_lock); -extern void __lockfunc tty_unlock(void) __releases(tty_lock); +extern void __lockfunc tty_lock(struct tty_struct *tty); +extern void __lockfunc tty_unlock(struct tty_struct *tty); +extern void __lockfunc tty_lock_pair(struct tty_struct *tty, + struct tty_struct *tty2); +extern void __lockfunc tty_unlock_pair(struct tty_struct *tty, + struct tty_struct *tty2); /* * this shall be called only from where BTM is held (like close) @@ -621,9 +626,9 @@ extern void __lockfunc tty_unlock(void) __releases(tty_lock); static inline void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout) { - tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */ + tty_unlock(tty); /* tty->ops->close holds the BTM, drop it while waiting */ tty_wait_until_sent(tty, timeout); - tty_lock(); + tty_lock(tty); } /* @@ -638,16 +643,16 @@ static inline void tty_wait_until_sent_from_close(struct tty_struct *tty, * * Do not use in new code. */ -#define wait_event_interruptible_tty(wq, condition) \ +#define wait_event_interruptible_tty(tty, wq, condition) \ ({ \ int __ret = 0; \ if (!(condition)) { \ - __wait_event_interruptible_tty(wq, condition, __ret); \ + __wait_event_interruptible_tty(tty, wq, condition, __ret); \ } \ __ret; \ }) -#define __wait_event_interruptible_tty(wq, condition, ret) \ +#define __wait_event_interruptible_tty(tty, wq, condition, ret) \ do { \ DEFINE_WAIT(__wait); \ \ @@ -656,9 +661,9 @@ do { \ if (condition) \ break; \ if (!signal_pending(current)) { \ - tty_unlock(); \ + tty_unlock(tty); \ schedule(); \ - tty_lock(); \ + tty_lock(tty); \ continue; \ } \ ret = -ERESTARTSYS; \ ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-03 21:24 ` [PATCH 3/3] tty_lock: Localise the lock Alan Cox @ 2012-05-04 20:48 ` Arnd Bergmann 2012-05-04 23:54 ` Greg KH 2012-05-07 16:03 ` Sasha Levin 2 siblings, 0 replies; 21+ messages in thread From: Arnd Bergmann @ 2012-05-04 20:48 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial On Thursday 03 May 2012, Alan Cox wrote: > > From: Alan Cox <alan@linux.intel.com> > > In each remaining case the tty_lock is associated with a specific tty. This > means we can now lock on a per tty basis. We do need tty_lock_pair() for > the pty case. Uglier but still a step in the right direction. > > Signed-off-by: Alan Cox <alan@linux.intel.com> Acked-by: Arnd Bergmann <arnd@arndb.de> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-03 21:24 ` [PATCH 3/3] tty_lock: Localise the lock Alan Cox 2012-05-04 20:48 ` Arnd Bergmann @ 2012-05-04 23:54 ` Greg KH 2012-05-04 23:55 ` Greg KH 2012-05-07 16:03 ` Sasha Levin 2 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2012-05-04 23:54 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial On Thu, May 03, 2012 at 10:24:08PM +0100, Alan Cox wrote: > From: Alan Cox <alan@linux.intel.com> > > In each remaining case the tty_lock is associated with a specific tty. This > means we can now lock on a per tty basis. We do need tty_lock_pair() for > the pty case. Uglier but still a step in the right direction. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > Acked-by: Arnd Bergmann <arnd@arndb.de> > --- > > drivers/tty/amiserial.c | 12 ++++---- > drivers/tty/cyclades.c | 2 + > drivers/tty/n_r3964.c | 11 ++++--- > drivers/tty/pty.c | 23 +++++++++------ > drivers/tty/synclink.c | 4 +-- > drivers/tty/synclink_gt.c | 4 +-- > drivers/tty/synclinkmp.c | 4 +-- > drivers/tty/tty_io.c | 67 +++++++++++++++++++++++++++------------------ > drivers/tty/tty_ldisc.c | 30 ++++++++++---------- > drivers/tty/tty_mutex.c | 60 ++++++++++++++++++++++++++++++---------- > drivers/tty/tty_port.c | 6 ++-- > include/linux/tty.h | 23 +++++++++------ > 12 files changed, 149 insertions(+), 97 deletions(-) You forgot the call in net/bluetooth/rfcomm/tty.c, I'll go fix that up... ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-04 23:54 ` Greg KH @ 2012-05-04 23:55 ` Greg KH 0 siblings, 0 replies; 21+ messages in thread From: Greg KH @ 2012-05-04 23:55 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial On Fri, May 04, 2012 at 04:54:12PM -0700, Greg KH wrote: > On Thu, May 03, 2012 at 10:24:08PM +0100, Alan Cox wrote: > > From: Alan Cox <alan@linux.intel.com> > > > > In each remaining case the tty_lock is associated with a specific tty. This > > means we can now lock on a per tty basis. We do need tty_lock_pair() for > > the pty case. Uglier but still a step in the right direction. > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > --- > > > > drivers/tty/amiserial.c | 12 ++++---- > > drivers/tty/cyclades.c | 2 + > > drivers/tty/n_r3964.c | 11 ++++--- > > drivers/tty/pty.c | 23 +++++++++------ > > drivers/tty/synclink.c | 4 +-- > > drivers/tty/synclink_gt.c | 4 +-- > > drivers/tty/synclinkmp.c | 4 +-- > > drivers/tty/tty_io.c | 67 +++++++++++++++++++++++++++------------------ > > drivers/tty/tty_ldisc.c | 30 ++++++++++---------- > > drivers/tty/tty_mutex.c | 60 ++++++++++++++++++++++++++++++---------- > > drivers/tty/tty_port.c | 6 ++-- > > include/linux/tty.h | 23 +++++++++------ > > 12 files changed, 149 insertions(+), 97 deletions(-) > > You forgot the call in net/bluetooth/rfcomm/tty.c, I'll go fix that > up... And drivers/tty/serial/crisv10.c and drivers/staging/serial/68360serial.c... ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-03 21:24 ` [PATCH 3/3] tty_lock: Localise the lock Alan Cox @ 2012-05-07 16:03 ` Sasha Levin 2012-05-04 23:54 ` Greg KH 2012-05-07 16:03 ` Sasha Levin 2 siblings, 0 replies; 21+ messages in thread From: Sasha Levin @ 2012-05-07 16:03 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial Hi Alan, On Thu, May 3, 2012 at 11:24 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index 5505ffc..d6fa842 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -62,9 +63,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp) > mutex_unlock(&devpts_mutex); > } > #endif > - tty_unlock(); > tty_vhangup(tty->link); > - tty_lock(); > } > } I don't believe that this change is correct. Consider the following scenario: tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock Which would cause a deadlock. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock @ 2012-05-07 16:03 ` Sasha Levin 0 siblings, 0 replies; 21+ messages in thread From: Sasha Levin @ 2012-05-07 16:03 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial Hi Alan, On Thu, May 3, 2012 at 11:24 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index 5505ffc..d6fa842 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -62,9 +63,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp) > mutex_unlock(&devpts_mutex); > } > #endif > - tty_unlock(); > tty_vhangup(tty->link); > - tty_lock(); > } > } I don't believe that this change is correct. Consider the following scenario: tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock Which would cause a deadlock. -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-07 16:03 ` Sasha Levin (?) @ 2012-05-07 16:11 ` Alan Cox 2012-05-07 16:30 ` Sasha Levin -1 siblings, 1 reply; 21+ messages in thread From: Alan Cox @ 2012-05-07 16:11 UTC (permalink / raw) To: Sasha Levin; +Cc: linux-kernel, linux-serial > I don't believe that this change is correct. > > Consider the following scenario: > > tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock We hang up tty->link not tty. It's now a per tty lock. So I think we are ok. Alan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-07 16:11 ` Alan Cox @ 2012-05-07 16:30 ` Sasha Levin 2012-05-07 16:42 ` Alan Cox 0 siblings, 1 reply; 21+ messages in thread From: Sasha Levin @ 2012-05-07 16:30 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial On Mon, 2012-05-07 at 17:11 +0100, Alan Cox wrote: > > I don't believe that this change is correct. > > > > Consider the following scenario: > > > > tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock > > We hang up tty->link not tty. > > It's now a per tty lock. So I think we are ok. Unless we can cause tty->link == tty, in which case: [ 6522.256890] ============================================= [ 6522.257023] [ INFO: possible recursive locking detected ] [ 6522.257023] 3.4.0-rc6-next-20120507-sasha-00001-g06a300f #175 Tainted: G W [ 6522.257023] --------------------------------------------- [ 6522.257023] trinity/18088 is trying to acquire lock: [ 6522.257023] (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82d8a6f2>] tty_lock+0x72/0x80 [ 6522.257023] [ 6522.257023] but task is already holding lock: [ 6522.257023] (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82d8a6f2>] tty_lock+0x72/0x80 [ 6522.257023] [ 6522.257023] other info that might help us debug this: [ 6522.257023] Possible unsafe locking scenario: [ 6522.257023] [ 6522.257023] CPU0 [ 6522.257023] ---- [ 6522.257023] lock(&tty->legacy_mutex); [ 6522.257023] lock(&tty->legacy_mutex); [ 6522.257023] [ 6522.257023] *** DEADLOCK *** [ 6522.257023] [ 6522.257023] May be due to missing lock nesting notation [ 6522.257023] [ 6522.257023] 1 lock held by trinity/18088: [ 6522.257023] #0: (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82d8a6f2>] tty_lock+0x72/0x80 [ 6522.257023] [ 6522.257023] stack backtrace: [ 6522.257023] Pid: 18088, comm: trinity Tainted: G W 3.4.0-rc6-next-20120507-sasha-00001-g06a300f #175 [ 6522.257023] Call Trace: [ 6522.257023] [<ffffffff8111a509>] print_deadlock_bug+0x119/0x140 [ 6522.257023] [<ffffffff8111c6fe>] validate_chain+0x5ee/0x790 [ 6522.257023] [<ffffffff810f1418>] ? sched_clock_cpu+0x108/0x120 [ 6522.257023] [<ffffffff8111ccc3>] __lock_acquire+0x423/0x4c0 [ 6522.257023] [<ffffffff8111ce3c>] lock_acquire+0xdc/0x120 [ 6522.257023] [<ffffffff82d8a6f2>] ? tty_lock+0x72/0x80 [ 6522.257023] [<ffffffff82d86b60>] __mutex_lock_common+0x60/0x590 [ 6522.257023] [<ffffffff82d8a6f2>] ? tty_lock+0x72/0x80 [ 6522.257023] [<ffffffff82d8a6f2>] ? tty_lock+0x72/0x80 [ 6522.257023] [<ffffffff82d871c0>] mutex_lock_nested+0x40/0x50 [ 6522.257023] [<ffffffff82d8a6f2>] tty_lock+0x72/0x80 [ 6522.257023] [<ffffffff81a2ce34>] __tty_hangup+0x74/0x400 [ 6522.257023] [<ffffffff82d8a154>] ? _raw_spin_unlock_irqrestore+0x94/0xc0 [ 6522.257023] [<ffffffff81a2d1e9>] tty_vhangup+0x9/0x10 [ 6522.257023] [<ffffffff81a36264>] pty_close+0x154/0x160 [ 6522.257023] [<ffffffff81a2dfcd>] tty_release+0xed/0x4d0 [ 6522.257023] [<ffffffff8122d0eb>] ? vfs_lock_file+0x3b/0x40 [ 6522.257023] [<ffffffff8122d18e>] ? locks_remove_posix+0x9e/0xe0 [ 6522.257023] [<ffffffff811e13ea>] __fput+0x11a/0x2c0 [ 6522.257023] [<ffffffff811e15a5>] fput+0x15/0x20 [ 6522.257023] [<ffffffff811dd8b2>] filp_close+0x82/0xa0 [ 6522.257023] [<ffffffff810bb914>] close_files+0x1b4/0x200 [ 6522.257023] [<ffffffff810bb760>] ? sys_waitid+0x200/0x200 [ 6522.257023] [<ffffffff810bb981>] put_files_struct+0x21/0x180 [ 6522.257023] [<ffffffff82d8a090>] ? _raw_spin_unlock+0x30/0x60 [ 6522.257023] [<ffffffff810bbb2d>] exit_files+0x4d/0x60 [ 6522.257023] [<ffffffff810bc7b5>] do_exit+0x285/0x460 [ 6522.257023] [<ffffffff810e74e1>] ? get_parent_ip+0x11/0x50 [ 6522.257023] [<ffffffff810bca31>] do_group_exit+0xa1/0xe0 [ 6522.257023] [<ffffffff810cceb8>] get_signal_to_deliver+0x348/0x3a0 [ 6522.257023] [<ffffffff810e855d>] ? finish_task_switch+0x8d/0x110 [ 6522.257023] [<ffffffff8104daf2>] do_signal+0x42/0x120 [ 6522.257023] [<ffffffff810e74e1>] ? get_parent_ip+0x11/0x50 [ 6522.257023] [<ffffffff810e7c1e>] ? sub_preempt_count+0xae/0xf0 [ 6522.257023] [<ffffffff82d8850f>] ? __schedule+0x79f/0x7d0 [ 6522.257023] [<ffffffff82d8a934>] ? retint_restore_args+0x13/0x13 [ 6522.257023] [<ffffffff82d8a9bf>] ? retint_signal+0x11/0x92 [ 6522.257023] [<ffffffff8104dc24>] do_notify_resume+0x54/0xb0 [ 6522.257023] [<ffffffff82d8a9fb>] retint_signal+0x4d/0x92 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-07 16:30 ` Sasha Levin @ 2012-05-07 16:42 ` Alan Cox 2012-05-07 17:00 ` Sasha Levin 0 siblings, 1 reply; 21+ messages in thread From: Alan Cox @ 2012-05-07 16:42 UTC (permalink / raw) To: Sasha Levin; +Cc: linux-kernel, linux-serial On Mon, 07 May 2012 18:30:08 +0200 Sasha Levin <levinsasha928@gmail.com> wrote: > On Mon, 2012-05-07 at 17:11 +0100, Alan Cox wrote: > > > I don't believe that this change is correct. > > > > > > Consider the following scenario: > > > > > > tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock > > > > We hang up tty->link not tty. > > > > It's now a per tty lock. So I think we are ok. > > Unless we can cause tty->link == tty, in which case: We should not be able to cause tty->link == tty. So that's a different problem altogether. tty->link is set to point to the other half of the pty in pty_install and in pty98_unix_install. It's never assigned to the same tty and ptys simply wouldn't work if this wasn't the case. So whatever your trace is showing, that's not the bug. Something more complicated would appear to be afoot. Alan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-07 16:42 ` Alan Cox @ 2012-05-07 17:00 ` Sasha Levin 2012-05-07 21:04 ` Jiri Slaby 0 siblings, 1 reply; 21+ messages in thread From: Sasha Levin @ 2012-05-07 17:00 UTC (permalink / raw) To: Alan Cox; +Cc: linux-kernel, linux-serial > So whatever your trace is showing, that's not the bug. Something more > complicated would appear to be afoot. Oddly enough, tty != tty->link, but the lockdep warning triggers. Any idea why it might happen? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-07 17:00 ` Sasha Levin @ 2012-05-07 21:04 ` Jiri Slaby 2012-05-08 18:12 ` Sasha Levin 0 siblings, 1 reply; 21+ messages in thread From: Jiri Slaby @ 2012-05-07 21:04 UTC (permalink / raw) To: Sasha Levin; +Cc: Alan Cox, linux-kernel, linux-serial, Jiri Slaby On 05/07/2012 07:00 PM, Sasha Levin wrote: >> So whatever your trace is showing, that's not the bug. Something more >> complicated would appear to be afoot. > > Oddly enough, tty != tty->link, but the lockdep warning triggers. > > Any idea why it might happen? I think so, both locks are the same lockdep class. So lockdep thinks it is the same lock. However this is a false positive. I guess we need mutex_lock_nested... regards, -- js suse labs ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-07 21:04 ` Jiri Slaby @ 2012-05-08 18:12 ` Sasha Levin 2012-05-11 10:40 ` Sasha Levin 0 siblings, 1 reply; 21+ messages in thread From: Sasha Levin @ 2012-05-08 18:12 UTC (permalink / raw) To: Jiri Slaby; +Cc: Alan Cox, linux-kernel, linux-serial, Jiri Slaby On Mon, May 7, 2012 at 11:04 PM, Jiri Slaby <jslaby@suse.cz> wrote: > On 05/07/2012 07:00 PM, Sasha Levin wrote: >>> So whatever your trace is showing, that's not the bug. Something more >>> complicated would appear to be afoot. >> >> Oddly enough, tty != tty->link, but the lockdep warning triggers. >> >> Any idea why it might happen? > > I think so, both locks are the same lockdep class. So lockdep thinks it > is the same lock. However this is a false positive. I guess we need > mutex_lock_nested... It looks like it causes an actual deadlock, and hung_tasks screams if left alone for a bit, so it doesn't look like a lockdep issue. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-08 18:12 ` Sasha Levin @ 2012-05-11 10:40 ` Sasha Levin 2012-05-11 14:52 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Sasha Levin @ 2012-05-11 10:40 UTC (permalink / raw) To: Jiri Slaby; +Cc: Alan Cox, linux-kernel, linux-serial, Jiri Slaby On Tue, May 8, 2012 at 8:12 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > On Mon, May 7, 2012 at 11:04 PM, Jiri Slaby <jslaby@suse.cz> wrote: >> On 05/07/2012 07:00 PM, Sasha Levin wrote: >>>> So whatever your trace is showing, that's not the bug. Something more >>>> complicated would appear to be afoot. >>> >>> Oddly enough, tty != tty->link, but the lockdep warning triggers. >>> >>> Any idea why it might happen? >> >> I think so, both locks are the same lockdep class. So lockdep thinks it >> is the same lock. However this is a false positive. I guess we need >> mutex_lock_nested... > > It looks like it causes an actual deadlock, and hung_tasks screams if > left alone for a bit, so it doesn't look like a lockdep issue. I've applied the patch that unlocks before hangup, and started seeing this new warning instead (sorry if output below looks a bit broken, linux-next has a printk rework in progress): [ 47.919734] ============================================= [ 47.920013] [ INFO: possible recursive locking detected ] [ 47.920013] 3.4.0-rc6-next-20120511-sasha-00001-g1975c5f #183 Tainted: G W [ 47.920013] --------------------------------------------- [ 47.920013] trinity/6825 is trying to acquire lock: [ 47.920013] (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82e9cd02>] tty_lock+0x72/0x80 [ 47.920013] but task is already holding lock: [ 47.920013] (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82e9cd02>] tty_lock+0x72/0x80 [ 47.920013] other info that might help us debug this: [ 47.920013] Possible unsafe locking scenario: [ 47.920013] CPU0 [ 47.920013] ---- [ 47.920013] lock(&tty->legacy_mutex); [ 47.920013] lock(&tty->legacy_mutex); [ 47.920013] *** DEADLOCK *** [ 47.920013] May be due to missing lock nesting notation [ 47.920013] 2 locks held by trinity/6825: [ 47.920013] #0: (tty_mutex){+.+.+.}, at: [<ffffffff81b00837>] tty_release+0x177/0x4d0 [ 47.920013] #1: (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82e9cd02>] tty_lock+0x72/0x80 [ 47.920013] stack backtrace: [ 47.920013] Pid: 6825, comm: trinity Tainted: G W 3.4.0-rc6-next-20120511-sasha-00001-g1975c5f #183 [ 47.920013] Call Trace: [ 47.920013] [<ffffffff811440b9>] print_deadlock_bug+0x119/0x140 [ 47.920013] [<ffffffff8114622e>] validate_chain+0x5ee/0x790 [ 47.920013] [<ffffffff81119eb8>] ? sched_clock_cpu+0x108/0x120 [ 47.920013] [<ffffffff811467f3>] __lock_acquire+0x423/0x4c0 [ 47.920013] [<ffffffff81146a1a>] lock_acquire+0x18a/0x1e0 [ 47.920013] [<ffffffff82e9cd02>] ? tty_lock+0x72/0x80 [ 47.920013] [<ffffffff82e99558>] ? __mutex_lock_common+0x518/0x590 [ 47.920013] [<ffffffff82e990a0>] __mutex_lock_common+0x60/0x590 [ 47.920013] [<ffffffff82e9cd02>] ? tty_lock+0x72/0x80 [ 47.920013] [<ffffffff82e9cd02>] ? tty_lock+0x72/0x80 [ 47.920013] [<ffffffff82e99700>] mutex_lock_nested+0x40/0x50 [ 47.920013] [<ffffffff82e9cd02>] tty_lock+0x72/0x80 [ 47.920013] [<ffffffff82e9cd36>] tty_lock_pair+0x26/0x54 [ 47.920013] [<ffffffff81b00842>] tty_release+0x182/0x4d0 [ 47.920013] [<ffffffff8122cf1a>] __fput+0x11a/0x2c0 [ 47.920013] [<ffffffff8122d0d5>] fput+0x15/0x20 [ 47.920013] [<ffffffff812293e2>] filp_close+0x82/0xa0 [ 47.920013] [<ffffffff810d6214>] close_files+0x1b4/0x200 [ 47.920013] [<ffffffff810d6060>] ? wait_task_stopped+0x3c0/0x3c0 [ 47.920013] [<ffffffff810d6425>] ? exit_files+0x45/0x60 [ 47.920013] [<ffffffff810d6281>] put_files_struct+0x21/0x180 [ 47.920013] [<ffffffff82e9c6a0>] ? _raw_spin_unlock+0x30/0x60 [ 47.920013] [<ffffffff810d642d>] exit_files+0x4d/0x60 [ 47.920013] [<ffffffff810d86e2>] do_exit+0x322/0x500 [ 47.920013] [<ffffffff810d8961>] do_group_exit+0xa1/0xe0 [ 47.920013] [<ffffffff810d89b2>] sys_exit_group+0x12/0x20 [ 47.920013] [<ffffffff82e9d7f9>] system_call_fastpath+0x16/0x1b ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-11 10:40 ` Sasha Levin @ 2012-05-11 14:52 ` Greg KH 2012-05-11 15:09 ` Sasha Levin 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2012-05-11 14:52 UTC (permalink / raw) To: Sasha Levin; +Cc: Jiri Slaby, Alan Cox, linux-kernel, linux-serial, Jiri Slaby On Fri, May 11, 2012 at 12:40:41PM +0200, Sasha Levin wrote: > On Tue, May 8, 2012 at 8:12 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > > On Mon, May 7, 2012 at 11:04 PM, Jiri Slaby <jslaby@suse.cz> wrote: > >> On 05/07/2012 07:00 PM, Sasha Levin wrote: > >>>> So whatever your trace is showing, that's not the bug. Something more > >>>> complicated would appear to be afoot. > >>> > >>> Oddly enough, tty != tty->link, but the lockdep warning triggers. > >>> > >>> Any idea why it might happen? > >> > >> I think so, both locks are the same lockdep class. So lockdep thinks it > >> is the same lock. However this is a false positive. I guess we need > >> mutex_lock_nested... > > > > It looks like it causes an actual deadlock, and hung_tasks screams if > > left alone for a bit, so it doesn't look like a lockdep issue. > > I've applied the patch that unlocks before hangup, and started seeing > this new warning instead (sorry if output below looks a bit broken, > linux-next has a printk rework in progress): The printk rework should now be working again in linux-next. Is it still causing problems for you? thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock 2012-05-11 14:52 ` Greg KH @ 2012-05-11 15:09 ` Sasha Levin 0 siblings, 0 replies; 21+ messages in thread From: Sasha Levin @ 2012-05-11 15:09 UTC (permalink / raw) To: Greg KH; +Cc: Jiri Slaby, Alan Cox, linux-kernel, linux-serial, Jiri Slaby On Fri, May 11, 2012 at 4:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > The printk rework should now be working again in linux-next. Is it > still causing problems for you? Mostly does, there's still a minor issue regarding showing timestamps on each line in the log, I've commented about it in the original thread ( http://permalink.gmane.org/gmane.linux.kernel/1295580). It's fairly visible in the lockdep spew, since lockdep does lots of this "printk("\nsomething\n");" ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] tty_lock: Localise the lock @ 2012-05-11 15:09 ` Sasha Levin 0 siblings, 0 replies; 21+ messages in thread From: Sasha Levin @ 2012-05-11 15:09 UTC (permalink / raw) To: Greg KH; +Cc: Jiri Slaby, Alan Cox, linux-kernel, linux-serial, Jiri Slaby On Fri, May 11, 2012 at 4:52 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > The printk rework should now be working again in linux-next. Is it > still causing problems for you? Mostly does, there's still a minor issue regarding showing timestamps on each line in the log, I've commented about it in the original thread ( http://permalink.gmane.org/gmane.linux.kernel/1295580). It's fairly visible in the lockdep spew, since lockdep does lots of this "printk("\nsomething\n");" -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-05-11 15:10 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-03 21:21 [PATCH 1/3] tty_lock: undo the old tty_lock use on the ctty Alan Cox 2012-05-03 21:22 ` [PATCH 2/3] pty: Lock the devpts bits privately Alan Cox 2012-05-08 18:18 ` H. Peter Anvin 2012-05-08 20:43 ` Alan Cox 2012-05-08 20:41 ` H. Peter Anvin 2012-05-03 21:24 ` [PATCH 3/3] tty_lock: Localise the lock Alan Cox 2012-05-04 20:48 ` Arnd Bergmann 2012-05-04 23:54 ` Greg KH 2012-05-04 23:55 ` Greg KH 2012-05-07 16:03 ` Sasha Levin 2012-05-07 16:03 ` Sasha Levin 2012-05-07 16:11 ` Alan Cox 2012-05-07 16:30 ` Sasha Levin 2012-05-07 16:42 ` Alan Cox 2012-05-07 17:00 ` Sasha Levin 2012-05-07 21:04 ` Jiri Slaby 2012-05-08 18:12 ` Sasha Levin 2012-05-11 10:40 ` Sasha Levin 2012-05-11 14:52 ` Greg KH 2012-05-11 15:09 ` Sasha Levin 2012-05-11 15:09 ` Sasha Levin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.