diff options
author | Jason Baron <jbaron@redhat.com> | 2005-09-09 20:01:57 (GMT) |
---|---|---|
committer | Linus Torvalds <torvalds@g5.osdl.org> | 2005-09-09 20:57:31 (GMT) |
commit | ff55fe2075e3901db4dbdc00e0b44a71bef97afd (patch) | |
tree | c6dfc8ba5d04fda4c5cb15cdebd5425ab674f8df /drivers/char/tty_io.c | |
parent | 69ac59647e66c1b53fb98fe8b6d0f2099cffad60 (diff) | |
download | linux-ff55fe2075e3901db4dbdc00e0b44a71bef97afd.tar.xz |
[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 <jbaron@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Diffstat (limited to 'drivers/char/tty_io.c')
-rw-r--r-- | drivers/char/tty_io.c | 79 |
1 files changed, 54 insertions, 25 deletions
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index 9d65712..6a56ae4 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 <ejbs@cs.cs.com.uy> */ /* Cyrus Durgin <cider@speakeasy.org> */ @@ -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; } |