Messages in this thread Patch in this message | | | Date | Wed, 21 Jun 2006 13:41:29 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: Possible bug in do_execve() |
| |
Quoting Sonny Rao (sonny@burdell.org): > While doing some stress testing with a reduced number of MMU contexts, > I found that an error path in exec seemed to call destroy_context() > via mmdrop() immediately after init_new_context() failed. > > specifically I got some warning from the idr code through powerpc > mmu_context code: > > idr_remove called for id=0 which is not allocated. > Call Trace: > [C0000003C9E73820] [C00000000000E760] .show_stack+0x74/0x1b4 (unreliable) > [C0000003C9E738D0] [C000000000212F30] .idr_remove+0x1c4/0x274 > [C0000003C9E73990] [C00000000002CA14] .destroy_context+0x2c/0x60 > [C0000003C9E73A20] [C00000000004CDAC] .__mmdrop+0x50/0x80 > [C0000003C9E73AB0] [C0000000000C9E38] .do_execve+0x218/0x290 > [C0000003C9E73B60] [C00000000000F28C] .sys_execve+0x74/0xf8 > [C0000003C9E73C00] [C00000000000871C] syscall_exit+0x0/0x40 > --- Exception: c01 at .execve+0x8/0x14 > LR = .____call_usermodehelper+0xdc/0xf4 > [C0000003C9E73EF0] [C000000000065388] .____call_usermodehelper+0xb0/0xf4 (unreliable) > [C0000003C9E73F90] [C000000000023928] .kernel_thread+0x4c/0x68 > > > Here's the code in do_execve(): > > retval = init_new_context(current, bprm->mm); > if (retval < 0) > goto out_mm > > <snip> > > out_mm: > if (bprm->mm) > mmdrop(bprm->mm); > > mmdrop() then calls destroy_context(). > There's a similar path in compat_do_execve(). > > > Anton pointed out a comment in fork.c, which seems to inidcate > incorrect behavior in the exec code. > > >From dup_mm() in fork.c: > > if (init_new_context(tsk, mm)) > goto fail_nocontext; > > <snip> > > fail_nocontext: > /* > * If init_new_context() failed, we cannot use mmput() to free the mm > * because it calls destroy_context() > */ > mm_free_pgd(mm); > free_mm(mm); > return NULL; > > > > Is the behavior in do_execve() correct?
Well, I assume the intent is for out_mm: to clean up from mm_alloc(), not from 'init_new_context'. So I think that code is correct. This bug appears to be powerpc-specific, so would the following patch be reasonable?
Note it is entirely untested, just to show where i think this should be solved. But I could try compile+boot test tonight.
thanks, -serge
From: Serge E. Hallyn <hallyn@sergelap.(none)> Date: Wed, 21 Jun 2006 13:37:27 -0500 Subject: [PATCH] powerpc: check for proper mm->context before destroying
arch/powerpc/mm/mmu_context_64.c:destroy_context() can be called from __mmput() in do_execve() if init_new_context() failed. This can result in idr_remove() being called for an invalid context.
So, don't call idr_remove if there is no context.
Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
arch/powerpc/mm/mmu_context_64.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
ee74da9d3c122b92541dd6b7670731bd4a033f04 diff --git a/arch/powerpc/mm/mmu_context_64.c b/arch/powerpc/mm/mmu_context_64.c index 714a84d..552d590 100644 --- a/arch/powerpc/mm/mmu_context_64.c +++ b/arch/powerpc/mm/mmu_context_64.c @@ -55,6 +55,9 @@ again: void destroy_context(struct mm_struct *mm) { + if (mm->context.id == NO_CONTEXT) + return; + spin_lock(&mmu_context_lock); idr_remove(&mmu_context_idr, mm->context.id); spin_unlock(&mmu_context_lock); -- 1.3.3 - 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/
| |