Possible bug in do_execve()

Possible bug in do_execve()

[Sonny Rao]

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Serge E. Hallyn]

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Sonny Rao]

On Wed, Jun 21, 2006 at 01:41:29PM -0500, Serge E. Hallyn wrote:
<snip>
> > 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);

Yeah, I proposed a similar patch to Anton, and it would quiet the
warning on powerpc, but that's not the point.  It happens that powerpc
doesn't use 0 as a context id, but that may not be true on another
architecture.  That's really what I'm concerned about.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Serge E. Hallyn]

Quoting Sonny Rao (sonny@burdell.org):
> On Wed, Jun 21, 2006 at 01:41:29PM -0500, Serge E. Hallyn wrote:
> <snip>
> > > 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);
> 
> Yeah, I proposed a similar patch to Anton, and it would quiet the
> warning on powerpc, but that's not the point.  It happens that powerpc
> doesn't use 0 as a context id, but that may not be true on another
> architecture.  That's really what I'm concerned about.

FWIW, ppc and cris do the NO_CONTEXT check, while others don't
even have a arch-specific 'mm->context.id'.

-serge

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Sonny Rao]

On Wed, Jun 21, 2006 at 02:09:10PM -0500, Serge E. Hallyn wrote:
<snip>
> > Yeah, I proposed a similar patch to Anton, and it would quiet the
> > warning on powerpc, but that's not the point.  It happens that powerpc
> > doesn't use 0 as a context id, but that may not be true on another
> > architecture.  That's really what I'm concerned about.
> 
> FWIW, ppc and cris do the NO_CONTEXT check, while others don't
> even have a arch-specific 'mm->context.id'.

Good point.  I probably stated that concern too narrowly.  Probably
what I should say is: What is the pre-condition for calling
destroy_context() ?  Is it that init_new_context() must have
succeeded?  Or is it merely that mm.context has been zeroed
out?

Here's destroy context on sparc64:

void destroy_context(struct mm_struct *mm)
{
        unsigned long flags, i;

        for (i = 0; i < MM_NUM_TSBS; i++)
                tsb_destroy_one(&mm->context.tsb_block[i]);

        spin_lock_irqsave(&ctx_alloc_lock, flags);

        if (CTX_VALID(mm->context)) {
                unsigned long nr = CTX_NRBITS(mm->context);
                mmu_context_bmap[nr>>6] &= ~(1UL << (nr & 63));
        }

        spin_unlock_irqrestore(&ctx_alloc_lock, flags);
}

It seems to assume that mm->context is valid before doing a check.

Since I don't have a sparc64 box, I can't check to see if this
actually breaks things or not.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Serge E. Hallyn]

Quoting Sonny Rao (sonny@burdell.org):
> On Wed, Jun 21, 2006 at 02:09:10PM -0500, Serge E. Hallyn wrote:
> <snip>
> > > Yeah, I proposed a similar patch to Anton, and it would quiet the
> > > warning on powerpc, but that's not the point.  It happens that powerpc
> > > doesn't use 0 as a context id, but that may not be true on another
> > > architecture.  That's really what I'm concerned about.
> > 
> > FWIW, ppc and cris do the NO_CONTEXT check, while others don't
> > even have a arch-specific 'mm->context.id'.
> 
> Good point.  I probably stated that concern too narrowly.  Probably
> what I should say is: What is the pre-condition for calling
> destroy_context() ?  Is it that init_new_context() must have
> succeeded?  Or is it merely that mm.context has been zeroed
> out?

Right, that may be the right question.  If that's the case, then the
problem is really include/linux/sched.h:__mmdrop() which is what's
calling destroy_context().  Separating that out becomes a pretty
big patch affecting at least all mmput() and mmdrop() callers.

> Here's destroy context on sparc64:
> 
> void destroy_context(struct mm_struct *mm)
> {
>         unsigned long flags, i;
> 
>         for (i = 0; i < MM_NUM_TSBS; i++)
>                 tsb_destroy_one(&mm->context.tsb_block[i]);
> 
>         spin_lock_irqsave(&ctx_alloc_lock, flags);
> 
>         if (CTX_VALID(mm->context)) {
>                 unsigned long nr = CTX_NRBITS(mm->context);
>                 mmu_context_bmap[nr>>6] &= ~(1UL << (nr & 63));
>         }
> 
>         spin_unlock_irqrestore(&ctx_alloc_lock, flags);
> }
> 
> It seems to assume that mm->context is valid before doing a check.
> 
> Since I don't have a sparc64 box, I can't check to see if this
> actually breaks things or not.

So we can either go through all arch's and make sure destroy_context is
safe for invalid context, or split mmput() and destroy_context()...

The former seems easier, but the latter seems more robust in the face of
future code changes I guess.

-serge

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Sonny Rao]

