• john stultz's avatar
    [PATCH] futex_handle_fault always fails · e579dcbf
    john stultz authored
    We found this issue last week w/ the -RT kernel, but it seems the same
    issue is in mainline as well.
    
    Basically it is possible for futex_unlock_pi to return without actually
    freeing the lock.  This is due to buggy logic in the use of
    futex_handle_fault() and its attempt argument in a failure case.
    
    Looking at futex.c the logic is as follows:
    
    1) In futex_unlock_pi() we start w/ ret=0 and we go down to the first
       futex_atomic_cmpxchg_inatomic(), where we find uval==-EFAULT.  We then
       jump to the pi_faulted label.
    
    2) From pi_faulted: We increment attempt, unlock the sem and hit the
       retry label.
    
    3) From the retry label, with ret still zero, we again hit EFAULT on the
       first futex_atomic_cmpxchg_inatomic(), and again goto the pi_faulted
       label.
    
    4) Again from pi_faulted: we increment attempt and enter the
       conditional, where we call futex_handle_fault.
    
    5) futex_handle_fault fails, and we goto the out_unlock_release_sem
       label.
    
    6) From out_unlock_release_sem we return, and since ret is still zero,
       we return without error, while never actually unlocking the lock.
    
    Issue #1: at the first futex_atomic_cmpxchg_inatomic() we should probably
    be setting ret=-EFAULT before jumping to pi_faulted: However in our case
    this doesn't really affect anything, as the glibc we're using ignores the
    error value from futex_unlock_pi().
    
    Issue #2: Look at futex_handle_fault(), its first conditional will return
    -EFAULT if attempt is >= 2.  However, from the "if(attempt++)
    futex_handle_fault(attempt)" logic above, we'll *never* call
    futex_handle_fault when attempt is less then two.  So we never get a chance
    to even try to fault the page in.
    
    The following patch addresses these two issues by 1) Always setting ret to
    -EFAULT if futex_handle_fault fails, and 2) Removing the = in
    futex_handle_fault's (attempt >= 2) check.
    
    I'm really not sure this is the right fix, but wanted to bring it up so
    folks knew the issue is alive and well in the current -git tree.  From
    looking at the git logs the logic was first introduced (then later copied
    to other places) in the following commit almost a year ago:
    
    http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=4732efbeb997189d9f9b04708dc26bf8613ed721;hp=5b039e681b8c5f30aac9cc04385cc94be45d0823
    
    Cc: Rusty Russell <rusty@rustcorp.com.au>
    Cc: Ingo Molnar <mingo@elte.hu>
    Acked-by: default avatarThomas Gleixner <tglx@linutronix.de>
    Signed-off-by: default avatarAndrew Morton <akpm@osdl.org>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
    e579dcbf
futex.c 44.9 KB