Linux sandbox: Provide AssertSingleThreaded() helper

Linux sandbox: Provide AssertSingleThreaded() helper

Provide a new helper API to assert being single threaded that can
wait on the kernel to update /proc/self/task/

Use the helper when started the seccomp sandbox single threaded.

BUG=455407
Committed: https://crrev.com/7b02f56d77aa785784f117c2c71e56987d80a57b
Cr-Commit-Position: refs/heads/master@{#315435}

[jln (very slow on Chromium), 2015-02-04 23:05:01]

jln@chromium.org changed reviewers:
+ rickyz@chromium.org

[jln (very slow on Chromium), 2015-02-04 23:05:21]

Ricky, could you please take a look?

[rickyz (no longer on Chrome), 2015-02-05 00:11:56]

lgtm

https://codereview.chromium.org/893993004/diff/40001/sandbox/linux/services/t...
File sandbox/linux/services/thread_helpers.cc (right):

https://codereview.chromium.org/893993004/diff/40001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:95: LOG(FATAL) <<
kAssertSingleThreadedError << " (iterations:" << iterations
nit: space after the colon.

https://codereview.chromium.org/893993004/diff/40001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:111: return
base::ScopedFD(HANDLE_EINTR(
Can we just use dup instead? Though maybe it's clearer to just have the two
cases for proc_self_task -1 vs. not in IsSingleThreaded.

[jln (very slow on Chromium), 2015-02-05 00:36:34]

New patchsets have been uploaded after l-g-t-m from rickyz@chromium.org

[jln (very slow on Chromium), 2015-02-05 00:36:49]

Thanks Ricky!

https://codereview.chromium.org/893993004/diff/40001/sandbox/linux/services/t...
File sandbox/linux/services/thread_helpers.cc (right):

https://codereview.chromium.org/893993004/diff/40001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:95: LOG(FATAL) <<
kAssertSingleThreadedError << " (iterations:" << iterations
On 2015/02/05 00:11:56, rickyz wrote:
> nit: space after the colon.

Done.

https://codereview.chromium.org/893993004/diff/40001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:111: return
base::ScopedFD(HANDLE_EINTR(
On 2015/02/05 00:11:56, rickyz wrote:
> Can we just use dup instead? Though maybe it's clearer to just have the two
> cases for proc_self_task -1 vs. not in IsSingleThreaded.

It requires dup3, which I think is not easily available on Android. But more
than that, I think we should change all APIs to take a fd to /proc or /proc/self
instead. Right now some APIs take /proc/self/task, others /proc/self/fd, which
is a bit of a mess.

[jln (very slow on Chromium), 2015-02-05 00:37:15]

jln@chromium.org changed reviewers:
+ mdempsky@chromium.org

[jln (very slow on Chromium), 2015-02-05 00:37:27]

Matthew: PTAL!

[jln (very slow on Chromium), 2015-02-05 01:00:17]

Matthew: send to CQ if you're happy with it, btw.

[mdempsky, 2015-02-05 02:12:42]

lgtm

https://codereview.chromium.org/893993004/diff/60001/sandbox/linux/services/t...
File sandbox/linux/services/thread_helpers.cc (right):

https://codereview.chromium.org/893993004/diff/60001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:51: fstatat(proc_self_task,
thread_id_dir_str.c_str(), &task_stat, 0);
minor: Since you don't care about the stat result, you could potentially use
faccessat() with F_OK.  But might be simpler to understand as is.

https://codereview.chromium.org/893993004/diff/60001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:65: void RunUntilFalse(const
base::Callback<bool(void)>& cb) {
"RunWhile" or "RunWhileTrue" feels like a more intuitive name IMO.

https://codereview.chromium.org/893993004/diff/60001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:72: while (true) {
I would think this would be more readable as something like:

  #if ...
  const unsigned int kMaxIterations = ...
  #else
  ...
  #endif

  for (unsigned int i = 0; i < kMaxIterations; ++i) {
    if (!cb.Run()) {
      return;
    }

    nanosleep(...);
  }

  LOG(FATAL) << ...
  NOTREACHED();

https://codereview.chromium.org/893993004/diff/60001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:112: openat(proc_self_task, "./",
O_RDONLY | O_DIRECTORY | O_CLOEXEC)));
Two small questions:
  1. Why use openat() instead of fcntl() with F_DUPFD_CLOEXEC?
  2. Assuming openat() is needed, any reason to use "./" here instead of "."?

https://codereview.chromium.org/893993004/diff/60001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:122: bool
ThreadHelpers::IsSingleThreaded(int proc_self_task) {
This feels like a kinda clunky interface.  What about just having two separate
functions: one that takes a file descriptor (which must be valid), and one that
opens internally?

[jln (very slow on Chromium), 2015-02-09 21:54:40]

New patchsets have been uploaded after l-g-t-m from mdempsky@chromium.org

[jln (very slow on Chromium), 2015-02-09 22:24:51]

Thanks Matthew!

I left a few of the nits as-is. In particular I keep for future CLs a
unification of the interface to functions that can take a fd but "try to work"
if they weren't given any.

I agree that it's a bit ugly, but it saves quite a bit of code.

I think they should all take a base::ScopedFD() anyways. But it would still be
quite ugly to pass invalid ScopedFD() around.

https://chromiumcodereview.appspot.com/893993004/diff/60001/sandbox/linux/ser...
File sandbox/linux/services/thread_helpers.cc (right):

https://chromiumcodereview.appspot.com/893993004/diff/60001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.cc:51: fstatat(proc_self_task,
thread_id_dir_str.c_str(), &task_stat, 0);
On 2015/02/05 02:12:42, mdempsky wrote:
> minor: Since you don't care about the stat result, you could potentially use
> faccessat() with F_OK.  But might be simpler to understand as is.

Good point! I'll keep as-is for now so that I don't have to go through another
cycle of updating seccomp policies.

https://chromiumcodereview.appspot.com/893993004/diff/60001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.cc:65: void RunUntilFalse(const
base::Callback<bool(void)>& cb) {
On 2015/02/05 02:12:42, mdempsky wrote:
> "RunWhile" or "RunWhileTrue" feels like a more intuitive name IMO.

Done.

https://chromiumcodereview.appspot.com/893993004/diff/60001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.cc:72: while (true) {
Ooch yeah, it looks like this re-factor wasn't complete yet.

Thanks, that's much nicer!

https://chromiumcodereview.appspot.com/893993004/diff/60001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.cc:112: openat(proc_self_task, "./",
O_RDONLY | O_DIRECTORY | O_CLOEXEC)));
On 2015/02/05 02:12:41, mdempsky wrote:
> Two small questions:
>   1. Why use openat() instead of fcntl() with F_DUPFD_CLOEXEC?

