Fix crash with --use-gl=egl on Linux.

Fix crash with --use-gl=egl on Linux.

eglCreateWindowSurface() in mesa egl driver needs mincore() system call, so
allow gpu process to call mincore().

BUG=423674
Committed: https://crrev.com/43815b018eb71e762657cb650496ea6d83134c31
Cr-Commit-Position: refs/heads/master@{#300445}

[dshwang, 2014-10-15 13:47:25]

dongseong.hwang@intel.com changed reviewers:
+ jln@chromium.org

[dshwang, 2014-10-15 13:47:26]

jln@, could you review?
BUG=423674 needs to use --use-gl=egl commend line but it causes crash. So I fix
it.

[dshwang, 2014-10-15 13:56:25]

dongseong.hwang@intel.com changed reviewers:
+ kalyan@intel.com

[dshwang, 2014-10-15 13:56:25]

I guess link_freon chromebook might need this CL, because link_freon uses mesa
egl driver. +kalyan

[dshwang, 2014-10-17 14:23:54]

dongseong.hwang@intel.com changed reviewers:
+ jorgelo@chromium.org

[dshwang, 2014-10-17 14:24:11]

dongseong.hwang@intel.com changed reviewers:
- jln@chromium.org

[dshwang, 2014-10-17 14:24:25]

dongseong.hwang@intel.com changed reviewers:
+ jln@chromium.org

[dshwang, 2014-10-17 14:24:54]

Jorge, could you review?

[jln (very slow on Chromium), 2014-10-17 18:13:14]

jln@chromium.org changed reviewers:
+ marcheu@chromium.org

[jln (very slow on Chromium), 2014-10-17 18:13:38]

Adding Stéphane for some GPU expertise.

https://codereview.chromium.org/656023003/diff/20001/content/common/sandbox_l...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/656023003/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:161: class
GpuEGLProcessPolicy : public GpuProcessPolicy {
Why make it a new class? If you really need this, please make it a parameter of
an existing class instead. See allow_sysv_shm for an example.

[Jorge Lucangeli Obes, 2014-10-17 18:17:02]

I think Julien's suggestion here makes sense.

[dshwang, 2014-10-17 19:56:40]

On 2014/10/17 18:13:38, jln wrote:
> Adding Stéphane for some GPU expertise.
> 
>
https://codereview.chromium.org/656023003/diff/20001/content/common/sandbox_l...
> File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):
> 
>
https://codereview.chromium.org/656023003/diff/20001/content/common/sandbox_l...
> content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:161: class
> GpuEGLProcessPolicy : public GpuProcessPolicy {
> Why make it a new class? If you really need this, please make it a parameter
of
> an existing class instead. See allow_sysv_shm for an example.

Thank you for review.

There are 2 reasons why I add new class instead of changing GpuProcessPolicy
1. mincore() is needed for only gpu process, not gpu broker process
2. GpuBrokerProcessPolicy, CrosArmGpuProcessPolicy,
CrosArmGpuBrokerProcessPolicy inherit GpuProcessPolicy, but mincore() is not
related to 3 of them.

If you still prefer changing GpuProcessPolicy, I'll update.

[jln (very slow on Chromium), 2014-10-17 20:31:56]

On 2014/10/17 19:56:40, dshwang wrote:
> On 2014/10/17 18:13:38, jln wrote:
> > Adding Stéphane for some GPU expertise.
> > 
> >
>
https://codereview.chromium.org/656023003/diff/20001/content/common/sandbox_l...
> > File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):
> > 
> >
>
https://codereview.chromium.org/656023003/diff/20001/content/common/sandbox_l...
> > content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:161: class
> > GpuEGLProcessPolicy : public GpuProcessPolicy {
> > Why make it a new class? If you really need this, please make it a parameter
> of
> > an existing class instead. See allow_sysv_shm for an example.
> 
> Thank you for review.
> 
> There are 2 reasons why I add new class instead of changing GpuProcessPolicy
> 1. mincore() is needed for only gpu process, not gpu broker process
> 2. GpuBrokerProcessPolicy, CrosArmGpuProcessPolicy,
> CrosArmGpuBrokerProcessPolicy inherit GpuProcessPolicy, but mincore() is not
> related to 3 of them.
> 
> If you still prefer changing GpuProcessPolicy, I'll update.

This seems ok, as long as the derived classes construct GpuProcessPolicy with
allow_mincore set to false.

[dshwang, 2014-10-20 11:26:04]

I update it as Julien's suggestion. Could you review again?

[Jorge Lucangeli Obes, 2014-10-20 18:41:03]

https://codereview.chromium.org/656023003/diff/40001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/656023003/diff/40001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:183: if (allow_mincore_ &&
sysno == __NR_mincore)
Please add this as a case in the switch statement below.

[dshwang, 2014-10-20 18:50:22]

On 2014/10/20 18:41:03, Jorge Lucangeli Obes wrote:
>
https://codereview.chromium.org/656023003/diff/40001/content/common/sandbox_l...
> File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):
> 
>
https://codereview.chromium.org/656023003/diff/40001/content/common/sandbox_l...
> content/common/sandbox_linux/bpf_gpu_policy_linux.cc:183: if (allow_mincore_
&&
> sysno == __NR_mincore)
> Please add this as a case in the switch statement below.

