Split NaCl SFI and non-SFI helpers into separate processes

Split NaCl SFI and non-SFI helpers into separate processes

With this change, NaCl SFI and non-SFI processes now run in disjoint
PID namespaces, so the kernel should prevent sending signals from an SFI
process to a non-SFI process, or vice versa.  (The NaCl PID namespaces
are still nested within the renderer's PID namespace though.)

BUG=364945
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270244

[mdempsky, 2014-05-09 05:39:42]

This CL builds on top of https://codereview.chromium.org/269413004/.  For now,
it just runs two instances of nacl_helper, one for SFI and one for non-SFI.  A
followup CL can change the non-SFI instance to use a newlib-based helper binary.

I was considering making NaClForkDelegate into an abstract class with concrete
subclasses for NaClSFIForkDelegate and NaClNonSFIForkDelegate, or using an enum
instead of the bool, but decided to just go with the simplest solution to start.

Another option is to have components/nacl export a function like

    namespace nacl {
    void AppendZygoteForkDelegates(
        ScopedVector<ZygoteForkDelegate>* delegates);
    }

that takes care of instantiates the appropriate subclasses, and then we can more
easily iterate on details without pestering chrome/app OWNERS.

Let me know what you guys think.

[jln (very slow on Chromium), 2014-05-09 20:51:19]

This lgtm. It's pretty magic that it "just works" :)

There are a few things that can be simplified / fixed after this lands. For
instance we should be able to close the SandboxIPCChannel earlier in nonsfi_mode
(Mark could confirm if we don't need it for something else).

The commit message should mention the fact that SFI NaCl and non SFI NaCl will
now run in separate namespaces and be isolated from each other.

https://chromiumcodereview.appspot.com/279693002/diff/1/chrome/app/chrome_mai...
File chrome/app/chrome_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/279693002/diff/1/chrome/app/chrome_mai...
chrome/app/chrome_main_delegate.cc:861: delegates->push_back(new
NaClForkDelegate(false));
Add /* non_sfi */ comments next to false and true?

https://chromiumcodereview.appspot.com/279693002/diff/1/components/nacl/zygot...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://chromiumcodereview.appspot.com/279693002/diff/1/components/nacl/zygot...
components/nacl/zygote/nacl_fork_delegate_linux.cc:283: if (process_type !=
(nonsfi_mode_ ? switches::kNaClLoaderNonSfiProcess
This is hard to read.

Maybe have an intermediate can_help_with_process_type variable?

I wonder if you shouldn't just save that information in the constructor,
depending on nonsfi_mode.

[mdempsky, 2014-05-09 21:34:11]

On 2014/05/09 20:51:19, jln wrote:
> This lgtm. It's pretty magic that it "just works" :)

Agreed. :)

> There are a few things that can be simplified / fixed after this lands. For
> instance we should be able to close the SandboxIPCChannel earlier in
nonsfi_mode
> (Mark could confirm if we don't need it for something else).

Acknowledged.

> The commit message should mention the fact that SFI NaCl and non SFI NaCl will
> now run in separate namespaces and be isolated from each other.

Done.

https://codereview.chromium.org/279693002/diff/1/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/279693002/diff/1/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:861: delegates->push_back(new
NaClForkDelegate(false));
On 2014/05/09 20:51:20, jln wrote:
> Add /* non_sfi */ comments next to false and true?

Done.

https://codereview.chromium.org/279693002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:283: if (process_type !=
(nonsfi_mode_ ? switches::kNaClLoaderNonSfiProcess
On 2014/05/09 20:51:20, jln wrote:
> This is hard to read.
> 
> Maybe have an intermediate can_help_with_process_type variable?

Done.  (I named it helpable_process_type.)

> I wonder if you shouldn't just save that information in the constructor,
> depending on nonsfi_mode.

I think this way is a bit cleaner (no redundant state to keep track of), but I
don't feel strongly.  Long term, I expect CanHelp() to go away when we get rid
of fork delegates and instead have multiple full zygotes.

[Mark Seaborn, 2014-05-09 22:44:47]

https://codereview.chromium.org/279693002/diff/20001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/279693002/diff/20001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:862: delegates->push_back(new
NaClForkDelegate(true /* nonsfi_mode */));
This will launch an extra process at browser startup on machines that don't need
it.

It would be nice to avoid that overhead.  Can we make this conditional, please,
so that it only happens if:

defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)
OR
cmd_line->HasSwitch(switches::kEnableNaClNonSfiMode)

-- following the conditionals Elijah is adding in
https://codereview.chromium.org/264923011/.  (Specifically in
https://codereview.chromium.org/264923011/diff/170001/components/nacl/browser...)

[jln (very slow on Chromium), 2014-05-09 22:59:28]

https://codereview.chromium.org/279693002/diff/20001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/279693002/diff/20001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:862: delegates->push_back(new
NaClForkDelegate(true /* nonsfi_mode */));
On 2014/05/09 22:44:48, Mark Seaborn wrote:
> This will launch an extra process at browser startup on machines that don't
need
> it.

Good point, but let's make sure that we don't duplicate this logic (which should
live in components/nacl/, once).

[mdempsky, 2014-05-09 23:49:35]

https://codereview.chromium.org/279693002/diff/20001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/279693002/diff/20001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:862: delegates->push_back(new
NaClForkDelegate(true /* nonsfi_mode */));
On 2014/05/09 22:44:48, Mark Seaborn wrote:
> This will launch an extra process at browser startup on machines that don't
need
> it.

