A patch for the file kernel/fork.c

A patch for the file kernel/fork.c

[André Pereira de Almeida]

  I think that the file kernel/fork.c should be patched as following:

--- linux-2.6.12-rc3.orig/kernel/fork.c 2005-05-03 22:31:21.000000000 -0300
+++ linux-2.6.12-rc3/kernel/fork.c      2005-05-04 09:37:46.000000000 -0300
@@ -429,7 +429,7 @@
                tsk->vfork_done = NULL;
                complete(vfork_done);
        }
-       if (tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
+       if (mm && tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
                u32 __user * tidptr = tsk->clear_child_tid;
                tsk->clear_child_tid = NULL;

If a process is killed and, for some reason, when closing all files a 
pagefault is generated (should not happen, but...) after the function 
exit_mm had been called, the kernel will try to exit this process again, 
calling exit_mm and mm_release with a null mm, and will generate another 
pagefault in atomic_read(&mm->mm_users), and so on and the system will 
hang. I think this patch solve this problem.

Re: A patch for the file kernel/fork.c

[Chris Wedgwood]

On Wed, May 04, 2005 at 11:46:18AM -0300, Andr? Pereira de Almeida wrote:

> -       if (tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
> +       if (mm && tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {

did you actually see a problem here?

Re: A patch for the file kernel/fork.c

[André Pereira de Almeida]

Chris Wedgwood wrote:

>On Wed, May 04, 2005 at 11:46:18AM -0300, Andr? Pereira de Almeida wrote:
>
>  
>
>>-       if (tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
>>+       if (mm && tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
>>    
>>
>
>did you actually see a problem here?
>-
>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/
>  
>
In a preemptible kernel with the serport module and a serial port try to 
run the following program:

int main(int argc, char **argv)
{
        int ldisc,fd;

        fd = open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NONBLOCK);
        ldisc = N_MOUSE;
        ioctl(fd, TIOCSETD, &ldisc);
        read(fd, NULL, 0);
        return 0;
}

and kill it.
In my case it will hang the computer. I think this is a problem with the 
serport module. With this patch, the serial mouse stop working, but the 
computer don't hang.

Re: A patch for the file kernel/fork.c

[Chris Wedgwood]

On Wed, May 04, 2005 at 03:26:44PM -0300, Andr? Pereira de Almeida wrote:

> >>-       if (tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
> >>+       if (mm && tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1)

> In a preemptible kernel with the serport module and a serial port try to
> run the following program:
>
> int main(int argc, char **argv)
> {
>        int ldisc,fd;
>
>        fd = open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NONBLOCK);
>        ldisc = N_MOUSE;
>        ioctl(fd, TIOCSETD, &ldisc);
>        read(fd, NULL, 0);
>        return 0;
> }
>
> and kill it.  In my case it will hang the computer. I think this is
> a problem with the serport module. With this patch, the serial mouse
> stop working, but the computer don't hang.

then above something like:

     BUG_ON(!mm);

or something might be better and eyeball the stack trace.

Re: A patch for the file kernel/fork.c

[André Pereira de Almeida]

Chris Wedgwood wrote:

>On Wed, May 04, 2005 at 03:26:44PM -0300, Andr? Pereira de Almeida wrote:
>
>  
>
>>>>-       if (tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1) {
>>>>+       if (mm && tsk->clear_child_tid && atomic_read(&mm->mm_users) > 1)
>>>>        
>>>>
>
>  
>
>>In a preemptible kernel with the serport module and a serial port try to
>>run the following program:
>>
>>int main(int argc, char **argv)
>>{
>>       int ldisc,fd;
>>
>>       fd = open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NONBLOCK);
>>       ldisc = N_MOUSE;
>>       ioctl(fd, TIOCSETD, &ldisc);
>>       read(fd, NULL, 0);
>>       return 0;
>>}
>>
>>and kill it.  In my case it will hang the computer. I think this is
>>a problem with the serport module. With this patch, the serial mouse
>>stop working, but the computer don't hang.
>>    
>>
>
>then above something like:
>
>     BUG_ON(!mm);
>
>or something might be better and eyeball the stack trace.
>  
>
In my example, a stack trace will be already generated by an oops:
[4300748.423000] Unable to handle kernel paging request at virtual 
address 6b6b6b7b
[4300748.423000]  printing eip:
[4300748.423000] d0927366
[4300748.423000] *pde = 00000000
[4300748.423000] Oops: 0000 [#1]
[4300748.423000] PREEMPT
and so on, with a call trace:
[4300748.423000] Call Trace:
[4300748.423000]  [<c0104bfa>] show_stack+0x7a/0x90
[4300748.423000]  [<c0104d7d>] show_registers+0x14d/0x1b0
[4300748.423000]  [<c0104fcc>] die+0x14c/0x2c0
[4300748.423000]  [<c0118b6f>] do_page_fault+0x31f/0x638
[4300748.423000]  [<c01046df>] error_code+0x4f/0x54
[4300748.423000]  [<c02b88fd>] tty_wakeup+0x5d/0x60

I think that maybe it's good to put a:
       WARN_ON(!mm);
but a BUG_ON or without this patch, the kernel will halt, even if the 
problem is not so severe.
André.

Re: A patch for the file kernel/fork.c

[Alexander Nyberg]

> >>In a preemptible kernel with the serport module and a serial port try to
> >>run the following program:
> >>
> >>int main(int argc, char **argv)
> >>{
> >>       int ldisc,fd;
> >>
> >>       fd = open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NONBLOCK);
> >>       ldisc = N_MOUSE;
> >>       ioctl(fd, TIOCSETD, &ldisc);
> >>       read(fd, NULL, 0);
> >>       return 0;
> >>}
> >>
> >>and kill it.  In my case it will hang the computer. I think this is
> >>a problem with the serport module. With this patch, the serial mouse
> >>stop working, but the computer don't hang.
> >>    
> >>
> >
> >then above something like:
> >
> >     BUG_ON(!mm);
> >
> >or something might be better and eyeball the stack trace.
> >  
> >
> In my example, a stack trace will be already generated by an oops:
> [4300748.423000] Unable to handle kernel paging request at virtual 
> address 6b6b6b7b
> [4300748.423000]  printing eip:
> [4300748.423000] d0927366
> [4300748.423000] *pde = 00000000
> [4300748.423000] Oops: 0000 [#1]
> [4300748.423000] PREEMPT
> and so on, with a call trace:
> [4300748.423000] Call Trace:
> [4300748.423000]  [<c0104bfa>] show_stack+0x7a/0x90
> [4300748.423000]  [<c0104d7d>] show_registers+0x14d/0x1b0
> [4300748.423000]  [<c0104fcc>] die+0x14c/0x2c0
> [4300748.423000]  [<c0118b6f>] do_page_fault+0x31f/0x638
> [4300748.423000]  [<c01046df>] error_code+0x4f/0x54
> [4300748.423000]  [<c02b88fd>] tty_wakeup+0x5d/0x60
> 
> I think that maybe it's good to put a:
>        WARN_ON(!mm);
> but a BUG_ON or without this patch, the kernel will halt, even if the 
> problem is not so severe.

Patching up the kernel hiding things that must not happen is not the way
to go. All kernel bugs are severe (as you just showed us!). Adding extra
checks like your original patch did may even cause much more harm
because it may hide other problems causing silent problems.

The kernel should scream as loudly as possible when something goes
wrong. Please get the underlying problem fixed instead.

Re: A patch for the file kernel/fork.c

[Andrew Morton]

Alexander Nyberg <alexn@dsv.su.se> wrote:
>
> > [4300748.423000] Call Trace:
> > [4300748.423000]  [<c0104bfa>] show_stack+0x7a/0x90
> > [4300748.423000]  [<c0104d7d>] show_registers+0x14d/0x1b0
> > [4300748.423000]  [<c0104fcc>] die+0x14c/0x2c0
> > [4300748.423000]  [<c0118b6f>] do_page_fault+0x31f/0x638
> > [4300748.423000]  [<c01046df>] error_code+0x4f/0x54
> > [4300748.423000]  [<c02b88fd>] tty_wakeup+0x5d/0x60
> > 
> > I think that maybe it's good to put a:
> >        WARN_ON(!mm);
> > but a BUG_ON or without this patch, the kernel will halt, even if the 
> > problem is not so severe.
> 
> Patching up the kernel hiding things that must not happen is not the way
> to go. All kernel bugs are severe (as you just showed us!). Adding extra
> checks like your original patch did may even cause much more harm
> because it may hide other problems causing silent problems.

If I understand Andre correctly, his patch will prevent infinite recursion
in the oops path - if some process oopses after having run exit_mm().

If so then it's a reasonable debugging aid.  Although there might be better
places to do it, such as

	if (!current->i_tried_to_exit++)
		return;

in do_exit().   Dunno.

Re: A patch for the file kernel/fork.c

[André Pereira de Almeida]

Andrew Morton wrote:

>Alexander Nyberg <alexn@dsv.su.se> wrote:
>  
>
>>>[4300748.423000] Call Trace:
>>>[4300748.423000]  [<c0104bfa>] show_stack+0x7a/0x90
>>>[4300748.423000]  [<c0104d7d>] show_registers+0x14d/0x1b0
>>>[4300748.423000]  [<c0104fcc>] die+0x14c/0x2c0
>>>[4300748.423000]  [<c0118b6f>] do_page_fault+0x31f/0x638
>>>[4300748.423000]  [<c01046df>] error_code+0x4f/0x54
>>>[4300748.423000]  [<c02b88fd>] tty_wakeup+0x5d/0x60
>>>
>>>I think that maybe it's good to put a:
>>>       WARN_ON(!mm);
>>>but a BUG_ON or without this patch, the kernel will halt, even if the 
>>>problem is not so severe.
>>>      
>>>
>>Patching up the kernel hiding things that must not happen is not the way
>>to go. All kernel bugs are severe (as you just showed us!). Adding extra
>>checks like your original patch did may even cause much more harm
>>because it may hide other problems causing silent problems.
>>    
>>
>
>If I understand Andre correctly, his patch will prevent infinite recursion
>in the oops path - if some process oopses after having run exit_mm().
>
>If so then it's a reasonable debugging aid.  Although there might be better
>places to do it, such as
>
>	if (!current->i_tried_to_exit++)
>		return;
>
>in do_exit().   Dunno.
>
>  
>
I agree. If you need this patch, the you are in already in trouble, your 
kernel had already oopsed, your system is halted and you can't even send 
a bug report because you can't read read the call trace (it's an 
infinite recursion) and you can see only the end of it. It's not 
intended to hide the problem, the problem will be already in your screen.
André.

Re: A patch for the file kernel/fork.c

[Alexander Nyberg]

> > > I think that maybe it's good to put a:
> > >        WARN_ON(!mm);
> > > but a BUG_ON or without this patch, the kernel will halt, even if the 
> > > problem is not so severe.
> > 
> > Patching up the kernel hiding things that must not happen is not the way
> > to go. All kernel bugs are severe (as you just showed us!). Adding extra
> > checks like your original patch did may even cause much more harm
> > because it may hide other problems causing silent problems.
> 
> If I understand Andre correctly, his patch will prevent infinite recursion
> in the oops path - if some process oopses after having run exit_mm().
> 
> If so then it's a reasonable debugging aid.  Although there might be better
> places to do it, such as
> 
> 	if (!current->i_tried_to_exit++)
> 		return;
> 
> in do_exit().   Dunno.

This patch is very crude but it is quite resistant to recursive faults
in do_exit(), survives the LTP hammering I've given it. The problem is
not knowing where in the previous path it broke down so I'd rather just
leave it lying around and try a graceful reset/power off. But if anyone
has a better suggestion than the msleep() I'm all ears but this area is
sensitive.

Where is that anonymous patch hot-line...


Index: mm/kernel/exit.c
===================================================================
--- mm.orig/kernel/exit.c	2005-05-04 22:24:57.000000000 +0200
+++ mm/kernel/exit.c	2005-05-04 23:19:08.000000000 +0200
@@ -29,6 +29,7 @@
 #include <linux/perfctr.h>
 #include <linux/syscalls.h>
 #include <linux/signal.h>
+#include <linux/delay.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -797,6 +798,14 @@
 		ptrace_notify((PTRACE_EVENT_EXIT << 8) | SIGTRAP);
 	}
 
+	/* We're taking recursive faults originating here in do_exit. Safest 
+	 * is to just leave this task alone and wait for reboot. */
+	if (tsk->flags & PF_EXITING) {
+		printk(KERN_ALERT "\nFixing recursive fault but reboot is needed!\n");
+		for (;;)
+			msleep(1000 * 10);
+	}
+
 	tsk->flags |= PF_EXITING;
 
 	/*

Re: A patch for the file kernel/fork.c

[Steven Rostedt]

On Wed, 2005-05-04 at 23:21 +0200, Alexander Nyberg wrote:

> This patch is very crude but it is quite resistant to recursive faults
> in do_exit(), survives the LTP hammering I've given it. The problem is
> not knowing where in the previous path it broke down so I'd rather just
> leave it lying around and try a graceful reset/power off. But if anyone
> has a better suggestion than the msleep() I'm all ears but this area is
> sensitive.
> 
> Where is that anonymous patch hot-line...
> 

> +	/* We're taking recursive faults originating here in do_exit. Safest 
> +	 * is to just leave this task alone and wait for reboot. */
> +	if (tsk->flags & PF_EXITING) {
> +		printk(KERN_ALERT "\nFixing recursive fault but reboot is needed!\n");
> +		for (;;)
> +			msleep(1000 * 10);
> +	}
> +
>  	tsk->flags |= PF_EXITING;
>  

Instead of the for(;;) msleep, why not just take it permanently off the
run queue? With the following:

   set_current_state(TASK_UNINTERRUPTIBLE);
   schedule();

It basically gives the same effect, but is cleaner.

-- Steve

Re: A patch for the file kernel/fork.c

[Alexander Nyberg]

> Instead of the for(;;) msleep, why not just take it permanently off the
> run queue? With the following:
> 
>    set_current_state(TASK_UNINTERRUPTIBLE);
>    schedule();
> 
> It basically gives the same effect, but is cleaner.

Ah perfect, now it looks ok. Thanks!

Prevent recursive faults in do_exit() by leaving the task alone and wait
for reboot. This may allow a more graceful shutdown and possibly save
the original oops.


Signed-off-by: Alexander Nyberg <alexn@telia.com>

Index: mm/kernel/exit.c
===================================================================
--- mm.orig/kernel/exit.c	2005-05-05 16:44:20.000000000 +0200
+++ mm/kernel/exit.c	2005-05-05 17:29:40.000000000 +0200
@@ -797,6 +797,14 @@
 		ptrace_notify((PTRACE_EVENT_EXIT << 8) | SIGTRAP);
 	}
 
+	/* We're taking recursive faults here in do_exit. Safest 
+	 * is to just leave this task alone and wait for reboot. */
+	if (unlikely(tsk->flags & PF_EXITING)) {
+		printk(KERN_ALERT "\nFixing recursive fault but reboot is needed!\n");
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule();
+	}
+
 	tsk->flags |= PF_EXITING;
 
 	/*

Re: A patch for the file kernel/fork.c

[Andrew Morton]

Alexander Nyberg <alexn@dsv.su.se> wrote:
>
> +	/* We're taking recursive faults here in do_exit. Safest 
>  +	 * is to just leave this task alone and wait for reboot. */

I find this comment-block style a bit hard to maintain, and am anal about
consistency.

>  +	if (unlikely(tsk->flags & PF_EXITING)) {
>  +		printk(KERN_ALERT "\nFixing recursive fault but reboot is needed!\n");
>  +		set_current_state(TASK_UNINTERRUPTIBLE);
>  +		schedule();
>  +	}
>  +

In the printk string, a \n will terminate the current facility level, so
your KERN_ALERT there is a no-op.  I simply removed it, which might cause
messy output sometimes but that seems better than always adding a newline.


--- 25/kernel/exit.c~avoid-recursive-oopses	2005-05-06 18:03:45.000000000 -0700
+++ 25-akpm/kernel/exit.c	2005-05-06 18:06:01.000000000 -0700
@@ -795,6 +795,17 @@ fastcall NORET_TYPE void do_exit(long co
 		ptrace_notify((PTRACE_EVENT_EXIT << 8) | SIGTRAP);
 	}
 
+	/*
+	 * We're taking recursive faults here in do_exit. Safest is to just
+	 * leave this task alone and wait for reboot.
+	 */
+	if (unlikely(tsk->flags & PF_EXITING)) {
+		printk(KERN_ALERT
+			"Fixing recursive fault but reboot is needed!\n");
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule();
+	}
+
 	tsk->flags |= PF_EXITING;
 
 	/*
_

Re: A patch for the file kernel/fork.c

[Valdis.Kletnieks]

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

On Wed, 04 May 2005 15:26:44 -0300, =?ISO-8859-1?Q?Andr=E9_Pereira_de_Almeida?= said:

> In a preemptible kernel with the serport module and a serial port try to 
> run the following program:

> and kill it.
> In my case it will hang the computer. I think this is a problem with the 
> serport module. With this patch, the serial mouse stop working, but the 
> computer don't hang.

The fact that the mouse stops working is indicative that this patch doesn't
actually fix the problem, it's just pushing it around in the kernel - sooner
or later something *else* is going to go pear-shaped on the null *mm.  The right
fix is to figure out why mm is bogus and fix that issue.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: A patch for the file kernel/fork.c

[Dmitry Torokhov]

On 5/4/05, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> On Wed, 04 May 2005 15:26:44 -0300, =?ISO-8859-1?Q?Andr=E9_Pereira_de_Almeida?= said:
> 
> > In a preemptible kernel with the serport module and a serial port try to
> > run the following program:
> 
> > and kill it.
> > In my case it will hang the computer. I think this is a problem with the
> > serport module. With this patch, the serial mouse stop working, but the
> > computer don't hang.
> 
> The fact that the mouse stops working is indicative that this patch doesn't
> actually fix the problem, it's just pushing it around in the kernel - sooner
> or later something *else* is going to go pear-shaped on the null *mm.  The right
> fix is to figure out why mm is bogus and fix that issue.
> 

serport module is busted at the moment. Look here for the patch:

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.12-rc3/2.6.12-rc3-mm2/broken-out/serport-oops-fix.patch

It should probably go into -stable but Vojtech seems to have
disappeared... I think Fedora already picked it up.

-- 
Dmitry

Re: A patch for the file kernel/fork.c

[André Pereira de Almeida]

Valdis.Kletnieks@vt.edu wrote:

>On Wed, 04 May 2005 15:26:44 -0300, =?ISO-8859-1?Q?Andr=E9_Pereira_de_Almeida?= said:
>
>  
>
>>In a preemptible kernel with the serport module and a serial port try to 
>>run the following program:
>>    
>>
>
>  
>
>>and kill it.
>>In my case it will hang the computer. I think this is a problem with the 
>>serport module. With this patch, the serial mouse stop working, but the 
>>computer don't hang.
>>    
>>
>
>The fact that the mouse stops working is indicative that this patch doesn't
>actually fix the problem, it's just pushing it around in the kernel - sooner
>or later something *else* is going to go pear-shaped on the null *mm.  The right
>fix is to figure out why mm is bogus and fix that issue.
>  
>
This patch is not to fix this problem, but to minimize a bug in other 
modules (in this case I think it's in the serport module) and to keep 
the system working, at least to make a clean shutdown or terminate some 
important process cleanly.

This patch is in the mm_release that should return a null *mm anyway.

The bogus mm came from a pagefault in the do_exit function in 
kernel/exit.c that occured after the exit_mm call. This pagefault, in my 
case, come from a filp_close call that called tty_wakeup. I'm trying to 
identify this problem, but a pagefault like this may came from several 
different modules.
André