Non-SFI mode: Open primary IPC::Channel before seccomp-sandbox enabled.

Non-SFI mode: Open primary IPC::Channel before seccomp-sandbox enabled.

Currently, the primary IPC::Channel is created in NonSfiListener::Listen().
However, it is run in CLIENT mode so that it requires socketpair() system
call, which we're going to prohibit.
This CL moves the IPC::Channel opening before seccomp-bpf engaged.
Along with the change, we get rid of IPC::SyncChannel for NonSfiListener.
It is because:
- SyncChannel's complicated structure is not necessary here, and
- we cannot create the IOThread required by SyncChannel before seccomp-bpf
  is engaged.

By this Change, IO operation for the primary IPC::Channel in Non-SFI mode
runs on the main thread rather than NaCl_IOThread.

TEST=Ran browser_tests --gtest_filter=*NonSfi* locally. Ran bots.
BUG=358417

[hidehiko, 2015-05-13 12:42:10]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org

[hidehiko, 2015-05-13 12:42:11]

It turned out that, to prohibit socketpair() by seccomp-bpf sandbox, we need to
tweak the primary IPC::Channel creation.

PTAL.

[Mark Seaborn, 2015-05-13 17:40:58]

https://codereview.chromium.org/1133303005/diff/1/components/nacl/loader/nacl...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/1133303005/diff/1/components/nacl/loader/nacl...
components/nacl/loader/nacl_helper_linux.cc:114: // Inside the creation, a
socket pair is created as a dedicated pipe
This is working around some hackery in ipc_channel_posix.cc that is very likely
obsolete.

See IPC_USES_READWRITE in ipc_channel_posix.{h,cc}, which says:

// On Linux, the seccomp sandbox makes it very expensive to call
// recvmsg() and sendmsg(). The restriction on calling read() and write(), which
// are cheap, is that we can't pass file descriptors over them.

This is referring to the old seccomp v1 sandbox
(http://code.google.com/p/seccompsandbox/), where read()/write() were fast but
sendmsg()/recvmsg() had to be intercepted and forwarded and so were slow.

ipc_channel_posix.cc creates a *second* socket for sending FDs so that we can
avoid sendmsg()/recvmsg() unless we know we need to send/receive an FD.

I had no idea that logic was still there and enabled on Linux.  It's probably a
pessimisation these days, since it means that sending/receiving IPC messages
will require two syscalls instead of one.

@jln: We should probably remove IPC_USES_READWRITE and thereby simplify the
code.  What do you think?

[hidehiko, 2015-05-15 12:54:16]

On 2015/05/13 17:40:58, Mark Seaborn wrote:
>
https://codereview.chromium.org/1133303005/diff/1/components/nacl/loader/nacl...
> File components/nacl/loader/nacl_helper_linux.cc (right):
> 
>
https://codereview.chromium.org/1133303005/diff/1/components/nacl/loader/nacl...
> components/nacl/loader/nacl_helper_linux.cc:114: // Inside the creation, a
> socket pair is created as a dedicated pipe
> This is working around some hackery in ipc_channel_posix.cc that is very
likely
> obsolete.
> 
> See IPC_USES_READWRITE in ipc_channel_posix.{h,cc}, which says:
> 
> // On Linux, the seccomp sandbox makes it very expensive to call
> // recvmsg() and sendmsg(). The restriction on calling read() and write(),
which
> // are cheap, is that we can't pass file descriptors over them.
> 
> This is referring to the old seccomp v1 sandbox
> (http://code.google.com/p/seccompsandbox/), where read()/write() were fast but
> sendmsg()/recvmsg() had to be intercepted and forwarded and so were slow.
> 
> ipc_channel_posix.cc creates a *second* socket for sending FDs so that we can
> avoid sendmsg()/recvmsg() unless we know we need to send/receive an FD.
> 
> I had no idea that logic was still there and enabled on Linux.  It's probably
a
> pessimisation these days, since it means that sending/receiving IPC messages
> will require two syscalls instead of one.
> 
> @jln: We should probably remove IPC_USES_READWRITE and thereby simplify the
> code.  What do you think?

jln@, ping?

[Mark Seaborn, 2015-05-15 15:57:52]

On 15 May 2015 at 05:54, <hidehiko@chromium.org> wrote:

> @jln: We should probably remove IPC_USES_READWRITE and thereby simplify the
>> code.  What do you think?
>>
>
> jln@, ping?


Hidehiko, I would suggest that you prepare a change that removes
IPC_USES_READWRITE, since I'm fairly sure that removing it is the right
thing to do.  The only problems with doing that are likely to be ones that
are hard to anticipate anyway (e.g. accidental performance side effects).

Presumably removing IPC_USES_READWRITE would land after the branch at this
point?  So the change would get a while to be tested.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2015-05-15 23:32:12]

I'd support getting rid of IPC_USES_READWRITE, and I expect jln@ would too.  I
hadn't realized that the performance comments were referring to the old sandbox.

[jln (very slow on Chromium), 2015-05-15 23:43:30]

On 2015/05/15 23:32:12, mdempsky wrote:
> I'd support getting rid of IPC_USES_READWRITE, and I expect jln@ would too.  I
> hadn't realized that the performance comments were referring to the old
sandbox.

Ohh, wow. Good catch Mark. Absolutely agreed, we should get rid of this!

[hidehiko, 2015-05-25 16:11:13]

Abandoning. Thanks to Matthew's work, we do not need this any more.