Bug 20985 - robust mutexes: lowlevelrobustlock assembly on x86 blocks on wrong condition
Summary: robust mutexes: lowlevelrobustlock assembly on x86 blocks on wrong condition
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: nptl (show other bugs)
Version: 2.25
: P2 normal
Target Milestone: 2.25
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-21 12:33 UTC by Torvald Riegel
Modified: 2017-10-18 15:52 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Torvald Riegel 2016-12-21 12:33:27 UTC
lll_robust_unlock on i386 and x86_64 first sets the futex word to FUTEX_WAITERS|0 before calling __LLL_unlock_wake, which will set the futex word to 0.  If the thread is killed between these steps, then the futex word will be FUTEX_WAITERS|0, and the kernel (at least current upstream) will not set it to FUTEX_OWNER_DIED|FUTEX_WAITERS because 0 is not equal to the TID of the crashed thread.

The lll_robust_lock assembly code on i386 and x86_64 is not prepared to deal with this case because the fastpath tries to only CAS 0 to TID and not FUTEX_WAITERS|0 to TID; the slowpath simply waits until it can CAS 0 to TID or the futex_word has the FUTEX_OWNER_DIED bit set.

The reproducer in bug 19402 can trigger this behavior (if the core problem of 19402 is fixed).
Comment 1 cvs-commit@gcc.gnu.org 2017-01-13 16:19:41 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  8e31cafb268938729a1314806a924d73fb1991c5 (commit)
       via  65810f0ef05e8c9e333f17a44e77808b163ca298 (commit)
      from  f32941d80c7f532031061f8dd4704fab9c275cfe (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=8e31cafb268938729a1314806a924d73fb1991c5

commit 8e31cafb268938729a1314806a924d73fb1991c5
Author: Torvald Riegel <triegel@redhat.com>
Date:   Wed Dec 21 13:37:19 2016 +0100

    Clear list of acquired robust mutexes in the child process after forking.
    
    Robust mutexes acquired at the time of a call to fork() do not remain
    acquired by the forked child process.  We have to clear the list of
    acquired robust mutexes before registering this list with the kernel;
    otherwise, if some of the robust mutexes are process-shared, the parent
    process can alter the child's robust mutex list, which can lead to
    deadlocks or even modification of memory that may not be occupied by a
    mutex anymore.
    
    	[BZ #19402]
    	* sysdeps/nptl/fork.c (__libc_fork): Clear list of acquired robust
    	mutexes.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=65810f0ef05e8c9e333f17a44e77808b163ca298

commit 65810f0ef05e8c9e333f17a44e77808b163ca298
Author: Torvald Riegel <triegel@redhat.com>
Date:   Thu Dec 22 10:20:43 2016 +0100

    robust mutexes: Fix broken x86 assembly by removing it
    
    lll_robust_unlock on i386 and x86_64 first sets the futex word to
    FUTEX_WAITERS|0 before calling __lll_unlock_wake, which will set the
    futex word to 0.  If the thread is killed between these steps, then the
    futex word will be FUTEX_WAITERS|0, and the kernel (at least current
    upstream) will not set it to FUTEX_OWNER_DIED|FUTEX_WAITERS because 0 is
    not equal to the TID of the crashed thread.
    
    The lll_robust_lock assembly code on i386 and x86_64 is not prepared to
    deal with this case because the fastpath tries to only CAS 0 to TID and
    not FUTEX_WAITERS|0 to TID; the slowpath simply waits until it can CAS 0
    to TID or the futex_word has the FUTEX_OWNER_DIED bit set.
    
    This issue is fixed by removing the custom x86 assembly code and using
    the generic C code instead.  However, instead of adding more duplicate
    code to the custom x86 lowlevellock.h, the code of the lll_robust* functions
    is inlined into the single call sites that exist for each of these functions
    in the pthread_mutex_* functions.  The robust mutex paths in the latter
    have been slightly reorganized to make them simpler.
    
    This patch is meant to be easy to backport, so C11-style atomics are not
    used.
    
    	[BZ #20985]
    	* nptl/Makefile: Adapt.
    	* nptl/pthread_mutex_cond_lock.c (LLL_ROBUST_MUTEX_LOCK): Remove.
    	(LLL_ROBUST_MUTEX_LOCK_MODIFIER): New.
    	* nptl/pthread_mutex_lock.c (LLL_ROBUST_MUTEX_LOCK): Remove.
    	(LLL_ROBUST_MUTEX_LOCK_MODIFIER): New.
    	(__pthread_mutex_lock_full): Inline lll_robust* functions and adapt.
    	* nptl/pthread_mutex_timedlock.c (pthread_mutex_timedlock): Inline
    	lll_robust* functions and adapt.
    	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
    	* sysdeps/nptl/lowlevellock.h (__lll_robust_lock_wait,
    	__lll_robust_lock, lll_robust_cond_lock, __lll_robust_timedlock_wait,
    	__lll_robust_timedlock, __lll_robust_unlock): Remove.
    	* sysdeps/unix/sysv/linux/i386/lowlevellock.h (lll_robust_lock,
    	lll_robust_cond_lock, lll_robust_timedlock, lll_robust_unlock): Remove.
    	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (lll_robust_lock,
    	lll_robust_cond_lock, lll_robust_timedlock, lll_robust_unlock): Remove.
    	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (__lll_robust_lock_wait,
    	__lll_robust_lock, lll_robust_cond_lock, __lll_robust_timedlock_wait,
    	__lll_robust_timedlock, __lll_robust_unlock): Remove.
    	* nptl/lowlevelrobustlock.c: Remove file.
    	* nptl/lowlevelrobustlock.sym: Likewise.
    	* sysdeps/unix/sysv/linux/i386/lowlevelrobustlock.S: Likewise.
    	* sysdeps/unix/sysv/linux/x86_64/lowlevelrobustlock.S: Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                          |   33 ++
 nptl/Makefile                                      |    4 +-
 nptl/lowlevelrobustlock.c                          |  136 ---------
 nptl/lowlevelrobustlock.sym                        |    6 -
 nptl/pthread_mutex_cond_lock.c                     |    6 +-
 nptl/pthread_mutex_lock.c                          |   79 ++++--
 nptl/pthread_mutex_timedlock.c                     |  106 ++++++--
 nptl/pthread_mutex_unlock.c                        |   18 +-
 sysdeps/nptl/fork.c                                |   20 +-
 sysdeps/nptl/lowlevellock.h                        |   68 -----
 sysdeps/unix/sysv/linux/i386/lowlevellock.h        |   60 ----
 sysdeps/unix/sysv/linux/i386/lowlevelrobustlock.S  |  232 ---------------
 sysdeps/unix/sysv/linux/sparc/lowlevellock.h       |   40 ---
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.h      |   74 -----
 .../unix/sysv/linux/x86_64/lowlevelrobustlock.S    |  306 --------------------
 15 files changed, 205 insertions(+), 983 deletions(-)
 delete mode 100644 nptl/lowlevelrobustlock.c
 delete mode 100644 nptl/lowlevelrobustlock.sym
 delete mode 100644 sysdeps/unix/sysv/linux/i386/lowlevelrobustlock.S
 delete mode 100644 sysdeps/unix/sysv/linux/x86_64/lowlevelrobustlock.S
Comment 2 Torvald Riegel 2017-01-13 17:13:37 UTC
Fixed in master.