VOOZH about

URL: https://codereview.chromium.org/759343002/

⇱ Issue 759343002: sandbox: sandbox_linux_unittests enforces TSYNC on ChromeOS - Code Review

Keyboard Shortcuts
File
u :up to issue
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
Issue
u :up to list of issues
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
👁 Image
Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(110)

Issues Search
    My Issues | Starred     Open | Closed | All

👁 Image
 Issue 759343002: sandbox: sandbox_linux_unittests enforces TSYNC on ChromeOS (Closed)

Created:
6 years ago by leecam
Modified:
6 years ago
CC:
chromium-reviews, jln+watch_chromium.org, Kees Cook
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

sandbox: sandbox_linux_unittests enforces TSYNC on ChromeOS Make Seccomp-bpf tsync support mandatory on ChromeOS now that all kernels support it. BUG= TEST=ChromeOS trybots Committed: https://crrev.com/d6b6dcb15f0d15d65de1decbb165afb427d570a8 Cr-Commit-Position: refs/heads/master@{#306673}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Check for running on ChromeOS #

Patch Set 3 : Adding header #

Total comments: 4

Patch Set 4 : nits #

Created: 6 years ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
👁 Image
 M sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc View 1 2 3 2 chunks +12 lines, -2 lines 0 comments Download

Messages

Total messages: 17 (4 generated)
leecam
leecam@chromium.org changed reviewers: + jorgelo@chromium.org
6 years ago (2014-11-26 22:05:38 UTC) #1
leecam
PTAL
6 years ago (2014-11-26 22:05:55 UTC) #2 
PTAL
leecam
keescook: Hey Kees, Jorge and I would like to make tsync support for ChromeOS mandatory. ...
6 years ago (2014-11-26 22:08:49 UTC) #3 
keescook: Hey Kees, Jorge and I would like to make tsync support for ChromeOS
mandatory.
We have tried this against the trybots with different kernel variants. Is our
assumption that all the boards now have tsync correct?
Jorge Lucangeli Obes
On 2014/11/26 22:08:49, leecam wrote: > keescook: Hey Kees, Jorge and I would like to ...
6 years ago (2014-11-26 22:12:03 UTC) #4 
On 2014/11/26 22:08:49, leecam wrote:
> keescook: Hey Kees, Jorge and I would like to make tsync support for ChromeOS
> mandatory.
> We have tried this against the trybots with different kernel variants. Is our
> assumption that all the boards now have tsync correct?

Also, the plan is to land this next week to be able to quickly revert if
something goes wrong.
Kees Cook
keescook@chromium.org changed reviewers: + keescook@chromium.org
6 years ago (2014-11-26 22:13:54 UTC) #5
Kees Cook
lgtm All the Chrome OS kernels should have tsync now, so yeah, go for it!
6 years ago (2014-11-26 22:13:55 UTC) #6 
lgtm

All the Chrome OS kernels should have tsync now, so yeah, go for it!
jln (very slow on Chromium)
jln@chromium.org changed reviewers: + jln@chromium.org
6 years ago (2014-11-26 22:22:32 UTC) #7 
jln@chromium.org changed reviewers:
+ jln@chromium.org
jln (very slow on Chromium)
https://codereview.chromium.org/759343002/diff/1/sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right): https://codereview.chromium.org/759343002/diff/1/sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc#newcode2271 sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2271: SandboxBPF::SupportsSeccompThreadFilterSynchronization(); This is a very outdated API, you should ...
6 years ago (2014-11-26 22:22:33 UTC) #8 
https://codereview.chromium.org/759343002/diff/1/sandbox/linux/bpf_dsl/bpf_ds...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right):

https://codereview.chromium.org/759343002/diff/1/sandbox/linux/bpf_dsl/bpf_ds...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2271:
SandboxBPF::SupportsSeccompThreadFilterSynchronization();
This is a very outdated API, you should re-sync :)