Thanks. DONE.

[Jorge Lucangeli Obes, 2014-10-20 19:01:07]

https://codereview.chromium.org/656023003/diff/60001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/656023003/diff/60001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:208: return Allow();
We try to keep this in alphabetical order, but it's tricky since if we put it
above mmap() it will fall through.

I think the best solution is to do:

case __NR_ioctl:
  return Allow();
case __NR_mincore:
  if (allow_mincore_)
    return Allow();
  else
    return SandboxBPFBasePolicy::EvaluateSyscall(sysno);

And put __NR_mincore right above mmap.

[dshwang, 2014-10-20 19:08:34]

On 2014/10/20 19:01:07, Jorge Lucangeli Obes wrote:
>
https://codereview.chromium.org/656023003/diff/60001/content/common/sandbox_l...
> File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):
> 
>
https://codereview.chromium.org/656023003/diff/60001/content/common/sandbox_l...
> content/common/sandbox_linux/bpf_gpu_policy_linux.cc:208: return Allow();
> We try to keep this in alphabetical order, but it's tricky since if we put it
> above mmap() it will fall through.
> 
> I think the best solution is to do:
> 
> case __NR_ioctl:
>   return Allow();
> case __NR_mincore:
>   if (allow_mincore_)
>     return Allow();
>   else
>     return SandboxBPFBasePolicy::EvaluateSyscall(sysno);
> 
> And put __NR_mincore right above mmap.

DONE.
Ah, thanks. I didn't know alphabetical order belongs to code style.
I did as your instruction as well as put __NR_access to top.

[Jorge Lucangeli Obes, 2014-10-20 19:52:59]

https://codereview.chromium.org/656023003/diff/80001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/656023003/diff/80001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:184: case __NR_access:
While we order things mostly alphabetically, we don't follow the suggestion when
it makes the code more prone to breakage. The reason why __NR_access was below
is because it's got the same behaviour as open and openat, and if we ever change
it we want to change it for the three syscalls. I think it would be better to
keep it below. Sorry about that.

[dshwang, 2014-10-20 20:32:21]

On 2014/10/20 19:52:59, Jorge Lucangeli Obes wrote:
>
https://codereview.chromium.org/656023003/diff/80001/content/common/sandbox_l...
> File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):
> 
>
https://codereview.chromium.org/656023003/diff/80001/content/common/sandbox_l...
> content/common/sandbox_linux/bpf_gpu_policy_linux.cc:184: case __NR_access:
> While we order things mostly alphabetically, we don't follow the suggestion
when
> it makes the code more prone to breakage. The reason why __NR_access was below
> is because it's got the same behaviour as open and openat, and if we ever
change
> it we want to change it for the three syscalls. I think it would be better to
> keep it below. Sorry about that.

Thank you for continuous explanation. Done. :)

[Jorge Lucangeli Obes, 2014-10-20 21:05:43]

lgtm

[dshwang, 2014-10-21 09:28:11]

On 2014/10/20 21:05:43, Jorge Lucangeli Obes wrote:
> lgtm

Thank you for review!

[dshwang, 2014-10-21 09:28:15]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-10-21 09:29:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656023003/100001

[commit-bot: I haz the power, 2014-10-21 10:29:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-21 10:30:36]

Patchset 6 (id:??) landed as
https://crrev.com/43815b018eb71e762657cb650496ea6d83134c31
Cr-Commit-Position: refs/heads/master@{#300445}