Mark and I chatted about this over IM, and I believe the new patch set (#4)
addresses his concerns.

I still unconditionally add the non-SFI delegate, but now I avoid launching the
helper process in Init() unless its actually needed/enabled.  The main upside
(IMO) to this is if components/nacl erroneously tries to launch a non-sfi
process, we won't fallback to the default zygote fork() logic.

I also went ahead and moved this code into components/nacl/app so we can easily
iterate on it in the future without needing to pester any chrome/app OWNERS.

[Mark Seaborn, 2014-05-10 00:10:40]

https://codereview.chromium.org/279693002/diff/60001/components/nacl/app/nacl...
File components/nacl/app/nacl_fork_delegates_linux.cc (right):

https://codereview.chromium.org/279693002/diff/60001/components/nacl/app/nacl...
components/nacl/app/nacl_fork_delegates_linux.cc:12: void
AddNaClZygoteForkDelegates(
I think this belongs in components/nacl/zygote/nacl_fork_delegate_linux.cc.

It runs in the zygote process, so it belongs in components/nacl/zygote/, and it
may as well live in the existing file.  (ZygoteStarting() gets called from
RunZygote() inside content/.)

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:47: #if defined(OS_CHROMEOS)
|| defined(ARCH_CPU_ARMEL)
I think Julien wanted to de-duplicate this condition, so once Elijah's change is
committed, maybe you could rebase and move the definition of this constant to a
header file?

[jln (very slow on Chromium), 2014-05-10 00:44:50]

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:47: #if defined(OS_CHROMEOS)
|| defined(ARCH_CPU_ARMEL)
On 2014/05/10 00:10:40, Mark Seaborn wrote:
> I think Julien wanted to de-duplicate this condition, so once Elijah's change
is
> committed, maybe you could rebase and move the definition of this constant to
a
> header file?

We could even make it a function that also queries the command line and return
whether or not to enable nonsfi. WDYT?

[mdempsky, 2014-05-10 01:58:52]

https://codereview.chromium.org/279693002/diff/60001/components/nacl/app/nacl...
File components/nacl/app/nacl_fork_delegates_linux.cc (right):

https://codereview.chromium.org/279693002/diff/60001/components/nacl/app/nacl...
components/nacl/app/nacl_fork_delegates_linux.cc:12: void
AddNaClZygoteForkDelegates(
On 2014/05/10 00:10:40, Mark Seaborn wrote:
> I think this belongs in components/nacl/zygote/nacl_fork_delegate_linux.cc.
> 
> It runs in the zygote process, so it belongs in components/nacl/zygote/, and
it
> may as well live in the existing file.  (ZygoteStarting() gets called from
> RunZygote() inside content/.)

Done.

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:47: #if defined(OS_CHROMEOS)
|| defined(ARCH_CPU_ARMEL)
On 2014/05/10 00:10:40, Mark Seaborn wrote:
> I think Julien wanted to de-duplicate this condition, so once Elijah's change
is
> committed, maybe you could rebase and move the definition of this constant to
a
> header file?

Looking at Elijah's pending CL, it doesn't look straight forward to me to
reconcile, but I can still give it a shot once his change has landed.

In the mean time, I restructured this code a bit to try to make it clearer.

[mdempsky, 2014-05-10 03:01:50]

https://codereview.chromium.org/279693002/diff/100001/components/nacl/zygote/...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/100001/components/nacl/zygote/...
components/nacl/zygote/nacl_fork_delegate_linux.cc:55:
switches::kEnableNaClNonSfiMode);
Argh, this switch isn't passed down to the zygote process currently.  I need to
figure out a way to plumb it down.

[mdempsky, 2014-05-12 22:38:30]

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/60001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:47: #if defined(OS_CHROMEOS)
|| defined(ARCH_CPU_ARMEL)
On 2014/05/10 01:58:52, mdempsky wrote:
> Looking at Elijah's pending CL, it doesn't look straight forward to me to
> reconcile, but I can still give it a shot once his change has landed.

Done: I've moved the IsNonSFIModeEnabled() function from nacl/renderer into
nacl/common, and I'm now also using it in nacl/zygote.

https://codereview.chromium.org/279693002/diff/100001/components/nacl/zygote/...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/279693002/diff/100001/components/nacl/zygote/...
components/nacl/zygote/nacl_fork_delegate_linux.cc:55:
switches::kEnableNaClNonSfiMode);
On 2014/05/10 03:01:50, mdempsky wrote:
> Argh, this switch isn't passed down to the zygote process currently.  I need
to
> figure out a way to plumb it down.

Done.

[Mark Seaborn, 2014-05-12 22:49:26]

LGTM, thanks

https://codereview.chromium.org/279693002/diff/140001/components/nacl/common/...
File components/nacl/common/nacl_nonsfi_util.h (right):

https://codereview.chromium.org/279693002/diff/140001/components/nacl/common/...
components/nacl/common/nacl_nonsfi_util.h:10: // Returns true if non-SFI mode
*can* run for a given platform and if non-SFI
Nit: maybe "current platform"?  (Since "given" suggests it's an argument.)

https://codereview.chromium.org/279693002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/279693002/diff/140001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:528: return
IsNonSFIModeEnabled() ? PP_TRUE : PP_FALSE;
Maybe use PP_FromBool as the original code did?

[mdempsky, 2014-05-12 22:54:24]

https://codereview.chromium.org/279693002/diff/140001/components/nacl/common/...
File components/nacl/common/nacl_nonsfi_util.h (right):

https://codereview.chromium.org/279693002/diff/140001/components/nacl/common/...
components/nacl/common/nacl_nonsfi_util.h:10: // Returns true if non-SFI mode
*can* run for a given platform and if non-SFI
On 2014/05/12 22:49:26, Mark Seaborn wrote:
> Nit: maybe "current platform"?  (Since "given" suggests it's an argument.)

Done.

https://codereview.chromium.org/279693002/diff/140001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/279693002/diff/140001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:528: return
IsNonSFIModeEnabled() ? PP_TRUE : PP_FALSE;
On 2014/05/12 22:49:26, Mark Seaborn wrote:
> Maybe use PP_FromBool as the original code did?

Done.

[mdempsky, 2014-05-12 22:55:44]

cpu: Please review tiny change to chrome/app/chrome_main_delegate.cc for OWNERS
approval. Thanks!

[cpu_(ooo_6.6-7.5), 2014-05-13 19:49:53]

lgtm

[mdempsky, 2014-05-13 20:05:56]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-05-13 20:06:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mdempsky@chromium.org/279693002/160001

[mdempsky, 2014-05-13 23:20:29]

The CQ bit was unchecked by mdempsky@chromium.org

[mdempsky, 2014-05-13 23:20:30]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2014-05-13 23:21:17]

On 2014/05/13 23:20:30, mdempsky wrote:
> The CQ bit was checked by mailto:mdempsky@chromium.org

Using NOTRY=true because all relevant bots are green, mac_chromium_rel is going
to take 2 hours to finish, and android_aosp is broken.

[commit-bot: I haz the power, 2014-05-13 23:25:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mdempsky@chromium.org/279693002/160001

[commit-bot: I haz the power, 2014-05-13 23:33:01]

Change committed as 270244