Ahh yeah, good point. I wanted to avoid dup3(), but F_DUPFD_CLOEXEC would work.

The other reasons were:
- The symmetry between open() and openat() is nice (I should probably even use
openat() with AT_FDCWD instead of open). Given seccomp etc, it's always nice to
reduce the syscall footprint.

- we have methods that take handles to /proc/self/task/ and others to
/proc/self/ and I want to unify everything (to /proc/self/), so the pattern
would already be correct.

>   2. Assuming openat() is needed, any reason to use "./" here instead of "."?

I like the pattern of "if X must be a directory, enforce it with X/". I admit
that this could be pushing the paranoia a little, given O_DIRECTORY.

https://chromiumcodereview.appspot.com/893993004/diff/60001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.cc:122: bool
ThreadHelpers::IsSingleThreaded(int proc_self_task) {
On 2015/02/05 02:12:42, mdempsky wrote:
> This feels like a kinda clunky interface.  What about just having two separate
> functions: one that takes a file descriptor (which must be valid), and one
that
> opens internally?

Yeah, you're right. I won't change it at this point as it should be part of a
larger refactor if we decide to do it (there are proxy function for these in
content/).

I'm not 100% sold yet: there are a bunch of functions in content/ with a similar
interface at this point and they all can call each other. If we unbundle one, we
need to unbundle all of them.

But maybe it's the right thing to do.

[jln (very slow on Chromium), 2015-02-09 22:34:54]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2015-02-09 22:37:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/893993004/100001

[jln (very slow on Chromium), 2015-02-09 22:40:20]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2015-02-09 22:41:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/893993004/120001

[commit-bot: I haz the power, 2015-02-09 23:34:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-09 23:35:07]

Patchset 7 (id:??) landed as
https://crrev.com/7b02f56d77aa785784f117c2c71e56987d80a57b
Cr-Commit-Position: refs/heads/master@{#315435}