Skip to content

Commit

Permalink
tty: rewrite the ldisc locking
Browse files Browse the repository at this point in the history
There are several pretty much unfixable races in the old ldisc code, especially
with respect to pty behaviour and also to hangup. It's easier to rewrite the
code than simply try and patch it up.

This patch
- splits the ldisc from the tty (so we will be able to refcount it more cleanly
  later)
- introduces a mutex lock for ldisc changing on an active device
- fixes the complete mess that hangup caused
- implements hopefully correct setldisc/close/hangup locking

There are still some problems around pty pairs that have always been there but
at least it is now possible to understand the code and fix further problems.

This fixes the following known bugs
- hang up can leak ldisc references
- hang up may not call open/close on ldisc in a matched way
- pty/tty pairs can deadlock during an ldisc change
- reading the ldisc proc files can cause every ldisc to be loaded

and probably a few other of the mysterious ldisc race reports.

I'm sure it also adds the odd new one.

Signed-off-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
  • Loading branch information
Alan Cox authored and Linus Torvalds committed Jun 11, 2009
1 parent e8b70e7 commit c65c9bc
Show file tree
Hide file tree
Showing 11 changed files with 330 additions and 254 deletions.
4 changes: 2 additions & 2 deletions drivers/bluetooth/hci_ldisc.c
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
/* FIXME: why is this needed. Note don't use ldisc_ref here as the
open path is before the ldisc is referencable */

if (tty->ldisc.ops->flush_buffer)
tty->ldisc.ops->flush_buffer(tty);
if (tty->ldisc->ops->flush_buffer)
tty->ldisc->ops->flush_buffer(tty);
tty_driver_flush_buffer(tty);

return 0;
Expand Down
2 changes: 1 addition & 1 deletion drivers/char/cyclades.c
Original file line number Diff line number Diff line change
Expand Up @@ -5200,7 +5200,7 @@ static int cyclades_proc_show(struct seq_file *m, void *v)
(cur_jifs - info->idle_stats.recv_idle)/
HZ, info->idle_stats.overruns,
/* FIXME: double check locking */
(long)info->port.tty->ldisc.ops->num);
(long)info->port.tty->ldisc->ops->num);
else
seq_printf(m, "%3d %8lu %10lu %8lu "
"%10lu %8lu %9lu %6ld\n",
Expand Down
4 changes: 2 additions & 2 deletions drivers/char/epca.c
Original file line number Diff line number Diff line change
Expand Up @@ -2114,8 +2114,8 @@ static int pc_ioctl(struct tty_struct *tty, struct file *file,
tty_wait_until_sent(tty, 0);
} else {
/* ldisc lock already held in ioctl */
if (tty->ldisc.ops->flush_buffer)
tty->ldisc.ops->flush_buffer(tty);
if (tty->ldisc->ops->flush_buffer)
tty->ldisc->ops->flush_buffer(tty);
}
unlock_kernel();
/* Fall Thru */
Expand Down
4 changes: 2 additions & 2 deletions drivers/char/ip2/i2lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -868,11 +868,11 @@ i2Input(i2ChanStrPtr pCh)
amountToMove = count;
}
// Move the first block
pCh->pTTY->ldisc.ops->receive_buf( pCh->pTTY,
pCh->pTTY->ldisc->ops->receive_buf( pCh->pTTY,
&(pCh->Ibuf[stripIndex]), NULL, amountToMove );
// If we needed to wrap, do the second data move
if (count > amountToMove) {
pCh->pTTY->ldisc.ops->receive_buf( pCh->pTTY,
pCh->pTTY->ldisc->ops->receive_buf( pCh->pTTY,
pCh->Ibuf, NULL, count - amountToMove );
}
// Bump and wrap the stripIndex all at once by the amount of data read. This
Expand Down
4 changes: 2 additions & 2 deletions drivers/char/ip2/ip2main.c
Original file line number Diff line number Diff line change
Expand Up @@ -1315,8 +1315,8 @@ static inline void isig(int sig, struct tty_struct *tty, int flush)
if (tty->pgrp)
kill_pgrp(tty->pgrp, sig, 1);
if (flush || !L_NOFLSH(tty)) {
if ( tty->ldisc.ops->flush_buffer )
tty->ldisc.ops->flush_buffer(tty);
if ( tty->ldisc->ops->flush_buffer )
tty->ldisc->ops->flush_buffer(tty);
i2InputFlush( tty->driver_data );
}
}
Expand Down
4 changes: 2 additions & 2 deletions drivers/char/n_hdlc.c
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,8 @@ static int n_hdlc_tty_open (struct tty_struct *tty)
#endif

/* Flush any pending characters in the driver and discipline. */
if (tty->ldisc.ops->flush_buffer)
tty->ldisc.ops->flush_buffer(tty);
if (tty->ldisc->ops->flush_buffer)
tty->ldisc->ops->flush_buffer(tty);

tty_driver_flush_buffer(tty);

Expand Down
10 changes: 5 additions & 5 deletions drivers/char/pty.c
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf,
c = to->receive_room;
if (c > count)
c = count;
to->ldisc.ops->receive_buf(to, buf, NULL, c);
to->ldisc->ops->receive_buf(to, buf, NULL, c);

return c;
}
Expand Down Expand Up @@ -148,11 +148,11 @@ static int pty_chars_in_buffer(struct tty_struct *tty)
int count;

/* We should get the line discipline lock for "tty->link" */
if (!to || !to->ldisc.ops->chars_in_buffer)
if (!to || !to->ldisc->ops->chars_in_buffer)
return 0;

