Re: [PATCH] Avoid a race in complete_change_console()

Re: [PATCH] Avoid a race in complete_change_console()

[PinkFreud]

Chris, THANK YOU!  I've been having a problem on my SMP system for months
now, where if I started X, switched to a text console, and then back to X,
the system would lock up.  Your patch actually fixed it!

Alan, could you please make sure this patch makes it into the kernel?


> Currently drivers/char/vt.c:complete_change_console() calls
> do_blank_screen() when switching from text to graphics mode after
> sending acqsig to the controlling process of the new console.
>
> On an SMP system, acqsig can wake the controlling process up and get it
> working before do_blank_screen completes.  At least on my ia64 systems,
> this causes accesses to the VGA range from the kernel and accesses to
> framebuffer from X to overlap with one another causing system lockups.
>
> Appended is a patch which ensures that do_blank_screen() is called
> before delivering acqsig instead of after.  do_blank_screen() is still
> called if signal delivery fails (as far as I could tell, handling this
> case was the reason screen blanking was done after signal delivery in
> the first place).
>
> Can someone familiar with this code comment on the correctness of this
> patch?  The patch is against vanilla 2.4.13.  Thanks,
>
> Chris
>
>
> --- linux-2.4.13-ia64-011024-pristine/drivers/char/vt.c       Mon Sep 17 22:52:35 2001
> +++ linux-2.4.13-ia64-011024-dev/drivers/char/vt.c    Thu Oct 25 20:46:08 2001
> @@ -1184,6 +1184,24 @@
>       switch_screen(new_console);
>
>       /*
> +      * This can't appear below a successful kill_proc().  If it did,
> +      * then the *blank_screen operation could occur while X, having
> +      * received acqsig, is waking up on another processor.  This
> +      * condition can lead to overlapping accesses to the VGA range
> +      * and the framebuffer (causing system lockups).
> +      *
> +      * To account for this we duplicate this code below only if the
> +      * controlling process is gone and we've called reset_vc.
> +      */
> +     if (old_vc_mode != vt_cons[new_console]->vc_mode)
> +     {
> +             if (vt_cons[new_console]->vc_mode == KD_TEXT)
> +                     unblank_screen();
> +             else
> +                     do_blank_screen(1);
> +     }
> +
> +     /*
>        * If this new console is under process control, send it a signal
>        * telling it that it has acquired. Also check if it has died and
>        * clean up (similar to logic employed in change_console())
> @@ -1209,19 +1227,15 @@
>                * to account for and tracking tty count may be undesirable.
>                */
>                       reset_vc(new_console);
> -             }
> -     }
>
> -     /*
> -      * We do this here because the controlling process above may have
> -      * gone, and so there is now a new vc_mode
> -      */
> -     if (old_vc_mode != vt_cons[new_console]->vc_mode)
> -     {
> -             if (vt_cons[new_console]->vc_mode == KD_TEXT)
> -                     unblank_screen();
> -             else
> -                     do_blank_screen(1);
> +                     if (old_vc_mode != vt_cons[new_console]->vc_mode)
> +                     {
> +                             if (vt_cons[new_console]->vc_mode == KD_TEXT)
> +                                     unblank_screen();
> +                             else
> +                                     do_blank_screen(1);
> +                     }
> +             }
>       }
>
>       /*




	Mike Edwards

Brainbench certified Master Linux Administrator
http://www.brainbench.com/transcript.jsp?pid=158188
-----------------------------------
Unsolicited advertisments to this address are not welcome.

Re: [PATCH] Avoid a race in complete_change_console()

[Alan Cox]

> Chris, THANK YOU!  I've been having a problem on my SMP system for months
> now, where if I started X, switched to a text console, and then back to X,
> the system would lock up.  Your patch actually fixed it!
> 
> Alan, could you please make sure this patch makes it into the kernel?

Already in my tree

Re: [PATCH] Avoid a race in complete_change_console()

[Mike Fedyk]

On Sun, Oct 28, 2001 at 12:25:14PM -0500, PinkFreud wrote:
> Chris, THANK YOU!  I've been having a problem on my SMP system for months
> now, where if I started X, switched to a text console, and then back to X,
> the system would lock up.  Your patch actually fixed it!
> 

I have seen this on 2.2 also.  Maybe it should be backported...

I've since changed my video card (from number nine t2r to permedia II),
upgraded X, and stopped switching from X to console so much...  So, I
haven't come across this problem since.

Mike

[PATCH] Avoid a race in complete_change_console()

[Chris Ahna]

Currently drivers/char/vt.c:complete_change_console() calls
do_blank_screen() when switching from text to graphics mode after
sending acqsig to the controlling process of the new console.

On an SMP system, acqsig can wake the controlling process up and get it
working before do_blank_screen completes.  At least on my ia64 systems,
this causes accesses to the VGA range from the kernel and accesses to
framebuffer from X to overlap with one another causing system lockups.

Appended is a patch which ensures that do_blank_screen() is called
before delivering acqsig instead of after.  do_blank_screen() is still
called if signal delivery fails (as far as I could tell, handling this
case was the reason screen blanking was done after signal delivery in
the first place).

Can someone familiar with this code comment on the correctness of this
patch?  The patch is against vanilla 2.4.13.  Thanks,

Chris


--- linux-2.4.13-ia64-011024-pristine/drivers/char/vt.c	Mon Sep 17 22:52:35 2001
+++ linux-2.4.13-ia64-011024-dev/drivers/char/vt.c	Thu Oct 25 20:46:08 2001
@@ -1184,6 +1184,24 @@
 	switch_screen(new_console);
 
 	/*
+	 * This can't appear below a successful kill_proc().  If it did,
+	 * then the *blank_screen operation could occur while X, having
+	 * received acqsig, is waking up on another processor.  This
+	 * condition can lead to overlapping accesses to the VGA range
+	 * and the framebuffer (causing system lockups).
+	 *
+	 * To account for this we duplicate this code below only if the
+	 * controlling process is gone and we've called reset_vc.
+	 */
+	if (old_vc_mode != vt_cons[new_console]->vc_mode)
+	{
+		if (vt_cons[new_console]->vc_mode == KD_TEXT)
+			unblank_screen();
+		else
+			do_blank_screen(1);
+	}
+
+	/*
 	 * If this new console is under process control, send it a signal
 	 * telling it that it has acquired. Also check if it has died and
 	 * clean up (similar to logic employed in change_console())
@@ -1209,19 +1227,15 @@
 		 * to account for and tracking tty count may be undesirable.
 		 */
 		        reset_vc(new_console);
-		}
-	}
 
-	/*
-	 * We do this here because the controlling process above may have
-	 * gone, and so there is now a new vc_mode
-	 */
-	if (old_vc_mode != vt_cons[new_console]->vc_mode)
-	{
-		if (vt_cons[new_console]->vc_mode == KD_TEXT)
-			unblank_screen();
-		else
-			do_blank_screen(1);
+			if (old_vc_mode != vt_cons[new_console]->vc_mode)
+			{
+				if (vt_cons[new_console]->vc_mode == KD_TEXT)
+					unblank_screen();
+				else
+					do_blank_screen(1);
+			}
+		}
 	}
 
 	/*

Re: [PATCH] Avoid a race in complete_change_console()

[Andrea Arcangeli]

On Thu, Oct 25, 2001 at 09:07:44PM -0700, Chris Ahna wrote:
> Can someone familiar with this code comment on the correctness of this
> patch?  The patch is against vanilla 2.4.13.  Thanks,

I'm not very familiar with it, but it seems sane fix. Only detail is
that the vc_mode can only be KD_TEXT after the reset_vc but the
additional check doesn't hurt and it makes it indipendent by the
reset_vc details.

Andrea