lkml.org 
[lkml]   [2009]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: futex question
    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 */
    >


    \
     
     \ /
      Last update: 2009-10-05 14:11    [W:0.028 / U:0.316 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site