[PATCH] Remove race between con_open and con_close

[PATCH] Remove race between con_open and con_close

[Paul Mackerras]

I have a laptop (G3 powerbook) which will pretty reliably hit a race
between con_open and con_close late in the boot process and oops in
vt_ioctl due to tty->driver_data being NULL.

What happens is this: process A opens /dev/tty6; it comes into
con_open() (drivers/char/vt.c) and assign a non-NULL value to
tty->driver_data.  Then process A closes that and concurrently process
B opens /dev/tty6.  Process A gets through con_close() and clears
tty->driver_data, since tty->count == 1.  However, before process A
can decrement tty->count, we switch to process B (e.g. at the
down(&tty_sem) call at drivers/char/tty_io.c line 1626).

So process B gets to run and comes into con_open with tty->count == 2,
as tty->count is incremented (in init_dev) before con_open is called.
Because tty->count != 1, we don't set tty->driver_data.  Then when the
process tries to do anything with that fd, it oopses.

The simple and effective fix for this is to test tty->driver_data
rather than tty->count in con_open.  The testing and setting of
tty->driver_data is serialized with respect to the clearing of
tty->driver_data in con_close by the console_sem.  We can't get a
situation where con_open sees tty->driver_data != NULL and then
con_close on a different fd clears tty->driver_data, because
tty->count is incremented before con_open is called.  Thus this patch
eliminates the race, and in fact with this patch my laptop doesn't
oops.

Could this go into 2.6.13 please?

Signed-off-by: Paul Mackerras <paulus@samba.org>

diff -urN linux-2.6/drivers/char/vt.c pmac-2.6/drivers/char/vt.c
--- linux-2.6/drivers/char/vt.c	2005-07-17 10:59:52.000000000 +1000
+++ pmac-2.6/drivers/char/vt.c	2005-08-27 22:59:36.000000000 +1000
@@ -2433,7 +2433,7 @@
 	int ret = 0;
 
 	acquire_console_sem();
-	if (tty->count == 1) {
+	if (tty->driver_data == NULL) {
 		ret = vc_allocate(currcons);
 		if (ret == 0) {
 			struct vc_data *vc = vc_cons[currcons].d;

Re: [PATCH] Remove race between con_open and con_close

[Antonino A. Daplas]

Paul Mackerras wrote:
> I have a laptop (G3 powerbook) which will pretty reliably hit a race
> between con_open and con_close late in the boot process and oops in
> vt_ioctl due to tty->driver_data being NULL.
> 
> What happens is this: process A opens /dev/tty6; it comes into
> con_open() (drivers/char/vt.c) and assign a non-NULL value to
> tty->driver_data.  Then process A closes that and concurrently process
> B opens /dev/tty6.  Process A gets through con_close() and clears
> tty->driver_data, since tty->count == 1.  However, before process A
> can decrement tty->count, we switch to process B (e.g. at the
> down(&tty_sem) call at drivers/char/tty_io.c line 1626).
> 
> So process B gets to run and comes into con_open with tty->count == 2,
> as tty->count is incremented (in init_dev) before con_open is called.
> Because tty->count != 1, we don't set tty->driver_data.  Then when the
> process tries to do anything with that fd, it oopses.
> 
> The simple and effective fix for this is to test tty->driver_data
> rather than tty->count in con_open.  The testing and setting of
> tty->driver_data is serialized with respect to the clearing of
> tty->driver_data in con_close by the console_sem.  We can't get a
> situation where con_open sees tty->driver_data != NULL and then
> con_close on a different fd clears tty->driver_data, because
> tty->count is incremented before con_open is called.  Thus this patch
> eliminates the race, and in fact with this patch my laptop doesn't
> oops.
> 
> Could this go into 2.6.13 please?

I agree this should go to 2.6.13.  Though you've been beaten to the punch
by Steven Rostedt.  This is already in the mm tree.

http://marc.theaimsgroup.com/?l=linux-kernel&m=112450820432121&w=2

Tony

> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> 
> diff -urN linux-2.6/drivers/char/vt.c pmac-2.6/drivers/char/vt.c
> --- linux-2.6/drivers/char/vt.c	2005-07-17 10:59:52.000000000 +1000
> +++ pmac-2.6/drivers/char/vt.c	2005-08-27 22:59:36.000000000 +1000
> @@ -2433,7 +2433,7 @@
>  	int ret = 0;
>  
>  	acquire_console_sem();
> -	if (tty->count == 1) {
> +	if (tty->driver_data == NULL) {
>  		ret = vc_allocate(currcons);
>  		if (ret == 0) {
>  			struct vc_data *vc = vc_cons[currcons].d;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] Remove race between con_open and con_close

[Russell King]

On Sun, Aug 28, 2005 at 09:40:01AM +1000, Paul Mackerras wrote:
> I have a laptop (G3 powerbook) which will pretty reliably hit a race
> between con_open and con_close late in the boot process and oops in
> vt_ioctl due to tty->driver_data being NULL.

Have you looked at how serial_core handles this kind of problem in
its open and close methods?  I put some comments in there because
of the issue, after thinking about it fairly carefully.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Remove race between con_open and con_close

[Paul Mackerras]

Russell King writes:

> Have you looked at how serial_core handles this kind of problem in
> its open and close methods?  I put some comments in there because
> of the issue, after thinking about it fairly carefully.

Yes, albeit briefly; the problem in con_open is much simpler because
we never need to block.

Paul.