Re: futex question

[Thomas Gleixner <tglx@linutronix.de>]

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 */
>