[uml-devel] bugs in line.c and stdio_console.c

[uml-devel] bugs in line.c and stdio_console.c

[Yu Dong-W4760C]

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

same bugs in the two files (uml-linux-2.4.26-3):
arch/um/drivers/line.c
int line_write(struct line *lines, struct tty_struct *tty, int from_user,
        const char *buf, int len)
{
 struct line *line;
 char *new;
 unsigned long flags;
 int n, err, i, ret = 0;
 
 if(tty->stopped) return 0;
 
 if(from_user){
  new = kmalloc(len, GFP_KERNEL);
  if(new == NULL)
   return(0);
  n = copy_from_user(new, buf, len);
  buf = new;
  if(n == len){
   len = -EFAULT;
   goto out_free;
  }
 
  len -= n;
 }
 
 i = minor(tty->device) - tty->driver.minor_start;
 line = &lines[i];
 
 down(&line->sem);  ========================> may cause shedule in interrrupt
 if(line->head != line->tail){
  local_irq_save(flags);
  ret += buffer_data(line, buf, len);
  err = flush_buffer(line);
  local_irq_restore(flags);
  if(err <= 0)
   goto out_up;
 }
 
 
in arch/um/drivers/stdio_console.c
static void console_write(struct console *console, const char *string, 
     unsigned len)
{
 struct line *line = &vts[console->index];
 
 if(con_init_done)
  down(&line->sem);  ========================> may cause shedule in interrrupt
 console_write_chan(&line->chan_list, string, len);
 if(con_init_done)
  up(&line->sem);
}
 
my fix is change
[w4760c@w4760c-x3 drivers]$ diff -Nru line.c line.c.old
--- line.c      2005-04-28 09:22:29.000000000 +0800
+++ line.c.old  2005-04-26 09:38:18.000000000 +0800
@@ -134,9 +134,7 @@
        i = minor(tty->device) - tty->driver.minor_start;
        line = &lines[i];
 
-       if(!in_interrupt())
-               down(&line->sem);
-
+       down(&line->sem);
        if(line->head != line->tail){
                local_irq_save(flags);
                ret += buffer_data(line, buf, len);
@@ -159,8 +157,7 @@
                        ret += buffer_data(line, buf + n, len);
        }
  out_up:
-       if(!in_interrupt())
-               up(&line->sem);
+       up(&line->sem);
 
  out_free:
        if(from_user)

 
--- stdio_console.c     2005-04-26 14:41:05.000000000 +0800
+++ stdio_console.c.bak 2005-04-26 14:33:30.000000000 +0800
@@ -188,10 +188,10 @@
 {
        struct line *line = &vts[console->index];
 
-       if(con_init_done && !in_interrupt())
+       if(con_init_done)
                down(&line->sem);
        console_write_chan(&line->chan_list, string, len);
-       if(con_init_done && !in_interrupt())
+       if(con_init_done)
                up(&line->sem);
 }
 
 
please help to confirm if it's correct?
 
best regards
Yu Dong
 
Tel: 010-65642141
Email:  w4760c@motorola.com <mailto:w4760c@motorola.com> 
PCS Beijing Design Center
Motorola(China) Electronics Ltd. 
 
 
 

[-- Attachment #2: Type: text/html, Size: 7520 bytes --]

Re: [uml-devel] bugs in line.c and stdio_console.c

[Blaisorblade]

On Thursday 28 April 2005 03:24, Yu Dong-W4760C wrote:
> same bugs in the two files (uml-linux-2.4.26-3):
> arch/um/drivers/line.c

>  down(&line->sem);  ========================> may cause shedule in
> interrrupt 

> in arch/um/drivers/stdio_console.c
> static void console_write(struct console *console, const char *string,
>      unsigned len)
> {

>  if(con_init_done)
>   down(&line->sem);  ========================> may cause shedule in
> interrrupt console_write_chan(&line->chan_list, string, len);
>  if(con_init_done)
>   up(&line->sem);
> }
>
> my fix is change
> [w4760c@w4760c-x3 drivers]$ diff -Nru line.c line.c.old
> --- line.c      2005-04-28 09:22:29.000000000 +0800
> +++ line.c.old  2005-04-26 09:38:18.000000000 +0800
> @@ -134,9 +134,7 @@
>         i = minor(tty->device) - tty->driver.minor_start;
>         line = &lines[i];
>
> -       if(!in_interrupt())
> -               down(&line->sem);
> -
> +       down(&line->sem);
>         if(line->head != line->tail){
>                 local_irq_save(flags);
>                 ret += buffer_data(line, buf, len);
> @@ -159,8 +157,7 @@
>                         ret += buffer_data(line, buf + n, len);
>         }
>   out_up:
> -       if(!in_interrupt())
> -               up(&line->sem);
> +       up(&line->sem);
>
>   out_free:
>         if(from_user)
>
>
> --- stdio_console.c     2005-04-26 14:41:05.000000000 +0800
> +++ stdio_console.c.bak 2005-04-26 14:33:30.000000000 +0800
> @@ -188,10 +188,10 @@
>  {
>         struct line *line = &vts[console->index];
>
> -       if(con_init_done && !in_interrupt())
> +       if(con_init_done)
>                 down(&line->sem);
>         console_write_chan(&line->chan_list, string, len);
> -       if(con_init_done && !in_interrupt())
> +       if(con_init_done)
>                 up(&line->sem);
>  }
>

> please help to confirm if it's correct?

1) The patch is reversed, and after noticing this it seems a bit more 
meaningful.

2)But that's not the correct fix, because data corruption is as bad as 
schedule in interrupt.

3) The correct fix (which I just sent in with the title "uml: redo console 
locking" to this list) is to replace that semaphore with a spinlock.
-- 
Paolo Giarrusso, aka Blaisorblade
Skype user "PaoloGiarrusso"
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade




-------------------------------------------------------
SF.Net email is sponsored by: Tell us your software development plans!
Take this survey and enter to win a one-year sub to SourceForge.net
Plus IDC's 2005 look-ahead and a copy of this survey
Click here to start!  http://www.idcswdc.com/cgi-bin/survey?id=105hix
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel