Clean up GPU process seccomp-bpf sandbox policies.

Clean up GPU process seccomp-bpf sandbox policies.

BUG=140901
TEST=WebGL conformance tests on Chrome OS.


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

[Jorge Lucangeli Obes, 2012-08-06 21:01:48]

Let's do some cleanup!

PTAL

[jln (very slow on Chromium), 2012-08-06 21:17:37]

- This is changing the semantic for stock Linux, from enabled by default to
disabled by default, isn't it ?
This may be the right choice temporarily, (I'm not too fond of this particular
policy being used generically), but this should be documented in the commit
message. Please also check with/inform cevans@.

- Any chance we could be a bit more aggressive and replace AllowAllPolicy by
BlackListPtracePolicy ? Since everything can still be disabled via
--disable-gpu-sandbox I don't think this is a big concern.

- It's not great that we have three states that do different things:
1. No flags at all (AllowAllPolicy or BlackListPtracePolicy)
2. --enable (GpuProcessPolicy_x86_64)
3. --disable (no seccomp bpf at all).

It's a bit counter intuitive to have 3 states for one enable/disable flag.
Perhaps the --enable flag should be renamed to --enabled-advanced-gpu-sandbox or
some such ? Feel free to punt on that one, it's not a huge deal.

[Jorge Lucangeli Obes, 2012-08-06 21:20:58]

On 2012/08/06 21:17:37, Julien Tinnes wrote:
> - This is changing the semantic for stock Linux, from enabled by default to
> disabled by default, isn't it ?
> This may be the right choice temporarily, (I'm not too fond of this particular
> policy being used generically), but this should be documented in the commit
> message. Please also check with/inform cevans@.

Why would this change the default? By default, --disable-gpu-sandbox is not
added to the cmdline, so on stock Linux we would return |false|.

> - Any chance we could be a bit more aggressive and replace AllowAllPolicy by
> BlackListPtracePolicy ? Since everything can still be disabled via
> --disable-gpu-sandbox I don't think this is a big concern.

We're shipping with "--enable-gpu-process" on Chrome OS so we're using Blacklist
already. My deal with the GPU guys is that no flag == no blocking.

> - It's not great that we have three states that do different things:
> 1. No flags at all (AllowAllPolicy or BlackListPtracePolicy)
> 2. --enable (GpuProcessPolicy_x86_64)
> 3. --disable (no seccomp bpf at all).
> 
> It's a bit counter intuitive to have 3 states for one enable/disable flag.
> Perhaps the --enable flag should be renamed to --enabled-advanced-gpu-sandbox
or
> some such ? Feel free to punt on that one, it's not a huge deal.

[jln (very slow on Chromium), 2012-08-06 21:30:28]

On 2012/08/06 21:20:58, Jorge Lucangeli Obes wrote:
> On 2012/08/06 21:17:37, Julien Tinnes wrote:
> > - This is changing the semantic for stock Linux, from enabled by default to
> > disabled by default, isn't it ?
> > This may be the right choice temporarily, (I'm not too fond of this
particular
> > policy being used generically), but this should be documented in the commit
> > message. Please also check with/inform cevans@.
> 
> Why would this change the default? By default, --disable-gpu-sandbox is not
> added to the cmdline, so on stock Linux we would return |false|.

But we don't pass --enable-gpu-sandbox by default either, so we would now end up
with AllowAll where we previously had the real policy on stock Linux, no ?

> > - Any chance we could be a bit more aggressive and replace AllowAllPolicy by
> > BlackListPtracePolicy ? Since everything can still be disabled via
> > --disable-gpu-sandbox I don't think this is a big concern.
> 
> We're shipping with "--enable-gpu-process" on Chrome OS so we're using
Blacklist
> already. My deal with the GPU guys is that no flag == no blocking.

You mean --enable-gpu-sandbox and "using
GpuProcessPolicy_x86_64/BlackListPtrace", right? It would be nice for
BlackListPtrace to become the new "AllowAll", as long as we maintain "emergency
shut down switches". But it's your call, we can do that later.

> > - It's not great that we have three states that do different things:
> > 1. No flags at all (AllowAllPolicy or BlackListPtracePolicy)
> > 2. --enable (GpuProcessPolicy_x86_64)
> > 3. --disable (no seccomp bpf at all).
> > 
> > It's a bit counter intuitive to have 3 states for one enable/disable flag.
> > Perhaps the --enable flag should be renamed to
--enabled-advanced-gpu-sandbox
> or
> > some such ? Feel free to punt on that one, it's not a huge deal.

[jln (very slow on Chromium), 2012-08-06 21:36:50]

https://chromiumcodereview.appspot.com/10836118/diff/1/content/common/sandbox...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/10836118/diff/1/content/common/sandbox...
content/common/sandbox_seccomp_bpf_linux.cc:431: return
command_line.HasSwitch(switches::kDisableGpuSandbox);
Also, could you kill this function and inline this in
SandboxSeccompBpf::ShouldEnableSeccompBpf() ?

[Jorge Lucangeli Obes, 2012-08-06 21:37:44]

