Linux: unify seccomp sandbox initialization

Linux: unify seccomp sandbox initialization

We unify seccomp-bpf and seccomp-legacy initialization in the same
file.

TBR=jamesr@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149381

[jln (very slow on Chromium), 2012-07-31 22:14:55]

This is a small first step to unifying the sandbox initialization code.

Now we initialize seccomp-legacy at the same place we do for seccomp-bpf.

[Markus (顧孟勤), 2012-07-31 23:17:37]

lgtm

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:572: // First, try to enable
seccomp-legacy.
I think I understand this logic. But just verifying. If I pass
--enable-seccomp-sandbox on the command line, or if I build in debug mode, I
will get legacy sandbox support.

In all other cases, I either get BPF support (if enabled in the kernel) or no
sandbox.

Is this correct? If so, that's fine (and probably unchanged from our existing
behavior), but it is also quite complex. You might want to add a comment
explaining this.

[Jorge Lucangeli Obes, 2012-07-31 23:24:29]

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:443: bool ShouldDisableSandbox_bpf(const
CommandLine& command_line,
The naming scheme looks weird. Do we have examples of using underscores in
Chrome code?

ShouldDisableSeccompBpfSandbox? ShouldDisableBpfSandbox?

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:444: const std::string& process_type) {
Weird indentation left behind.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:502: bool InitializeSandbox_bpf_x86(const
CommandLine& command_line,
This is getting kinda crazy =P. InitializeBpfSandbox_x86? I don't like the
underscore but InitializeBpfSandboxX86 also looks weird.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:541: bool
InitializeSandbox_legacy_x86(const CommandLine& command_line,
InitializeLegacySandbox_x86 or InitializeLegacySandboxX86?

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:587: LogSandboxStarted("seccomp-bpf",
process_type);
We've been logging "seccomp filter" before, and that's the name of the flag.
It's kinda confusing if the flag enables a sandbox named differently.

[jln (very slow on Chromium), 2012-07-31 23:48:27]

PTAL

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:443: bool ShouldDisableSandbox_bpf(const
CommandLine& command_line,
On 2012/07/31 23:24:29, Jorge Lucangeli Obes wrote:
> The naming scheme looks weird. Do we have examples of using underscores in
> Chrome code?
> 
> ShouldDisableSeccompBpfSandbox? ShouldDisableBpfSandbox?

You're right. I've used _X86 because it's special and is there as a "warning you
probably need a #ifdef somewhere". This is not warranted here.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:444: const std::string& process_type) {
On 2012/07/31 23:24:29, Jorge Lucangeli Obes wrote:
> Weird indentation left behind.

Done.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:502: bool InitializeSandbox_bpf_x86(const
CommandLine& command_line,
On 2012/07/31 23:24:29, Jorge Lucangeli Obes wrote:
> This is getting kinda crazy =P. InitializeBpfSandbox_x86? I don't like the
> underscore but InitializeBpfSandboxX86 also looks weird.

Done.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:541: bool
InitializeSandbox_legacy_x86(const CommandLine& command_line,
On 2012/07/31 23:24:29, Jorge Lucangeli Obes wrote:
> InitializeLegacySandbox_x86 or InitializeLegacySandboxX86?

Done.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:572: // First, try to enable
seccomp-legacy.
On 2012/07/31 23:17:37, Markus (顧孟勤) wrote:
> I think I understand this logic. But just verifying. If I pass
> --enable-seccomp-sandbox on the command line, or if I build in debug mode, I
> will get legacy sandbox support.
> 
> In all other cases, I either get BPF support (if enabled in the kernel) or no
> sandbox.
> 
> Is this correct? If so, that's fine (and probably unchanged from our existing
> behavior), but it is also quite complex. You might want to add a comment
> explaining this.

Yes, the behavior is unchanged in this CL.

Other refactors are coming and will make this more simple. For instance
SeccompSandboxEnabled() is currently in a header file behind #ifdef and should
soon be part of the LinuxSandbox singleton.

https://chromiumcodereview.appspot.com/10828102/diff/7001/content/common/sand...
content/common/sandbox_init_linux.cc:587: LogSandboxStarted("seccomp-bpf",
process_type);
On 2012/07/31 23:24:29, Jorge Lucangeli Obes wrote:
> We've been logging "seccomp filter" before, and that's the name of the flag.
> It's kinda confusing if the flag enables a sandbox named differently.

Yeah, but all those names confuse people a lot. I think we should really aim to
match the directories in /sandbox/linux. Ideally we would rename the flag, but
that's a tad annoying for legacy reasons.

I would suggest we keep things like this. Let me know what you think.

[Jorge Lucangeli Obes, 2012-08-01 00:18:08]

lgtm

[commit-bot: I haz the power, 2012-08-01 00:29:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10828102/10002

[commit-bot: I haz the power, 2012-08-01 00:29:25]

Presubmit check for 10828102-10002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer

Presubmit checks took 1.0s to calculate.

[commit-bot: I haz the power, 2012-08-01 04:46:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10828102/10002

[commit-bot: I haz the power, 2012-08-01 06:29:45]

Change committed as 149381