the webrtc.org 57 import (Bug 1341285) uses libevent to implement rtc::task_queue (very similar to mozilla::TaskQueue, though not identical). libevent uses epoll on linux. epoll is allowed in Chrome (per gcp), and epoll_wait/etc are allowed in Mozilla -- but not epoll_create/epoll_create1. The options are to enable epoll_create/create1 in our sandbox, or re-implement task_queue using TaskQueue.

Enables using libevent on Linux in content

Comment on attachment 8864101 [details] [diff] [review] enable NR_epoll_create/create1 in linux sandbox Review of attachment 8864101 [details] [diff] [review]: ----------------------------------------------------------------- Some questions/remarks. ::: security/sandbox/linux/SandboxFilter.cpp @@ +193,5 @@ > return Allow(); > > // Asynchronous I/O > +#ifdef __NR_epoll_create1 > + case __NR_epoll_create1: You're adding this in SandboxPolicyCommon. That might be reasonable, but you really only need it in content, so it would be better in the more specific ContentSandboxPolicy. That's unless the GMP stuff needs it too, in which case Common is fine. @@ +196,5 @@ > +#ifdef __NR_epoll_create1 > + case __NR_epoll_create1: > +#endif > +#ifdef __NR_epoll_create > + case __NR_epoll_create: Do we support anything that uses this version of the syscall? @@ +198,5 @@ > +#endif > +#ifdef __NR_epoll_create > + case __NR_epoll_create: > +#endif > +#ifdef __NR_epoll_wait We apparently don't need the extra #ifdefs here for the platforms we support.

> You're adding this in SandboxPolicyCommon. That might be reasonable, but you > really only need it in content, so it would be better in the more specific > ContentSandboxPolicy. > > That's unless the GMP stuff needs it too, in which case Common is fine. libevent/epoll is a pretty common thing... but we don't need to have it in GMP at this point. > > @@ +196,5 @@ > > +#ifdef __NR_epoll_create1 > > + case __NR_epoll_create1: > > +#endif > > +#ifdef __NR_epoll_create > > + case __NR_epoll_create: > > Do we support anything that uses this version of the syscall? Libevent uses either depending on which is defined: epoll_create1() was added to the kernel in version 2.6.27. Library support is provided in glibc starting with version 2.9. Since epoll_create won't actually be used by libevent if create1 is around., we could leave it off *if* epoll_create1 is defined. However, other uses of epoll might just use the older one. I'd replace the ifdef with: #if defined(__NR_epoll_create) and !defined(__NR_epoll_create1) > @@ +198,5 @@ > > +#endif > > +#ifdef __NR_epoll_create > > + case __NR_epoll_create: > > +#endif > > +#ifdef __NR_epoll_wait > > We apparently don't need the extra #ifdefs here for the platforms we support. Again, libevent uses either depending on which is defined. Same argument as above.

So... do we have a minimum kernel version? Note that 2.6.27 was released in 2008. (Cf. https://crbug.com/357670, where Chromium decided to stop working around a bug fixed in 2.6.35.)

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #4) > So... do we have a minimum kernel version? Note that 2.6.27 was released in > 2008. (Cf. https://crbug.com/357670, where Chromium decided to stop working > around a bug fixed in 2.6.35.) I don't know that we do. However, in this case I see no reason to not expose epoll_create -- while it's duplicative, it appears to still be in common use. See http://man7.org/linux/man-pages/man2/epoll_create.2.html which notably doesn't say it's deprecated. FYI, RHEL 5.x doesn't have epoll_create1.

Leaving in Common because the existing epoll Allow()s are there, and are prettyuseless without epoll_create/create1. We can move them all if you prefer, but it's a common IO interface

Comment on attachment 8870680 [details] [diff] [review] enable NR_epoll_create/create1 in linux sandbox Review of attachment 8870680 [details] [diff] [review]: ----------------------------------------------------------------- Going to defer to jld here who probably understands offhand why there are epoll calls without epoll_create in the common sandbox code.

Comment on attachment 8870680 [details] [diff] [review] enable NR_epoll_create/create1 in linux sandbox Review of attachment 8870680 [details] [diff] [review]: ----------------------------------------------------------------- There is a reason we didn't need epoll_create* previously: Gecko IPC uses only one epoll instance per process and creates it early in startup, before seccomp-bpf is applied. It does need the syscalls to add/remove events and wait, but it doesn't need to create additional event loops. (For now. That could potentially change in the future, if the I/O thread becomes a bottleneck, but on the other hand there's also bug 1343699 suggesting it switch from libevent to poll(2).) I think this can stay in the Common policy, but I'd suggest changing the commit message to clarify that this is about creating new event bases after sandboxing is started, because as written it's kind of confusing given that libevent is already being used successfully. r=me with the ifdefs removed (see below). ::: security/sandbox/linux/SandboxFilter.cpp @@ +195,5 @@ > // Asynchronous I/O > +#ifdef __NR_epoll_create1 > + case __NR_epoll_create1: > +#endif > +#if defined(__NR_epoll_create) && !defined(__NR_epoll_create1) This won't do what you think — the headers from security/sandbox/chromium/sandbox/linux/system_headers are included here, so __NR_epoll_create1 is always defined and RHEL 5 will break. This is deliberate: usually, we need to allow syscalls even if they don't exist on the build host, because the host it's run on may be newer and thus have libraries that use them. (We also need to *use* newer syscalls than the build host had; for example, official Firefox builds are on CentOS 6, which doesn't even have basic seccomp-bpf.) So, these don't need to be ifdef'ed; the only reason we need to do that is when some architectures never had the syscall. (This file as it exists is not a perfect example of that, I notice, but that's the idea.)

(To nitpick myself slightly: builds hosted on RHEL 5 would potentially be affected, but they'd have to be run on something new enough to have seccomp-bpf. The ifdefs should still be removed.)

Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbdb7d36ee3 enable NR_epoll_create/create1 in linux sandbox r=jld