On 2012/08/06 21:30:28, Julien Tinnes wrote:
> On 2012/08/06 21:20:58, Jorge Lucangeli Obes wrote:
> > On 2012/08/06 21:17:37, Julien Tinnes wrote:
> > > - This is changing the semantic for stock Linux, from enabled by default
to
> > > disabled by default, isn't it ?
> > > This may be the right choice temporarily, (I'm not too fond of this
> particular
> > > policy being used generically), but this should be documented in the
commit
> > > message. Please also check with/inform cevans@.
> > 
> > Why would this change the default? By default, --disable-gpu-sandbox is not
> > added to the cmdline, so on stock Linux we would return |false|.
> 
> But we don't pass --enable-gpu-sandbox by default either, so we would now end
up
> with AllowAll where we previously had the real policy on stock Linux, no ?

C'est vrai ça.

> > > - Any chance we could be a bit more aggressive and replace AllowAllPolicy
by
> > > BlackListPtracePolicy ? Since everything can still be disabled via
> > > --disable-gpu-sandbox I don't think this is a big concern.
> > 
> > We're shipping with "--enable-gpu-process" on Chrome OS so we're using
> Blacklist
> > already. My deal with the GPU guys is that no flag == no blocking.
> 
> You mean --enable-gpu-sandbox and "using
> GpuProcessPolicy_x86_64/BlackListPtrace", right? It would be nice for
> BlackListPtrace to become the new "AllowAll", as long as we maintain
"emergency
> shut down switches". But it's your call, we can do that later.
> 
> > > - It's not great that we have three states that do different things:
> > > 1. No flags at all (AllowAllPolicy or BlackListPtracePolicy)
> > > 2. --enable (GpuProcessPolicy_x86_64)
> > > 3. --disable (no seccomp bpf at all).
> > > 
> > > It's a bit counter intuitive to have 3 states for one enable/disable flag.
> > > Perhaps the --enable flag should be renamed to
> --enabled-advanced-gpu-sandbox
> > or
> > > some such ? Feel free to punt on that one, it's not a huge deal.

[Jorge Lucangeli Obes, 2012-08-06 21:38:57]

On 2012/08/06 21:36:50, Julien Tinnes wrote:
>
https://chromiumcodereview.appspot.com/10836118/diff/1/content/common/sandbox...
> File content/common/sandbox_seccomp_bpf_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10836118/diff/1/content/common/sandbox...
> content/common/sandbox_seccomp_bpf_linux.cc:431: return
> command_line.HasSwitch(switches::kDisableGpuSandbox);
> Also, could you kill this function and inline this in
> SandboxSeccompBpf::ShouldEnableSeccompBpf() ?

Hmmm I thought we might find it useful to keep a "single point of disable" but I
guess since hopefully we'll never have something as complicated as the old GPU
enable logic, we can keep everything in ShouldEnable.

[jln (very slow on Chromium), 2012-08-06 21:44:12]

On 2012/08/06 21:30:28, Julien Tinnes wrote:
> On 2012/08/06 21:20:58, Jorge Lucangeli Obes wrote:
> > On 2012/08/06 21:17:37, Julien Tinnes wrote:
> > > - This is changing the semantic for stock Linux, from enabled by default
to
> > > disabled by default, isn't it ?
> > > This may be the right choice temporarily, (I'm not too fond of this
> particular
> > > policy being used generically), but this should be documented in the
commit
> > > message. Please also check with/inform cevans@.
> > 
> > Why would this change the default? By default, --disable-gpu-sandbox is not
> > added to the cmdline, so on stock Linux we would return |false|.
> 
> But we don't pass --enable-gpu-sandbox by default either, so we would now end
up
> with AllowAll where we previously had the real policy on stock Linux, no ?
> 
> > > - Any chance we could be a bit more aggressive and replace AllowAllPolicy
by
> > > BlackListPtracePolicy ? Since everything can still be disabled via
> > > --disable-gpu-sandbox I don't think this is a big concern.
> > 
> > We're shipping with "--enable-gpu-process" on Chrome OS so we're using
> Blacklist
> > already. My deal with the GPU guys is that no flag == no blocking.
> 
> You mean --enable-gpu-sandbox and "using
> GpuProcessPolicy_x86_64/BlackListPtrace", right? It would be nice for
> BlackListPtrace to become the new "AllowAll", as long as we maintain
"emergency
> shut down switches". But it's your call, we can do that later.

Also as an additional note: using AllowAll is breaking the semantic of
"ShouldEnableSeccompBpf()". We cheat in that we say we enable SeccompBpf, but
then we end-up using a policy that does nothing.

If we want to have an AllowAll policy when ShouldEnableSeccompBpf() returns
false, I would rather have SandboxSeccompBpf::StartSandbox() not query
ShouldEnableSeccompBpf() at all and StartBpfSandbox_x86 return AllowAll if
ShouldEnableSeccompBpf() returns false.

[Jorge Lucangeli Obes, 2012-08-06 22:01:52]

PTAL

[jln (very slow on Chromium), 2012-08-06 22:15:24]

https://chromiumcodereview.appspot.com/10836118/diff/3002/content/common/sand...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/10836118/diff/3002/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:432: if (!IsChromeOS() ||
command_line.HasSwitch(switches::kEnableGpuSandbox))
Do you mind putting the "exceptional case" first?

if (IsChromeOs() && !HasSwitch...) with a comment saying that on ChromeOS
--enable-gpu-sandbox triggers the more advances policy ?

[Jorge Lucangeli Obes, 2012-08-06 22:21:55]

PTAnotherL

[jln (very slow on Chromium), 2012-08-06 22:31:25]

LGTM

[commit-bot: I haz the power, 2012-08-07 17:32:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10836118/13001

[commit-bot: I haz the power, 2012-08-07 19:08:33]

Change committed as 150386