Re: futex question

From: Thomas Gleixner
Date: Mon Oct 05 2009 - 08:01:26 EST


On Mon, 5 Oct 2009, Peter Zijlstra wrote:

> On Mon, 2009-10-05 at 12:36 +0200, Peter Zijlstra wrote:
> > On Sun, 2009-10-04 at 18:59 +0200, Thomas Gleixner wrote:
> >
> > > > do. It does not feel right. Currently, with or without my change,
> > > > such a thing would indefinitely block other waiters on the same
> > > > futex.
> > >
> > > Right. Which completely defeats the purpose of the robust list. Will
> > > have a look tomorrow.
> >
> > Right, so mm_release() which is meant to destroy the old mm context
> > actually does exit_robust_list(), but the problem is that it does so on
> > the new mm, not the old one that got passed down to mm_release().
> >
> > The other detail is that exit_robust_list() doesn't clear
> > current->robust_list.
> >
> > The problem with the patch send my Ani is that it clears the robust
> > lists before the point of no return, so on a failing execve() we'd have
> > messed up the state.
> >
> > Making exit_robust_list() deal with an mm that is not the current mm is
> > interesting indeed.
>
> Hmm...
>
> static int exec_mmap(struct mm_struct *mm)
> {
> struct task_struct *tsk;
> struct mm_struct * old_mm, *active_mm;
>
> /* Notify parent that we're no longer interested in the old VM */
> tsk = current;
> old_mm = current->mm;
> mm_release(tsk, old_mm);
>
> if (old_mm) {
> /*
> * Make sure that if there is a core dump in progress
> * for the old mm, we get out and die instead of going
> * through with the exec. We must hold mmap_sem around
> * checking core_state and changing tsk->mm.
> */
> down_read(&old_mm->mmap_sem);
> if (unlikely(old_mm->core_state)) {
> up_read(&old_mm->mmap_sem);
> return -EINTR;
> }
> }
> task_lock(tsk);
> active_mm = tsk->active_mm;
> tsk->mm = mm;
> tsk->active_mm = mm;
> activate_mm(active_mm, mm);
> task_unlock(tsk);
> arch_pick_mmap_layout(mm);
> if (old_mm) {
> up_read(&old_mm->mmap_sem);
> BUG_ON(active_mm != old_mm);
> mm_update_next_owner(old_mm);
> mmput(old_mm);
> return 0;
> }
> mmdrop(active_mm);
> return 0;
> }
>
> Actually calls mm_release() before the flip, so the below might actually
> be sufficient?

Stared at the same place a minute ago :) But still I wonder if it's a
good idea to silently release locks and set the state to OWNERDEAD
instead of hitting the app programmer with a big clue stick in case
the app holds locks when calling execve().

Thanks,

tglx

> ---
> kernel/fork.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 266c6af..4c20fff 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -570,12 +570,18 @@ void mm_release(struct task_struct *tsk, struct mm_struct *mm)
>
> /* Get rid of any futexes when releasing the mm */
> #ifdef CONFIG_FUTEX
> - if (unlikely(tsk->robust_list))
> + if (unlikely(tsk->robust_list)) {
> exit_robust_list(tsk);
> + tsk->robust_list = NULL;
> + }
> #ifdef CONFIG_COMPAT
> - if (unlikely(tsk->compat_robust_list))
> + if (unlikely(tsk->compat_robust_list)) {
> compat_exit_robust_list(tsk);
> + tsk->compat_robust_list = NULL;
> + }
> #endif
> + if (unlikely(!list_empty(&tsk->pi_state_list)))
> + exit_pi_state_list(tsk);
> #endif
>
> /* Get rid of any cached register state */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/