Re: 2.6.5-rc3-mm1

Andrew Morton
2004-03-31T08:52:08Z

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
> On Tue, Mar 30, 2004 at 06:26:27PM -0800, Andrew Morton wrote:
> > Andrew Morton <akpm@osdl.org> wrote:
> > >
> > > I'm thinking that this can be fixed from the other direction: just before
> > > release_dev() calls close (dropping BKL), if tty->count==1, make the
> > > going-away tty ineligible for concurrent lookups. Do that by setting
> > > tty->driver->ttys[idx] to NULL. Maybe.
> >
> > Famous last word: Volia!
>
> I suspect you may just be able to get away with this for serial drivers
> using serial_core. However, I suspect it'll break non-serial_core using
> serial drivers.
>
> The serial drivers track the tty count themselves, so that they know
> when to do the final close processing (why you may ask - because of
> the blocking for DCD in the open code.) I wouldn't like to say what
> would happen if ->open were called for a different tty structure for
> the same port while ->close was in progress.

You cannot defeat me that easily!

Actually the race+oops _was_ fixed in Linus's tree a week ago, sort-of. If
the race happens we end up running vcs_devfs_add() and vcs_devfs_remove()
against the tty concurrently, leaving it in indeterminate state. This will
cause devfs warnings and missing devfs/sysfs stuff, but no oops.

The con_open-speedup.patch in -mm reintroduces the oops by not bogusly
overwriting tty->driver_data when tty->count > 1. Fun.

This rather nasty patch fixes things up again.

25-akpm/drivers/char/tty_io.c | 2 +-
25-akpm/drivers/char/vt.c | 14 ++++++++++++++
25-akpm/include/linux/tty.h | 3 +++
3 files changed, 18 insertions(+), 1 deletion(-)

diff -puN drivers/char/vt.c~tty-race-fix-43 drivers/char/vt.c
--- 25/drivers/char/vt.c~tty-race-fix-43 2004-03-31 00:44:54.582831728 -0800
+++ 25-akpm/drivers/char/vt.c 2004-03-31 00:44:54.589830664 -0800
@@ -2480,8 +2480,16 @@ static int con_open(struct tty_struct *t
return ret;
}

+/*
+ * We take tty_sem in here to prevent another thread from coming in via init_dev
+ * and taking a ref against the tty while we're in the process of forgetting
+ * about it and cleaning things up.
+ *
+ * This is because vcs_remove_devfs() can sleep and will drop the BKL.
+ */
static void con_close(struct tty_struct *tty, struct file *filp)
{
+ down(&tty_sem);
acquire_console_sem();
if (tty && tty->count == 1) {
struct vt_struct *vt;
@@ -2492,9 +2500,15 @@ static void con_close(struct tty_struct
tty->driver_data = 0;
release_console_sem();
vcs_remove_devfs(tty);
+ up(&tty_sem);
+ /*
+ * tty_sem is released, but we still hold BKL, so there is
+ * still exclusion against init_dev()
+ */
return;
}
release_console_sem();
+ up(&tty_sem);
}

static void vc_init(unsigned int currcons, unsigned int rows,
diff -puN drivers/char/tty_io.c~tty-race-fix-43 drivers/char/tty_io.c
--- 25/drivers/char/tty_io.c~tty-race-fix-43 2004-03-31 00:44:54.584831424 -0800
+++ 25-akpm/drivers/char/tty_io.c 2004-03-31 00:44:54.591830360 -0800
@@ -123,7 +123,7 @@ LIST_HEAD(tty_drivers); /* linked list
struct tty_ldisc ldiscs[NR_LDISCS]; /* line disc dispatch table */

/* Semaphore to protect creating and releasing a tty */
-static DECLARE_MUTEX(tty_sem);
+DECLARE_MUTEX(tty_sem);

#ifdef CONFIG_UNIX98_PTYS
extern struct tty_driver *ptm_driver; /* Unix98 pty masters; for /dev/ptmx */
diff -puN include/linux/tty.h~tty-race-fix-43 include/linux/tty.h
--- 25/include/linux/tty.h~tty-race-fix-43 2004-03-31 00:44:54.586831120 -0800
+++ 25-akpm/include/linux/tty.h 2004-03-31 00:44:54.592830208 -0800
@@ -363,6 +363,9 @@ extern void tty_flip_buffer_push(struct
extern int tty_get_baud_rate(struct tty_struct *tty);
extern int tty_termios_baud_rate(struct termios *termios);

+struct semaphore;
+extern struct semaphore tty_sem;
+
/* n_tty.c */
extern struct tty_ldisc tty_ldisc_N_TTY;

_