2001-10-30 01:04:18

by Robert Love

Subject: [PATCH] tty race on con_close and con_flush_chars

There is a race in the console code between con_close and
con_flush_chars. n_tty_receive_buf writes to the tty queue and then
writes it out via con_flush_chars. The race arises in between the above
two operations; the console can close and thus zero tty->drive_data.
When con_flush_chars runs, it will dereference a null pointer.

The following fix, by Andrew Morton, merely checks if the tty still
exists because continuing. I am submitting the patch because the race
is uncovered often with a preemptive kernel. The fix is in the preempt
tree, but it should be pushed to mainline since it should affect SMP
too.

Linus and Alan, please apply.

diff -urN linux-2.4.13-ac5/drivers/char/console.c linux/drivers/char/console.c
--- linux-2.4.13-ac5/drivers/char/console.c Mon Oct 29 17:27:19 2001
+++ linux/drivers/char/console.c Mon Oct 29 17:28:24 2001
@@ -2387,9 +2387,15 @@
return;

pm_access(pm_con);
- acquire_console_sem();
- set_cursor(vt->vc_num);
- release_console_sem();
+ if (vt) {
+ /*
+ * If we raced with con_close(), `vt' may be null.
+ * Hence this bandaid. - akpm
+ */
+ acquire_console_sem();
+ set_cursor(vt->vc_num);
+ release_console_sem();
+ }
}

/*

Robert Love

2001-10-30 03:13:37

by Robert Love

Subject: Re: [PATCH] tty race on con_close and con_flush_chars

Someone pointed out (via private email) that a race can still exist
between checking that vt is non-zero and acquiring the semaphore,
especially since we can sleep doing that.

I agree; the following patch should alleviate that race.

diff -u linux-2.4.13-ac5/drivers/char/console.c
linux/drivers/char/console.c
--- linux-2.4.13-ac5/drivers/char/console.c Mon Oct 29 17:27:19 2001
+++ linux/drivers/char/console.c Mon Oct 29 22:11:27 2001
@@ -2387,8 +2387,14 @@
return;

pm_access(pm_con);
+
+ /*
+ * If we raced with con_close(), `vt' may be null.
+ * Hence this bandaid. - akpm
+ */
acquire_console_sem();
- set_cursor(vt->vc_num);
+ if (vt)
+ set_cursor(vt->vc_num);
release_console_sem();
}


Robert Love

2001-10-31 05:08:34

by Tachino Nobuhiro

Subject: Re: [PATCH] tty race on con_close and con_flush_chars

Hello,

At 29 Oct 2001 22:13:55 -0500,
Robert Love wrote:
>
> Someone pointed out (via private email) that a race can still exist
> between checking that vt is non-zero and acquiring the semaphore,
> especially since we can sleep doing that.
>
> I agree; the following patch should alleviate that race.

Your patch only fixes con_flush_chars(), but the same races exists in
do_con_write() and maybe con_unthrottle(). I think the patch below is
better because the oops would never happen with this patch.


diff -r -u -N linux.org/drivers/char/console.c linux/drivers/char/console.c
--- linux.org/drivers/char/console.c Tue Oct 30 18:55:17 2001
+++ linux/drivers/char/console.c Tue Oct 30 19:07:08 2001
@@ -2424,7 +2424,6 @@
return;
if (tty->count != 1) return;
vcs_make_devfs (MINOR (tty->device) - tty->driver.minor_start, 1);
- tty->driver_data = 0;
}

static void vc_init(unsigned int currcons, unsigned int rows, unsigned int cols, int do_clear)



>
> diff -u linux-2.4.13-ac5/drivers/char/console.c
> linux/drivers/char/console.c
> --- linux-2.4.13-ac5/drivers/char/console.c Mon Oct 29 17:27:19 2001
> +++ linux/drivers/char/console.c Mon Oct 29 22:11:27 2001
> @@ -2387,8 +2387,14 @@
> return;
>
> pm_access(pm_con);
> +
> + /*
> + * If we raced with con_close(), `vt' may be null.
> + * Hence this bandaid. - akpm
> + */
> acquire_console_sem();
> - set_cursor(vt->vc_num);
> + if (vt)
> + set_cursor(vt->vc_num);
> release_console_sem();
> }
>
>
> Robert Love
>