/* The ldisc must report 0 if no characters available to be read */
count = to->ldisc.ops->chars_in_buffer(to);
count = to->ldisc->ops->chars_in_buffer(to);

if (tty->driver->subtype == PTY_TYPE_SLAVE)
return count;
Expand Down Expand Up @@ -186,8 +186,8 @@ static void pty_flush_buffer(struct tty_struct *tty)
if (!to)
return;

if (to->ldisc.ops->flush_buffer)
to->ldisc.ops->flush_buffer(to);
if (to->ldisc->ops->flush_buffer)
to->ldisc->ops->flush_buffer(to);

if (to->packet) {
spin_lock_irqsave(&tty->ctrl_lock, flags);
Expand Down
2 changes: 1 addition & 1 deletion drivers/char/selection.c
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ int paste_selection(struct tty_struct *tty)
}
count = sel_buffer_lth - pasted;
count = min(count, tty->receive_room);
tty->ldisc.ops->receive_buf(tty, sel_buffer + pasted,
tty->ldisc->ops->receive_buf(tty, sel_buffer + pasted,
NULL, count);
pasted += count;
}
Expand Down
64 changes: 8 additions & 56 deletions drivers/char/tty_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -491,22 +491,6 @@ void tty_ldisc_flush(struct tty_struct *tty)

EXPORT_SYMBOL_GPL(tty_ldisc_flush);

/**
* tty_reset_termios - reset terminal state
* @tty: tty to reset
*
* Restore a terminal to the driver default state
*/

static void tty_reset_termios(struct tty_struct *tty)
{
mutex_lock(&tty->termios_mutex);
*tty->termios = tty->driver->init_termios;
tty->termios->c_ispeed = tty_termios_input_baud_rate(tty->termios);
tty->termios->c_ospeed = tty_termios_baud_rate(tty->termios);
mutex_unlock(&tty->termios_mutex);
}

/**
* do_tty_hangup - actual handler for hangup events
* @work: tty device
Expand Down Expand Up @@ -536,7 +520,6 @@ static void do_tty_hangup(struct work_struct *work)
struct file *cons_filp = NULL;
struct file *filp, *f = NULL;
struct task_struct *p;
struct tty_ldisc *ld;
int closecount = 0, n;
unsigned long flags;
int refs = 0;
Expand Down Expand Up @@ -567,40 +550,8 @@ static void do_tty_hangup(struct work_struct *work)
filp->f_op = &hung_up_tty_fops;
}
file_list_unlock();
/*
* FIXME! What are the locking issues here? This may me overdoing
* things... This question is especially important now that we've
* removed the irqlock.
*/
ld = tty_ldisc_ref(tty);
if (ld != NULL) {
/* We may have no line discipline at this point */
if (ld->ops->flush_buffer)
ld->ops->flush_buffer(tty);
tty_driver_flush_buffer(tty);
if ((test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) &&
ld->ops->write_wakeup)
ld->ops->write_wakeup(tty);
if (ld->ops->hangup)
ld->ops->hangup(tty);
}
/*
* FIXME: Once we trust the LDISC code better we can wait here for
* ldisc completion and fix the driver call race
*/
wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
wake_up_interruptible_poll(&tty->read_wait, POLLIN);
/*
* Shutdown the current line discipline, and reset it to
* N_TTY.
*/
if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS)
tty_reset_termios(tty);
/* Defer ldisc switch */
/* tty_deferred_ldisc_switch(N_TTY);

This should get done automatically when the port closes and
tty_release is called */
tty_ldisc_hangup(tty);

read_lock(&tasklist_lock);
if (tty->session) {
Expand Down Expand Up @@ -629,12 +580,15 @@ static void do_tty_hangup(struct work_struct *work)
read_unlock(&tasklist_lock);

spin_lock_irqsave(&tty->ctrl_lock, flags);
tty->flags = 0;
clear_bit(TTY_THROTTLED, &tty->flags);
clear_bit(TTY_PUSH, &tty->flags);
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
put_pid(tty->session);
put_pid(tty->pgrp);
tty->session = NULL;
tty->pgrp = NULL;
tty->ctrl_status = 0;
set_bit(TTY_HUPPED, &tty->flags);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);

/* Account for the p->signal references we killed */
Expand All @@ -660,10 +614,7 @@ static void do_tty_hangup(struct work_struct *work)
* can't yet guarantee all that.
*/
set_bit(TTY_HUPPED, &tty->flags);
if (ld) {
tty_ldisc_enable(tty);
tty_ldisc_deref(ld);
}
tty_ldisc_enable(tty);
unlock_kernel();
if (f)
fput(f);
Expand Down Expand Up @@ -2570,7 +2521,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
case TIOCGSID:
return tiocgsid(tty, real_tty, p);
case TIOCGETD:
return put_user(tty->ldisc.ops->num, (int __user *)p);
return put_user(tty->ldisc->ops->num, (int __user *)p);
case TIOCSETD:
return tiocsetd(tty, p);
/*
Expand Down Expand Up @@ -2785,6 +2736,7 @@ void initialize_tty_struct(struct tty_struct *tty,
tty->buf.head = tty->buf.tail = NULL;
tty_buffer_init(tty);
mutex_init(&tty->termios_mutex);
mutex_init(&tty->ldisc_mutex);
init_waitqueue_head(&tty->write_wait);
init_waitqueue_head(&tty->read_wait);
INIT_WORK(&tty->hangup_work, do_tty_hangup);
Expand Down
Loading

0 comments on commit c65c9bc

Please sign in to comment.