From: Jason Baron Date: Fri, 9 Sep 2005 20:01:57 +0000 (-0700) Subject: [PATCH] pty_chars_in_buffer oops fix X-Git-Tag: v2.6.14-rc1~458 X-Git-Url: https://err.no/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ff55fe2075e3901db4dbdc00e0b44a71bef97afd;p=linux-2.6 [PATCH] pty_chars_in_buffer oops fix The idea of this patch is to lock both sides of a ptmx/pty pair during line discipline changing. This is needed to ensure that say a poll on one side of the pty doesn't occur while the line discipline is actively being changed. This resulted in an oops reported on lkml, see: http://marc.theaimsgroup.com/?l=linux-kernel&m=111342171410005&w=2 A 'hacky' approach was previously implmemented which served to eliminate the poll vs. line discipline changing race. However, this patch takes a more general approach to the issue. The patch only adds locking on a less often used path, the line-discipline changing path, as opposed to locking the ptmx/pty pair on read/write/poll paths. The patch below, takes both ldisc locks in either order b/c the locks are both taken under the same spinlock(). I thought about locking the ptmx/pty separately, such as master always first but that introduces a 3 way deadlock. For example, process 1 does a blocking read on the slave side. Then, process 2 does an ldisc change on the slave side, which acquires the master ldisc lock but not the slave's. Finally, process 3 does a write which blocks on the process 2's ldisc reference. This patch does introduce some changes in semantics. For example, a line discipline change on side 'a' of a ptmx/pty pair, will now wait for a read/write to complete on the other side, or side 'b'. The current behavior is to simply wait for any read/writes on only side 'a', not both sides 'a' and 'b'. I think this behavior makes sense, but I wanted to point it out. I've tested the patch with a bunch of read/write/poll while changing the line discipline out from underneath. This patch obviates the need for the above "hide the problem" patch. Signed-off-by: Jason Baron Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- diff --git a/drivers/char/pty.c b/drivers/char/pty.c index da32889d22..49f3997fd2 100644 --- a/drivers/char/pty.c +++ b/drivers/char/pty.c @@ -149,15 +149,14 @@ static int pty_write_room(struct tty_struct *tty) static int pty_chars_in_buffer(struct tty_struct *tty) { struct tty_struct *to = tty->link; - ssize_t (*chars_in_buffer)(struct tty_struct *); int count; /* We should get the line discipline lock for "tty->link" */ - if (!to || !(chars_in_buffer = to->ldisc.chars_in_buffer)) + if (!to || !to->ldisc.chars_in_buffer) return 0; /* The ldisc must report 0 if no characters available to be read */ - count = chars_in_buffer(to); + count = to->ldisc.chars_in_buffer(to); if (tty->driver->subtype == PTY_TYPE_SLAVE) return count; diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index 9d657127f3..6a56ae4f77 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -469,21 +469,19 @@ static void tty_ldisc_enable(struct tty_struct *tty) static int tty_set_ldisc(struct tty_struct *tty, int ldisc) { - int retval = 0; - struct tty_ldisc o_ldisc; + int retval = 0; + struct tty_ldisc o_ldisc; char buf[64]; int work; unsigned long flags; struct tty_ldisc *ld; + struct tty_struct *o_tty; if ((ldisc < N_TTY) || (ldisc >= NR_LDISCS)) return -EINVAL; restart: - if (tty->ldisc.num == ldisc) - return 0; /* We are already in the desired discipline */ - ld = tty_ldisc_get(ldisc); /* Eduardo Blanco */ /* Cyrus Durgin */ @@ -494,45 +492,74 @@ restart: if (ld == NULL) return -EINVAL; - o_ldisc = tty->ldisc; - tty_wait_until_sent(tty, 0); + if (tty->ldisc.num == ldisc) { + tty_ldisc_put(ldisc); + return 0; + } + + o_ldisc = tty->ldisc; + o_tty = tty->link; + /* * Make sure we don't change while someone holds a * reference to the line discipline. The TTY_LDISC bit * prevents anyone taking a reference once it is clear. * We need the lock to avoid racing reference takers. */ - + spin_lock_irqsave(&tty_ldisc_lock, flags); - if(tty->ldisc.refcount) - { - /* Free the new ldisc we grabbed. Must drop the lock - first. */ + if (tty->ldisc.refcount || (o_tty && o_tty->ldisc.refcount)) { + if(tty->ldisc.refcount) { + /* Free the new ldisc we grabbed. Must drop the lock + first. */ + spin_unlock_irqrestore(&tty_ldisc_lock, flags); + tty_ldisc_put(ldisc); + /* + * There are several reasons we may be busy, including + * random momentary I/O traffic. We must therefore + * retry. We could distinguish between blocking ops + * and retries if we made tty_ldisc_wait() smarter. That + * is up for discussion. + */ + if (wait_event_interruptible(tty_ldisc_wait, tty->ldisc.refcount == 0) < 0) + return -ERESTARTSYS; + goto restart; + } + if(o_tty && o_tty->ldisc.refcount) { + spin_unlock_irqrestore(&tty_ldisc_lock, flags); + tty_ldisc_put(ldisc); + if (wait_event_interruptible(tty_ldisc_wait, o_tty->ldisc.refcount == 0) < 0) + return -ERESTARTSYS; + goto restart; + } + } + + /* if the TTY_LDISC bit is set, then we are racing against another ldisc change */ + + if (!test_bit(TTY_LDISC, &tty->flags)) { spin_unlock_irqrestore(&tty_ldisc_lock, flags); tty_ldisc_put(ldisc); - /* - * There are several reasons we may be busy, including - * random momentary I/O traffic. We must therefore - * retry. We could distinguish between blocking ops - * and retries if we made tty_ldisc_wait() smarter. That - * is up for discussion. - */ - if(wait_event_interruptible(tty_ldisc_wait, tty->ldisc.refcount == 0) < 0) - return -ERESTARTSYS; + ld = tty_ldisc_ref_wait(tty); + tty_ldisc_deref(ld); goto restart; } - clear_bit(TTY_LDISC, &tty->flags); + + clear_bit(TTY_LDISC, &tty->flags); clear_bit(TTY_DONT_FLIP, &tty->flags); + if (o_tty) { + clear_bit(TTY_LDISC, &o_tty->flags); + clear_bit(TTY_DONT_FLIP, &o_tty->flags); + } spin_unlock_irqrestore(&tty_ldisc_lock, flags); - + /* * From this point on we know nobody has an ldisc * usage reference, nor can they obtain one until * we say so later on. */ - + work = cancel_delayed_work(&tty->flip.work); /* * Wait for ->hangup_work and ->flip.work handlers to terminate @@ -583,10 +610,12 @@ restart: */ tty_ldisc_enable(tty); + if (o_tty) + tty_ldisc_enable(o_tty); /* Restart it in case no characters kick it off. Safe if already running */ - if(work) + if (work) schedule_delayed_work(&tty->flip.work, 1); return retval; }