https://codereview.chromium.org/759343002/diff/1/sandbox/linux/bpf_dsl/bpf_ds...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2272: #if defined(OS_CHROMEOS)
This won't work unfortunately.

OS_CHROMEOS is just a type of build, that we run on Linux machines currently on
the WF.

We could ask infra to make sure that they're all running Trusty with updated
kernels, but that will take a little bit.
leecam
Ok how about this... I ran this change on a few ChromeOS bots and all ...
6 years ago (2014-12-03 17:01:32 UTC) #9 
Ok how about this...

I ran this change on a few ChromeOS bots and all green there too.
Jorge Lucangeli Obes
Two comment nits. Have you ran enough trybots to cover all kernels? https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc ...
6 years ago (2014-12-03 17:19:38 UTC) #10 
Two comment nits. Have you ran enough trybots to cover all kernels?

https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right):

https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2259: // On ChromeOS tsync is
mandatory.
Nit: "Chrome OS" with a space.

https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2264: // else a ChromeOS build
not running on a ChromeOS device e.g. chrome bots.
Same nit, and you probably want to capitalize Chrome.
leecam
https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right): https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc#newcode2259 sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2259: // On ChromeOS tsync is mandatory. On 2014/12/03 17:19:38, ...
6 years ago (2014-12-03 17:25:23 UTC) #11 
https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right):

https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2259: // On ChromeOS tsync is
mandatory.
On 2014/12/03 17:19:38, Jorge Lucangeli Obes wrote:
> Nit: "Chrome OS" with a space.

Done.

https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2264: // else a ChromeOS build
not running on a ChromeOS device e.g. chrome bots.
On 2014/12/03 17:19:38, Jorge Lucangeli Obes wrote:
> Same nit, and you probably want to capitalize Chrome.

Done.
Jorge Lucangeli Obes
lgtm, assuming trybot coverage on all kernel versions.
6 years ago (2014-12-03 17:28:35 UTC) #12 
lgtm, assuming trybot coverage on all kernel versions.
leecam
On 2014/12/03 17:19:38, Jorge Lucangeli Obes wrote: > Two comment nits. Have you ran enough ...
6 years ago (2014-12-03 17:30:33 UTC) #13 
On 2014/12/03 17:19:38, Jorge Lucangeli Obes wrote:
> Two comment nits. Have you ran enough trybots to cover all kernels?
I've just set the full set of bots off again. x86 is all green and ran all the
tests, some bots didnt run the full tests.
Will report back in few hours

> 
>
https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
> File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right):
> 
>
https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
> sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2259: // On ChromeOS tsync is
> mandatory.
> Nit: "Chrome OS" with a space.
> 
>
https://codereview.chromium.org/759343002/diff/40001/sandbox/linux/bpf_dsl/bp...
> sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:2264: // else a ChromeOS build
> not running on a ChromeOS device e.g. chrome bots.
> Same nit, and you probably want to capitalize Chrome.
leecam
The CQ bit was checked by leecam@chromium.org
6 years ago (2014-12-03 20:53:24 UTC) #14 
The CQ bit was checked by leecam@chromium.org
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/759343002/60001
6 years ago (2014-12-03 20:54:23 UTC) #15
commit-bot: I haz the power
Committed patchset #4 (id:60001)
6 years ago (2014-12-03 20:57:04 UTC) #16
Message was sent while issue was closed. 
Committed patchset #4 (id:60001)
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/d6b6dcb15f0d15d65de1decbb165afb427d570a8 Cr-Commit-Position: refs/heads/master@{#306673}
6 years ago (2014-12-03 20:57:53 UTC) #17
Message was sent while issue was closed. 
Patchset 4 (id:??) landed as
https://crrev.com/d6b6dcb15f0d15d65de1decbb165afb427d570a8
Cr-Commit-Position: refs/heads/master@{#306673}
👁 Powered by Google App Engine
This is Rietveld 408576698