On Wed, Jun 21, 2006 at 02:42:50PM -0500, Serge E. Hallyn wrote:
> Quoting Sonny Rao (sonny@burdell.org):
> > On Wed, Jun 21, 2006 at 02:09:10PM -0500, Serge E. Hallyn wrote:
> > <snip>
> > > > Yeah, I proposed a similar patch to Anton, and it would quiet the
> > > > warning on powerpc, but that's not the point.  It happens that powerpc
> > > > doesn't use 0 as a context id, but that may not be true on another
> > > > architecture.  That's really what I'm concerned about.
> > > 
> > > FWIW, ppc and cris do the NO_CONTEXT check, while others don't
> > > even have a arch-specific 'mm->context.id'.
> > 
> > Good point.  I probably stated that concern too narrowly.  Probably
> > what I should say is: What is the pre-condition for calling
> > destroy_context() ?  Is it that init_new_context() must have
> > succeeded?  Or is it merely that mm.context has been zeroed
> > out?
> 
> Right, that may be the right question.  If that's the case, then the
> problem is really include/linux/sched.h:__mmdrop() which is what's
> calling destroy_context().  Separating that out becomes a pretty
> big patch affecting at least all mmput() and mmdrop() callers.

So mmdrop() inlines to an atomic_dec_and_test on mm_count and a call
to __mmdrop which makes three calls : mm_free_pgd(), destroy_context(),
and free_mm().  I _think_ that in this case __mmdrop() will always get
called.

We know that the destroy_context() is unnecessary, but mm_free_pgd()
and free_mm() are necessary.

I was thinking we _could_ open code these calls in exec.c but that seems
like a "Really Bad Idea" w.r.t abstraction/maintenance etc,
and the alternative is to make another function/macro just for this
special case, which also seems like a poor choice.

> > It seems to assume that mm->context is valid before doing a check.
> > 
> > Since I don't have a sparc64 box, I can't check to see if this
> > actually breaks things or not.
> 
> So we can either go through all arch's and make sure destroy_context is
> safe for invalid context, or split mmput() and destroy_context()...
> 
> The former seems easier, but the latter seems more robust in the face of
> future code changes I guess.

Yes, the former does seem easier, and perhaps easiest is to do that
and document what the pre-conditions are so future developers at least
have a clue.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Serge E. Hallyn]

Quoting Sonny Rao (sonny@burdell.org):
> > > It seems to assume that mm->context is valid before doing a check.
> > > 
> > > Since I don't have a sparc64 box, I can't check to see if this
> > > actually breaks things or not.
> > 
> > So we can either go through all arch's and make sure destroy_context is
> > safe for invalid context, or split mmput() and destroy_context()...
> > 
> > The former seems easier, but the latter seems more robust in the face of
> > future code changes I guess.
> 
> Yes, the former does seem easier, and perhaps easiest is to do that
> and document what the pre-conditions are so future developers at least
> have a clue.

Hmm, but document it where, since there is no single destroy_context()
definition?  At the mmput() and __mmdrop() definitions in kernel/fork.c?

(like so: ?)

-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 +++
 kernel/fork.c                    |    4 ++++
 2 files changed, 7 insertions(+), 0 deletions(-)

d4fdebfda2170615db87f5aaf45b8478e223824a
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);
diff --git a/kernel/fork.c b/kernel/fork.c
index ac8100e..0fe51aa 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -354,6 +354,10 @@ struct mm_struct * mm_alloc(void)
  * Called when the last reference to the mm
  * is dropped: either by a lazy thread or by
  * mmput. Free the page directory and the mm.
+ *
+ * Arch-specific destroy_context() implementations
+ * should be aware that this can be called when
+ * the mm->context initialization has failed.
  */
 void fastcall __mmdrop(struct mm_struct *mm)
 {
-- 
1.3.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible bug in do_execve()

[Sonny Rao]

On Thu, Jun 22, 2006 at 06:59:07AM -0500, Serge E. Hallyn wrote:
> Quoting Sonny Rao (sonny@burdell.org):
> > > > It seems to assume that mm->context is valid before doing a check.
> > > > 
> > > > Since I don't have a sparc64 box, I can't check to see if this
> > > > actually breaks things or not.
> > > 
> > > So we can either go through all arch's and make sure destroy_context is
> > > safe for invalid context, or split mmput() and destroy_context()...
> > > 
> > > The former seems easier, but the latter seems more robust in the face of
> > > future code changes I guess.
> > 
> > Yes, the former does seem easier, and perhaps easiest is to do that
> > and document what the pre-conditions are so future developers at least
> > have a clue.
> 
> Hmm, but document it where, since there is no single destroy_context()
> definition?  At the mmput() and __mmdrop() definitions in kernel/fork.c?
> 
That seems reasonable to me.  

I was hoping some of the arch maintainers might chime in with their
insight on the issue.  

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>