Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028303005:
Add PTHREAD_PRIO_INHERIT to POSIX Locks (Closed)
|
Created:
4 years, 6 months ago by robliao Modified:
4 years, 4 months ago CC:
chromium-reviews, rickyz+watch_chromium.org, jln+watch_chromium.org, gab, fdoray, jschuh, jln (very slow on Chromium), rickyz (no longer on Chrome) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd PTHREAD_PRIO_INHERIT to POSIX Locks
The Browser Task Scheduler will be using thread priorities.
PTHREAD_PRIO_INHERIT will help avoid priority inversion with the use of thread
priorities.
BUG=611856
Patch Set 1 #
Total comments: 2
Patch Set 2 : CR Feedback #
Total comments: 4
Patch Set 3 : CR Feedback #
Messages
Total messages: 41 (15 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303005/1
robliao@chromium.org changed reviewers: + danakj@chromium.org
danakj: Please review base/* for this CL. Thanks! Perf impact for mac: https://codereview.chromium.org/1971093004/ Perf impact on linux unavailable due to issues in the perfbot.
This looks kinda bad: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-05... no?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/06/02 20:56:37, danakj wrote: > This looks kinda bad: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-05... > no? The results had a large uncertainty that it was hard to conclude if it was better or worse. page_load_time = 4963.63 ± 68.06% (22.26% Worse) 4059.98 ± 46.65% warm_times = 4337.94 ± 67.43% (16.26% Better) 5180.55 ± 109.12% I figured that from this there was unlikely to be an improvement or regression.
I'd like to have someone review the sandbox stuff, it's strange and foreign, so I can understand why we need the POSSIBLE define at all. The rest seems okay. https://codereview.chromium.org/2028303005/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2028303005/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:79: return glibc_major_version != 2 || glibc_minor_version >= 17; You can use std::tie to combine these into one a < b check.
> I'd like to have someone review the sandbox stuff, it's strange and foreign, so > I can understand why we need the POSSIBLE define at all. The rest seems okay. No worries! I'm going to have the sandbox people review that. Just wanted to make sure there were no objections to adding this support to base::LockImpl. The reason why we need POSSIBLE is because pthreads is inconsistent from platform to platform. NACL doesn't have PTHREAD_PRIO_INHERIT and Android does have PTHREAD_PRIO_INHERIT or pthread_mutexattr_setprotocol. https://codereview.chromium.org/2028303005/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2028303005/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:79: return glibc_major_version != 2 || glibc_minor_version >= 17; On 2016/06/02 21:59:21, danakj wrote: > You can use std::tie to combine these into one a < b check. Neat! Done.
On Thu, Jun 2, 2016 at 3:17 PM, <robliao@chromium.org> wrote: > > I'd like to have someone review the sandbox stuff, it's strange and > foreign, > so > > I can understand why we need the POSSIBLE define at all. The rest seems > okay. > > No worries! I'm going to have the sandbox people review that. Just wanted > to > make sure there were no objections to adding this support to > base::LockImpl. > Ya given the apparent no perf impact, making our lock support this on posix seems okay with me. > > The reason why we need POSSIBLE is because pthreads is inconsistent from > platform to platform. > > NACL doesn't have PTHREAD_PRIO_INHERIT and Android does have > PTHREAD_PRIO_INHERIT or pthread_mutexattr_setprotocol. > > > > https://codereview.chromium.org/2028303005/diff/1/base/synchronization/lock_i... > File base/synchronization/lock_impl_posix.cc (right): > > > https://codereview.chromium.org/2028303005/diff/1/base/synchronization/lock_i... > base/synchronization/lock_impl_posix.cc:79: return glibc_major_version > != 2 || glibc_minor_version >= 17; > On 2016/06/02 21:59:21, danakj wrote: > > You can use std::tie to combine these into one a < b check. > > Neat! Done. > > https://codereview.chromium.org/2028303005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
robliao@chromium.org changed reviewers: + rickyz@chromium.org
rickyz@chromium.org: Please review changes in sandbox/* Thanks!
robliao@chromium.org changed reviewers: + jln@chromium.org
rickyz is OOO this week, adding jln. jln@chromium.org: Please review changes in sandbox/* Thanks!
Drive-by, thanks for doing this :-) https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... base/synchronization/lock.h:21: // available. Lock::PriorityInheritanceAvailable must still be checked. Why not have everyone go through the method and encode this macro in it? https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:67: // Due to glibc Bug 14652, priority inheritance mutexes may deadlock with Link to bug? (googling "glibc Bug 14652" doesn't yield much)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... base/synchronization/lock.h:21: // available. Lock::PriorityInheritanceAvailable must still be checked. On 2016/06/07 12:28:18, gab wrote: > Why not have everyone go through the method and encode this macro in it? A couple of reasons: 1) Callers who care about this may call libraries that aren't available when priority inheritance isn't possible at compile time. 2) The sandbox unit tests use this to conditionally include the tests. I went ahead and included in this check in PriorityInheritanceAvailable. https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:67: // Due to glibc Bug 14652, priority inheritance mutexes may deadlock with On 2016/06/07 12:28:19, gab wrote: > Link to bug? (googling "glibc Bug 14652" doesn't yield much) Done.
On 2016/06/07 14:11:04, robliao wrote: > https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... > File base/synchronization/lock.h (right): > > https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... > base/synchronization/lock.h:21: // available. Lock::PriorityInheritanceAvailable > must still be checked. > On 2016/06/07 12:28:18, gab wrote: > > Why not have everyone go through the method and encode this macro in it? > > A couple of reasons: > 1) Callers who care about this may call libraries that aren't available when > priority inheritance isn't possible at compile time. > 2) The sandbox unit tests use this to conditionally include the tests. > > I went ahead and included in this check in PriorityInheritanceAvailable. > > https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... > File base/synchronization/lock_impl_posix.cc (right): > > https://codereview.chromium.org/2028303005/diff/20001/base/synchronization/lo... > base/synchronization/lock_impl_posix.cc:67: // Due to glibc Bug 14652, priority > inheritance mutexes may deadlock with > On 2016/06/07 12:28:19, gab wrote: > > Link to bug? (googling "glibc Bug 14652" doesn't yield much) > > Done. Ping for the sandbox OWNERs!
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robliao@chromium.org changed reviewers: + jschuh@chromium.org
Moving up a directory level. jschuh: Please review the changes in sandbox/*. Thanks!
Description was changed from ========== Add PTHREAD_PRIO_INHERIT to POSIX Locks The Browser Task Scheduler will be using thread priorities. PTHREAD_PRIO_INHERIT will help avoid priority inversion with the use of thread priorities. BUG=611856 ========== to ========== Add PTHREAD_PRIO_INHERIT to POSIX Locks The Browser Task Scheduler will be using thread priorities. PTHREAD_PRIO_INHERIT will help avoid priority inversion with the use of thread priorities. BUG=611856 ==========
robliao@chromium.org changed reviewers: - jln@chromium.org, jschuh@chromium.org, rickyz@chromium.org
robliao@chromium.org changed reviewers: + mdempsky@chromium.org
Adding mdempsky@ on recommendation by jschuh@. mdempsky@chromium.org: Please review changes in sandbox/*. Thanks!
mdempsky@chromium.org changed reviewers: + jln@chromium.org
Hm, this is a bit tricky because we actually specifically wanted to avoid allowing futex's *_PI operations due to security concerns over kernel exploits (e.g., CVE-2010-0622, CVE-2012-6647, CVE-2014-3153). Is there any way we can avoid requiring these operations?
On 2016/06/16 20:45:52, mdempsky wrote: > Hm, this is a bit tricky because we actually specifically wanted to avoid > allowing futex's *_PI operations due to security concerns over kernel exploits > (e.g., CVE-2010-0622, CVE-2012-6647, CVE-2014-3153). > > Is there any way we can avoid requiring these operations? Unfortunately for Linux glibc POSIX, the pthread mutex uses Futex with PI to accomplish its work. No PI Futex seems to mean no Priority Inheritance for Linux glibc POSIX. https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/pthread_mutex_lock.c Do you have any recommendations on alternatives?
On 2016/06/16 20:58:50, robliao wrote: > Do you have any recommendations on alternatives? Nothing concrete, unfortunately. I notice the linked bug describes using priority inheritance to "help avoid" priority inversion. Am I misreading that wording to understand we should be able to avoid priority inversion even without priority inheritance? Would it be helpful at all if we allowed priority inheritance under some compile-time or run-time flag, but defaulted it to off for release builds? Can we build some sort of "priority sanitizer" to help detect mistakes?
On 2016/06/16 21:09:10, mdempsky wrote: > On 2016/06/16 20:58:50, robliao wrote: > > Do you have any recommendations on alternatives? > > Nothing concrete, unfortunately. > > I notice the linked bug describes using priority inheritance to "help avoid" > priority inversion. Am I misreading that wording to understand we should be able > to avoid priority inversion even without priority inheritance? > > Would it be helpful at all if we allowed priority inheritance under some > compile-time or run-time flag, but defaulted it to off for release builds? Can > we build some sort of "priority sanitizer" to help detect mistakes? "help avoid" may not have been assertive enough on the bug. Chrome does not extensively use priorities today, avoiding the problem. We want to turn on priorities so that important threads gets prioritized over background threads. It's natural for important threads and background threads to contend over the same resources (say counters), so I'm not sure it's possible to avoid all priority inversion scenarios. Linux glibc is the only platform where this feature seems to be problematic. Mac seems to implement priority inheritance correctly (thus far). Windows uses random priority boosting on ready threads to get around the problem.
On 2016/06/16 21:33:42, robliao wrote: > "help avoid" may not have been assertive enough on the bug. > > Chrome does not extensively use priorities today, avoiding the problem. > We want to turn on priorities so that important threads gets prioritized over > background threads. I see. Thanks for clarifying. > It's natural for important threads and background threads to contend over the > same resources (say counters), so I'm not sure it's possible to avoid all > priority inversion scenarios. Is it possible to get some data on how common these problem scenarios are and/or how severe the consequences are? Or how hard they would be to prevent without priority inheritance? Like I pointed out, the futex PI operations have an unfortunate history of kernel exploits, and unfortunately using bpf_dsl to disallow them is our only safeguard against future exploits. If it's critical that we re-enable them, then so be it: chrome-security@ will have to do what it can to improve its confidence in PI futex ops. But I'd really like to make sure we evaluate all possible alternatives before committing to a solution.
On 2016/06/16 21:51:40, mdempsky wrote: > On 2016/06/16 21:33:42, robliao wrote: > > "help avoid" may not have been assertive enough on the bug. > > > > Chrome does not extensively use priorities today, avoiding the problem. > > We want to turn on priorities so that important threads gets prioritized over > > background threads. > > I see. Thanks for clarifying. > > > It's natural for important threads and background threads to contend over the > > same resources (say counters), so I'm not sure it's possible to avoid all > > priority inversion scenarios. > > Is it possible to get some data on how common these problem scenarios are and/or > how severe the consequences are? Or how hard they would be to prevent without > priority inheritance? > > Like I pointed out, the futex PI operations have an unfortunate history of > kernel exploits, and unfortunately using bpf_dsl to disallow them is our only > safeguard against future exploits. If it's critical that we re-enable them, > then so be it: chrome-security@ will have to do what it can to improve its > confidence in PI futex ops. But I'd really like to make sure we evaluate all > possible alternatives before committing to a solution. > Is it possible to get some data on how common these problem scenarios are and/or >how severe the consequences are? Or how hard they would be to prevent without >priority inheritance? Hard to say without throwing the switch. We're going to be enabling priorities for Windows (and likely mac), so we can probably add some bookkeeping into base::Lock's existing validation checker to measure these scenarios.
Message was sent while issue was closed.
Closing - Superseded by https://codereview.chromium.org/2178503003/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
