bug in fork failure path?

bug in fork failure path?

[Colin Gibbs]

Hi, 

I came across this bug hacking on sparc32. If fork fails in copy_mm ->
dup_mmap, destroy_context is called on the new mm before
init_new_context. 

fork.c:330 
	retval = -ENOMEM; 
	mm = allocate_mm(); 
	if (!mm) 
		goto fail_nomem; 

	/* Copy the current MM stuff.. */ 
	memcpy(mm, oldmm, sizeof(*mm)); 
	if (!mm_init(mm)) 
		goto fail_nomem; 

Failure is ok here. We don't try to do an mmput, but we have memcpy'd
the mm struct context and all. 

	down_write(&oldmm->mmap_sem); 
	retval = dup_mmap(mm); 
	up_write(&oldmm->mmap_sem); 

	if (retval) 
		goto free_pt; 

If we fail and call mmput, destroy_context gets called before
init_new_context below. This removes the parent process's context since
it was just memcpy'd from the parent's mm struct. 

	/* 
	 * child gets a private LDT (if there was an LDT in the parent) 
	 */ 
	copy_segments(tsk, mm); 

	if (init_new_context(tsk,mm)) 
		goto free_pt; 

Can we move the init_new_context to just after the mm_init call? Works
nicely on sparc. Most archs have a fairly trivial init_new_context
anyway. 

Colin 

--- 2.4.19-pre4/kernel/fork.c	Thu Mar 28 19:49:36 2002
+++ tortoise-19-pre4/kernel/fork.c	Wed Apr 17 23:26:20 2002
@@ -336,6 +336,9 @@
 	if (!mm_init(mm))
 		goto fail_nomem;
 
+	if (init_new_context(tsk,mm))
+		goto free_pt;
+
 	down_write(&oldmm->mmap_sem);
 	retval = dup_mmap(mm);
 	up_write(&oldmm->mmap_sem);
@@ -347,9 +350,6 @@
 	 * child gets a private LDT (if there was an LDT in the parent)
 	 */
 	copy_segments(tsk, mm);
-
-	if (init_new_context(tsk,mm))
-		goto free_pt;
 
 good_mm:
 	tsk->mm = mm;

Re: bug in fork failure path?

[Kanoj Sarcar]

--- Colin Gibbs <colin@gibbs.dhs.org> wrote:
> Hi, 
> 
> I came across this bug hacking on sparc32. If fork
> fails in copy_mm ->
> dup_mmap, destroy_context is called on the new mm
> before
> init_new_context. 

Yes, this definitely sounds like a bug. I added the
init_new_context() call for mips64 in 2.3. With the
way the current code is, I belive even mips64 has
a problem.

Kanoj

> 
> fork.c:330 
> 	retval = -ENOMEM; 
> 	mm = allocate_mm(); 
> 	if (!mm) 
> 		goto fail_nomem; 
> 
> 	/* Copy the current MM stuff.. */ 
> 	memcpy(mm, oldmm, sizeof(*mm)); 
> 	if (!mm_init(mm)) 
> 		goto fail_nomem; 
> 
> Failure is ok here. We don't try to do an mmput, but
> we have memcpy'd
> the mm struct context and all. 
> 
> 	down_write(&oldmm->mmap_sem); 
> 	retval = dup_mmap(mm); 
> 	up_write(&oldmm->mmap_sem); 
> 
> 	if (retval) 
> 		goto free_pt; 
> 
> If we fail and call mmput, destroy_context gets
> called before
> init_new_context below. This removes the parent
> process's context since
> it was just memcpy'd from the parent's mm struct. 
> 
> 	/* 
> 	 * child gets a private LDT (if there was an LDT in
> the parent) 
> 	 */ 
> 	copy_segments(tsk, mm); 
> 
> 	if (init_new_context(tsk,mm)) 
> 		goto free_pt; 
> 
> Can we move the init_new_context to just after the
> mm_init call? Works
> nicely on sparc. Most archs have a fairly trivial
> init_new_context
> anyway. 
> 
> Colin 
> 
> --- 2.4.19-pre4/kernel/fork.c	Thu Mar 28 19:49:36
> 2002
> +++ tortoise-19-pre4/kernel/fork.c	Wed Apr 17
> 23:26:20 2002
> @@ -336,6 +336,9 @@
>  	if (!mm_init(mm))
>  		goto fail_nomem;
>  
> +	if (init_new_context(tsk,mm))
> +		goto free_pt;
> +
>  	down_write(&oldmm->mmap_sem);
>  	retval = dup_mmap(mm);
>  	up_write(&oldmm->mmap_sem);
> @@ -347,9 +350,6 @@
>  	 * child gets a private LDT (if there was an LDT
> in the parent)
>  	 */
>  	copy_segments(tsk, mm);
> -
> -	if (init_new_context(tsk,mm))
> -		goto free_pt;
>  
>  good_mm:
>  	tsk->mm = mm;
> 
> -
> To unsubscribe from this list: send the line
> "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at 
http://vger.kernel.org/majordomo-info.html


__________________________________________________
Do You Yahoo!?
Yahoo! Tax Center - online filing with TurboTax
http://taxes.yahoo.com/

Re: bug in fork failure path?

[David S. Miller]

   From: Colin Gibbs <colin@gibbs.dhs.org>
   Date: 18 Apr 2002 20:10:22 -0500
   
   Can we move the init_new_context to just after the mm_init call? Works
   nicely on sparc. Most archs have a fairly trivial init_new_context
   anyway. 

I've put this fix into my trees to make sure it doesn't get
lost.  Thanks for spotting it.