[PATCH 1/1] Char: vt, make sysfs operations atomic

[PATCH 1/1] Char: vt, make sysfs operations atomic

[Jiri Slaby]

Hold console sem while creating/destroying sysfs files. Serialisation is
so far done by BKL held in tty release_dev and chrdev_open, but no other
locks are held in open path.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/char/vt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index fa1ffbf..0043cff 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -2746,8 +2746,8 @@ static int con_open(struct tty_struct *tty, struct file *filp)
 				tty->termios->c_iflag |= IUTF8;
 			else
 				tty->termios->c_iflag &= ~IUTF8;
-			release_console_sem();
 			vcs_make_sysfs(tty);
+			release_console_sem();
 			return ret;
 		}
 	}
@@ -2772,8 +2772,8 @@ static void con_close(struct tty_struct *tty, struct file *filp)
 		if (vc)
 			vc->vc_tty = NULL;
 		tty->driver_data = NULL;
-		release_console_sem();
 		vcs_remove_sysfs(tty);
+		release_console_sem();
 		mutex_unlock(&tty_mutex);
 		/*
 		 * tty_mutex is released, but we still hold BKL, so there is
-- 
1.5.4.5

Re: [PATCH 1/1] Char: vt, make sysfs operations atomic

[Alan Cox]

On Mon, 26 May 2008 13:53:32 +0200
Jiri Slaby <jirislaby@gmail.com> wrote:

> Hold console sem while creating/destroying sysfs files. Serialisation is
> so far done by BKL held in tty release_dev and chrdev_open, but no other
> locks are held in open path.

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH 1/1] Char: vt, make sysfs operations atomic

[Aristeu Rozanski]

> Hold console sem while creating/destroying sysfs files. Serialisation is
> so far done by BKL held in tty release_dev and chrdev_open, but no other
> locks are held in open path.
> 
>  				tty->termios->c_iflag |= IUTF8;
>  			else
>  				tty->termios->c_iflag &= ~IUTF8;
> -			release_console_sem();
>  			vcs_make_sysfs(tty);
> +			release_console_sem();
>  			return ret;
>  		}
>  	}
> @@ -2772,8 +2772,8 @@ static void con_close(struct tty_struct *tty, struct file *filp)
>  		if (vc)
>  			vc->vc_tty = NULL;
>  		tty->driver_data = NULL;
> -		release_console_sem();
>  		vcs_remove_sysfs(tty);
> +		release_console_sem();
>  		mutex_unlock(&tty_mutex);
>  		/*
>  		 * tty_mutex is released, but we still hold BKL, so there is
the reason for the code be the way it is is because vcs_{add,remove}_sysfs()
may sleep

-- 
Aristeu

Re: [PATCH 1/1] Char: vt, make sysfs operations atomic

[Jiri Slaby]

On 05/27/2008 09:43 PM, Aristeu Rozanski wrote:
>> Hold console sem while creating/destroying sysfs files. Serialisation is
>> so far done by BKL held in tty release_dev and chrdev_open, but no other
>> locks are held in open path.
>>
>>  				tty->termios->c_iflag |= IUTF8;
>>  			else
>>  				tty->termios->c_iflag &= ~IUTF8;
>> -			release_console_sem();
>>  			vcs_make_sysfs(tty);
>> +			release_console_sem();
>>  			return ret;
>>  		}
>>  	}
>> @@ -2772,8 +2772,8 @@ static void con_close(struct tty_struct *tty, struct file *filp)
>>  		if (vc)
>>  			vc->vc_tty = NULL;
>>  		tty->driver_data = NULL;
>> -		release_console_sem();
>>  		vcs_remove_sysfs(tty);
>> +		release_console_sem();
>>  		mutex_unlock(&tty_mutex);
>>  		/*
>>  		 * tty_mutex is released, but we still hold BKL, so there is
> the reason for the code be the way it is is because vcs_{add,remove}_sysfs()
> may sleep

What's the point? To have races in the code but not sleep inside the semaphore?

Re: [PATCH 1/1] Char: vt, make sysfs operations atomic

[Aristeu Rozanski]

>> the reason for the code be the way it is is because vcs_{add,remove}_sysfs()
>> may sleep
>
> What's the point? To have races in the code but not sleep inside the semaphore?
what about fixing the code to remove the race _and_ not sleep inside the
semaphore? :)

-- 
Aristeu

Re: [PATCH 1/1] Char: vt, make sysfs operations atomic

[Andrew Morton]

On Tue, 27 May 2008 16:48:58 -0400
Aristeu Rozanski <aris@ruivo.org> wrote:

> >> the reason for the code be the way it is is because vcs_{add,remove}_sysfs()
> >> may sleep
> >
> > What's the point? To have races in the code but not sleep inside the semaphore?
> what about fixing the code to remove the race _and_ not sleep inside the
> semaphore? :)
> 

The patch does fix a race, by extending console_sem coverage to provide
exclusion between the sysfs creation and teardown operations.

I assume - no race was identified in the changelog.  Can we actually
simultaneously run con_open() and con_close() against the same device? 
I guess it might be possible if userspace tried hard enough.

I renamed the patch to the much more accurate "vt: hold console_sem
across sysfs operations" - "atomic" in the kernel context means "cannot
context switch" and nothing "atomic" is happening in this patch.