Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit ff55fe20 authored by Jason Baron's avatar Jason Baron Committed by Linus Torvalds
Browse files

[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: default avatarJason Baron <jbaron@redhat.com>
Signed-off-by: default avatarAndrew Morton <akpm@osdl.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@osdl.org>
parent 69ac5964

drivers/char/pty.c

+2 −3
Original line number Diff line number Diff line
@@ -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;

drivers/char/tty_io.c

+54 −25
Original line number Diff line number Diff line
@@ -475,15 +475,13 @@ static int tty_set_ldisc(struct tty_struct *tty, int ldisc) 
	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,10 +492,16 @@ static int tty_set_ldisc(struct tty_struct *tty, int ldisc) 
	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
@@ -506,8 +510,8 @@ static int tty_set_ldisc(struct tty_struct *tty, int ldisc) 
	 */



	spin_lock_irqsave(&tty_ldisc_lock, flags);

	if(tty->ldisc.refcount)

	{

	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);
@@ -523,8 +527,31 @@ static int tty_set_ldisc(struct tty_struct *tty, int ldisc) 
				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);

		ld = tty_ldisc_ref_wait(tty);

		tty_ldisc_deref(ld);

		goto restart;

	}



	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);



	/*
@@ -583,6 +610,8 @@ static int tty_set_ldisc(struct tty_struct *tty, int ldisc) 
	 */

	 

	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 */