Gitweb:     
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5a39e8c6d655b4fe8305ef8cc2d9bbe782bfee5f
Commit:     5a39e8c6d655b4fe8305ef8cc2d9bbe782bfee5f
Parent:     3a0ee2ce8cc4f962031d7520df960431c2f26a9c
Author:     Aristeu Sergio Rozanski Filho <[EMAIL PROTECTED]>
AuthorDate: Wed Feb 28 20:13:53 2007 -0800
Committer:  Linus Torvalds <[EMAIL PROTECTED]>
CommitDate: Thu Mar 1 14:53:39 2007 -0800

    [PATCH] tty_io: fix race in master pty close/slave pty close path
    
    This patch fixes a possible race that leads to double freeing an idr index.
     When the master begin to close, release_dev() is called and then
    pty_close() is called:
    
            if (tty->driver->close)
                    tty->driver->close(tty, filp);
    
    This is done without helding any locks other than BKL.  Inside pty_close(),
    being a master close, the devpts entry will be removed:
    
    #ifdef CONFIG_UNIX98_PTYS
                    if (tty->driver == ptm_driver)
                            devpts_pty_kill(tty->index);
    #endif
    
    But devpts_pty_kill() will call get_node() that may sleep while waiting for
    &devpts_root->d_inode->i_sem.  When this happens and the slave is being
    opened, tty_open() just found the driver and index:
    
            driver = get_tty_driver(device, &index);
            if (!driver) {
                    mutex_unlock(&tty_mutex);
                    return -ENODEV;
            }
    
    This part of the code is already protected under tty_mute.  The problem is
    that the slave close already got an index.  Then init_dev() is called and
    blocks waiting for the same &devpts_root->d_inode->i_sem.
    
    When the master close resumes, it removes the devpts entry, and the
    relation between idr index and the tty is gone.  The master then sleeps
    waiting for the tty_mutex on release_dev().
    
    Slave open resumes and found no tty for that index.  As result, a NULL tty
    is returned and init_dev() doesn't flow to fast_track:
    
            /* check whether we're reopening an existing tty */
            if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
                    tty = devpts_get_tty(idx);
                    if (tty && driver->subtype == PTY_TYPE_MASTER)
                            tty = tty->link;
            } else {
                    tty = driver->ttys[idx];
            }
            if (tty) goto fast_track;
    
    The result of this, is that a new tty will be created and init_dev() returns
    sucessfull. After returning, tty_mutex is dropped and master close may 
resume.
    
    Master close finds it's the only use and both sides are closing, then 
releases
    the tty and the index. At this point, the idr index is free, but slave still
    has it.
    
    Slave open then calls pty_open() and finds that tty->link->count is 0,
    because there's no master and returns error.  Then tty_open() calls
    release_dev() which executes without any warning, as it was a case of last
    slave close when the master is already closed (master->count == 0,
    slave->count == 1).  The tty is then released with the already released idr
    index.
    
    This normally would only issue a warning on idr_remove() but in case of a
    customer's critical application, it's never too simple:
    
    thread1: opens master, gets index X
    thread1: begin closing master
    thread2: begin opening slave with index X
    thread1: finishes closing master, index X released
    thread3: opens master, gets index X, just released
    thread2: fails opening slave, releases index X         <----
    thread4: opens master, gets index X, init_dev() then find an already in use
         and healthy tty and fails
    
    If no more indexes are released, ptmx_open() will keep failing, as the
    first free index available is X, and it will make init_dev() fail because
    you're trying to "reopen a master" which isn't valid.
    
    The patch notices when this race happens and make init_dev() fail
    imediately.  The init_dev() function is called with tty_mutex held, so it's
    safe to continue with tty till the end of function because release_dev()
    won't make any further changes without grabbing the tty_mutex.
    
    Without the patch, on some machines it's possible get easily idr warnings
    like this one:
    
    idr_remove called for id=15 which is not allocated.
     [<c02555b9>] idr_remove+0x139/0x170
     [<c02a1b62>] release_mem+0x182/0x230
     [<c02a28e7>] release_dev+0x4b7/0x700
     [<c02a0ea7>] tty_ldisc_enable+0x27/0x30
     [<c02a1e64>] init_dev+0x254/0x580
     [<c02a0d64>] check_tty_count+0x14/0xb0
     [<c02a4f05>] tty_open+0x1c5/0x340
     [<c02a4d40>] tty_open+0x0/0x340
     [<c017388f>] chrdev_open+0xaf/0x180
     [<c017c2ac>] open_namei+0x8c/0x760
     [<c01737e0>] chrdev_open+0x0/0x180
     [<c0167bc9>] __dentry_open+0xc9/0x210
     [<c0167e2c>] do_filp_open+0x5c/0x70
     [<c0167a91>] get_unused_fd+0x61/0xd0
     [<c0167e93>] do_sys_open+0x53/0x100
     [<c0167f97>] sys_open+0x27/0x30
     [<c010303b>] syscall_call+0x7/0xb
    
    using this test application available on:
     http://www.ruivo.org/~aris/pty_sodomizer.c
    
    Signed-off-by: Aristeu Sergio Rozanski Filho <[EMAIL PROTECTED]>
    Cc: "H. Peter Anvin" <[EMAIL PROTECTED]>
    Cc: Chuck Ebbert <[EMAIL PROTECTED]>
    Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
    Signed-off-by: Linus Torvalds <[EMAIL PROTECTED]>
---
 drivers/char/tty_io.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index f24c26d..e453268 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1901,6 +1901,20 @@ static int init_dev(struct tty_driver *driver, int idx,
        /* check whether we're reopening an existing tty */
        if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
                tty = devpts_get_tty(idx);
+               /*
+                * If we don't have a tty here on a slave open, it's because
+                * the master already started the close process and there's
+                * no relation between devpts file and tty anymore.
+                */
+               if (!tty && driver->subtype == PTY_TYPE_SLAVE) {
+                       retval = -EIO;
+                       goto end_init;
+               }
+               /*
+                * It's safe from now on because init_dev() is called with
+                * tty_mutex held and release_dev() won't change tty->count
+                * or tty->flags without having to grab tty_mutex
+                */
                if (tty && driver->subtype == PTY_TYPE_MASTER)
                        tty = tty->link;
        } else {
-
To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to