[PATCH] Still a mm bug in the fork error path

[PATCH] Still a mm bug in the fork error path

[John Byrne]

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


About a month ago, the thread "[no patch] broken use of mm_release / 
activate_mm" ("http://marc.theaimsgroup.com/?t=109510277700003&r=1&w=2") 
ended up with the following patch being applied to fork.c:

@@ -1104,9 +1146,7 @@
  bad_fork_cleanup_namespace:
  	exit_namespace(p);
  bad_fork_cleanup_mm:
-	exit_mm(p);
-	if (p->active_mm)
-		mmdrop(p->active_mm);
+	mmput(p->mm);
  bad_fork_cleanup_signal:
  	exit_signal(p);
  bad_fork_cleanup_sighand:

However, the new code will panic if the thread being forked is a process 
with a NULL mm. It looks very unlikely to be hit in the real world, but 
it is possible. (My modified kernel makes it much more likely which is 
how I found it.) The attached patch is against 2.6.9-rc4. This time for 
sure!

John Byrne







[-- Attachment #2: fork.patch --]
[-- Type: text/x-patch, Size: 423 bytes --]

diff -Nar -U4 linux-2.6.9-rc4/kernel/fork.c new/kernel/fork.c
--- linux-2.6.9-rc4/kernel/fork.c	2004-10-12 16:18:03.661895647 -0700
+++ new/kernel/fork.c	2004-10-12 15:59:14.287779998 -0700
@@ -1145,9 +1145,10 @@
 
 bad_fork_cleanup_namespace:
 	exit_namespace(p);
 bad_fork_cleanup_mm:
-	mmput(p->mm);
+	if (p->mm)
+		mmput(p->mm);
 bad_fork_cleanup_signal:
 	exit_signal(p);
 bad_fork_cleanup_sighand:
 	exit_sighand(p);

Re: [PATCH] Still a mm bug in the fork error path

[Linus Torvalds]

On Tue, 12 Oct 2004, John Byrne wrote:
> 
> @@ -1104,9 +1146,7 @@
>   bad_fork_cleanup_namespace:
>   	exit_namespace(p);
>   bad_fork_cleanup_mm:
> -	exit_mm(p);
> -	if (p->active_mm)
> -		mmdrop(p->active_mm);
> +	mmput(p->mm);
>   bad_fork_cleanup_signal:
>   	exit_signal(p);
>   bad_fork_cleanup_sighand:
> 
> However, the new code will panic if the thread being forked is a process 
> with a NULL mm. It looks very unlikely to be hit in the real world, but 
> it is possible.

Hmm.. How does it happen? As far as I can tell, we only get here if
 - copy_thread or copy_namespaces had an error
and "mm" can be NULL only for kernel threads.

Now, I don't think any kernel threads will ask for new namespaces, so 
copy_namespaces can't return an error. Similarly, I don't see how 
copy_thread() could either (at least on x86 it can only return an error if 
an IO bitmap allocation fails, I think - again something that shouldn't 
happen for kernel threads. And most other architectures will never fail 
at all, I do believe).

> (My modified kernel makes it much more likely which is how I found it.)
> The attached patch is against 2.6.9-rc4. This time for sure!

I don't mind the patch per se, but I'd rather put it in after 2.6.9 unless
you can tell me how this can actually happen with an unmodified kernel.

			Linus

Re: [PATCH] Still a mm bug in the fork error path

[John Byrne]

Linus Torvalds wrote:
> 
> On Tue, 12 Oct 2004, John Byrne wrote:
> 
>>@@ -1104,9 +1146,7 @@
>>  bad_fork_cleanup_namespace:
>>  	exit_namespace(p);
>>  bad_fork_cleanup_mm:
>>-	exit_mm(p);
>>-	if (p->active_mm)
>>-		mmdrop(p->active_mm);
>>+	mmput(p->mm);
>>  bad_fork_cleanup_signal:
>>  	exit_signal(p);
>>  bad_fork_cleanup_sighand:
>>
>>However, the new code will panic if the thread being forked is a process 
>>with a NULL mm. It looks very unlikely to be hit in the real world, but 
>>it is possible.
> 
> 
> Hmm.. How does it happen? As far as I can tell, we only get here if
>  - copy_thread or copy_namespaces had an error
> and "mm" can be NULL only for kernel threads.
> 
> Now, I don't think any kernel threads will ask for new namespaces, so 
> copy_namespaces can't return an error. Similarly, I don't see how 
> copy_thread() could either (at least on x86 it can only return an error if 
> an IO bitmap allocation fails, I think - again something that shouldn't 
> happen for kernel threads. And most other architectures will never fail 
> at all, I do believe).
> 
> 
>>(My modified kernel makes it much more likely which is how I found it.)
>>The attached patch is against 2.6.9-rc4. This time for sure!
> 
> 
> I don't mind the patch per se, but I'd rather put it in after 2.6.9 unless
> you can tell me how this can actually happen with an unmodified kernel.
> 
> 			Linus
> 

In my kernel, it was a SIGKILL to a forking kernel thread that caused 
the problem. While I see SIGKILLs being sent to some kernel threads, I 
don't know if any of the kernel threads ever fork. If they don't, 
barring a demented root user sending SIGKILLs to kernel threads, I don't 
know if anyone else will ever see this. So, I don't have any problems 
with it being fixed post 2.6.9.

Thanks,

John Byrne

Re: [PATCH] Still a mm bug in the fork error path

[Linus Torvalds]

On Tue, 12 Oct 2004, John Byrne wrote:
>
> In my kernel, it was a SIGKILL to a forking kernel thread that caused 
> the problem. While I see SIGKILLs being sent to some kernel threads, I 
> don't know if any of the kernel threads ever fork. If they don't, 
> barring a demented root user sending SIGKILLs to kernel threads, I don't 
> know if anyone else will ever see this. So, I don't have any problems 
> with it being fixed post 2.6.9.

Heh. Andrew picked up the patch anyway, so in it goes.

		Linus

Re: [PATCH] Still a mm bug in the fork error path

[Pavel Machek]

Hi!

> In my kernel, it was a SIGKILL to a forking kernel thread that caused 
> the problem. While I see SIGKILLs being sent to some kernel threads, I 
> don't know if any of the kernel threads ever fork. If they don't, 
> barring a demented root user sending SIGKILLs to kernel threads, I don't 
> know if anyone else will ever see this. So, I don't have any problems 
> with it being fixed post 2.6.9.

Actually I do not think that root sending SIGKILLs to kernel threads
is demented... I tried to recover broken machine by SIGKILLing
kacpid. It did not work, so I just reniced it to get machine back.

								Pavel
PS: BTW it was evo n620c. There's something wrong with thermal
handling after swsusp. Evo will not die by overheat, but it almost
damaged machine it was standing on... ouch. Cover got deformed by
heat (quite a bit), fortunately it returned to previous shape when it
cooled down. 

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!