Add env var for GPU process sandbox.

Add env var for GPU process sandbox.

Since the seccomp filter GPU sandbox blocks access to the file system,
we need to set an env var with the config line found in /etc/drirc on
Chrome OS. This allows games that use S3TC to work inside the sandbox.

BUG=127664
TEST=Bastion works on Chrome OS with '--enable-gpu-sandbox'.


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

[Jorge Lucangeli Obes, 2012-07-10 18:41:31]

PTAL

cevans for actual change, apatrick for OWNERS stamp.

Thanks!

[Chris Evans, 2012-07-10 18:43:52]

LGTM

(I recommend following up by replacing the ChromeOS /etc/drirc with an XML
comment that says to not add things)

[apatrick_chromium, 2012-07-10 20:24:14]

lgtm

[commit-bot: I haz the power, 2012-07-10 20:31:55]

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

[commit-bot: I haz the power, 2012-07-10 22:15:18]

Try job failure for 10690116-1 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Jorge Lucangeli Obes, 2012-07-10 22:18:33]

On 2012/07/10 22:15:18, I haz the power (commit-bot) wrote:
> Try job failure for 10690116-1 (retry) on win for step "compile" (clobber
> build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Failure looks unrelated, CL is #ifdef OS_CHROMEOS and win_rel is passing.

[commit-bot: I haz the power, 2012-07-10 22:19:32]

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

[commit-bot: I haz the power, 2012-07-10 23:39:56]

Change committed as 145990

[piman, 2012-07-10 23:53:05]

https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:103:
base::Environment::Create()->SetVar("force_s3tc_enable", "true");
Sigh... that file may be used to do per-platform configuration. Is it going to
leak all over the place here instead of in Chrome OS per-board overlay?

Can't we change the driver to make sure that it reads that file early (before we
turn on the sandbox) instead?

[Jorge Lucangeli Obes, 2012-07-10 23:58:06]

On 2012/07/10 23:53:05, piman wrote:
> https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc
> File content/gpu/gpu_main.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.c...
> content/gpu/gpu_main.cc:103:
> base::Environment::Create()->SetVar("force_s3tc_enable", "true");
> Sigh... that file may be used to do per-platform configuration. Is it going to
> leak all over the place here instead of in Chrome OS per-board overlay?
> 
> Can't we change the driver to make sure that it reads that file early (before
we
> turn on the sandbox) instead?

I tried making the change outside of Chrome. The problem is that:

src/third_party/mesa/MesaLib/src/mesa/drivers/dri/i915/intel_context.c:

GLboolean
intelInitContext(struct intel_context *intel,
                 int api,
                 const __GLcontextModes * mesaVis,
                 __DRIcontext * driContextPriv,
                 void *sharedContextPrivate,
                 struct dd_function_table *functions)

Always calls open() to read /etc/drirc, and does not have any caching mechanism
whatsoever. Every time a new context is created, we do the call to open(); we
create contexts after the sandbox is on.

[piman, 2012-07-11 00:28:59]

On Tue, Jul 10, 2012 at 4:58 PM, <jorgelo@chromium.org> wrote:

> On 2012/07/10 23:53:05, piman wrote:
>
>> https://chromiumcodereview.**appspot.com/10690116/diff/1/**
>>
content/gpu/gpu_main.cc<https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc>
>> File content/gpu/gpu_main.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10690116/diff/1/**
>
content/gpu/gpu_main.cc#**newcode103<https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc#newcode103>
>
>> content/gpu/gpu_main.cc:103:
>> base::Environment::Create()->**SetVar("force_s3tc_enable", "true");
>> Sigh... that file may be used to do per-platform configuration. Is it
>> going to
>> leak all over the place here instead of in Chrome OS per-board overlay?
>>
>
>  Can't we change the driver to make sure that it reads that file early
>> (before
>>
> we
>
>> turn on the sandbox) instead?
>>
>
> I tried making the change outside of Chrome. The problem is that:
>
> src/third_party/mesa/MesaLib/**src/mesa/drivers/dri/i915/**
> intel_context.c:
>
> GLboolean
> intelInitContext(struct intel_context *intel,
>                  int api,
>                  const __GLcontextModes * mesaVis,
>                  __DRIcontext * driContextPriv,
>                  void *sharedContextPrivate,
>                  struct dd_function_table *functions)
>
> Always calls open() to read /etc/drirc, and does not have any caching
> mechanism
> whatsoever. Every time a new context is created, we do the call to open();
> we
> create contexts after the sandbox is on.
>

I think it wouldn't be too hard to move the parsing to intelInitScreen2,
which should get called before the sandbox is on.

Antoine


>
https://chromiumcodereview.**appspot.com/10690116/<https://chromiumcodereview...
>

[Jorge Lucangeli Obes, 2012-07-11 01:02:54]

On 2012/07/11 00:28:59, piman wrote:
> On Tue, Jul 10, 2012 at 4:58 PM, <mailto:jorgelo@chromium.org> wrote:
> 
> > On 2012/07/10 23:53:05, piman wrote:
> >
> >> https://chromiumcodereview.**appspot.com/10690116/diff/1/**
> >>
>
content/gpu/gpu_main.cc<https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc>
> >> File content/gpu/gpu_main.cc (right):
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/10690116/diff/1/**
> >
>
content/gpu/gpu_main.cc#**newcode103<https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc#newcode103>
> >
> >> content/gpu/gpu_main.cc:103:
> >> base::Environment::Create()->**SetVar("force_s3tc_enable", "true");
> >> Sigh... that file may be used to do per-platform configuration. Is it
> >> going to
> >> leak all over the place here instead of in Chrome OS per-board overlay?
> >>
> >
> >  Can't we change the driver to make sure that it reads that file early
> >> (before
> >>
> > we
> >
> >> turn on the sandbox) instead?
> >>
> >
> > I tried making the change outside of Chrome. The problem is that:
> >
> > src/third_party/mesa/MesaLib/**src/mesa/drivers/dri/i915/**
> > intel_context.c:
> >
> > GLboolean
> > intelInitContext(struct intel_context *intel,
> >                  int api,
> >                  const __GLcontextModes * mesaVis,
> >                  __DRIcontext * driContextPriv,
> >                  void *sharedContextPrivate,
> >                  struct dd_function_table *functions)
> >
> > Always calls open() to read /etc/drirc, and does not have any caching
> > mechanism
> > whatsoever. Every time a new context is created, we do the call to open();
> > we
> > create contexts after the sandbox is on.
> >
> 
> I think it wouldn't be too hard to move the parsing to intelInitScreen2,
> which should get called before the sandbox is on.

What's the policy for Mesa changes? Is it OK to carry local changes, or is it
frowned upon?

> 
> >
>
https://chromiumcodereview.**appspot.com/10690116/%3Chttps://chromiumcoderevi...>
> >

[piman, 2012-07-11 01:16:06]

On Tue, Jul 10, 2012 at 6:02 PM, <jorgelo@chromium.org> wrote:

> On 2012/07/11 00:28:59, piman wrote:
>
>  On Tue, Jul 10, 2012 at 4:58 PM, <mailto:jorgelo@chromium.org> wrote:
>>
>
>  > On 2012/07/10 23:53:05, piman wrote:
>> >
>> >>
https://chromiumcodereview.**a**ppspot.com/10690116/diff/1/**<http://appspot....
>> >>
>>
>
> content/gpu/gpu_main.cc<https:**//chromiumcodereview.appspot.**
>
com/10690116/diff/1/content/**gpu/gpu_main.cc<https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc>
> >
>
>> >> File content/gpu/gpu_main.cc (right):
>> >>
>> >
>> >
>> >
https://chromiumcodereview.**a**ppspot.com/10690116/diff/1/**<http://appspot....
>> >
>>
>
> content/gpu/gpu_main.cc#****newcode103<https://**
> chromiumcodereview.appspot.**com/10690116/diff/1/content/**
>
gpu/gpu_main.cc#newcode103<https://chromiumcodereview.appspot.com/10690116/diff/1/content/gpu/gpu_main.cc#newcode103>
> >
>
>> >
>> >> content/gpu/gpu_main.cc:103:
>> >> base::Environment::Create()->****SetVar("force_s3tc_enable", "true");
>>
>> >> Sigh... that file may be used to do per-platform configuration. Is it
>> >> going to
>> >> leak all over the place here instead of in Chrome OS per-board overlay?
>> >>
>> >
>> >  Can't we change the driver to make sure that it reads that file early
>> >> (before
>> >>
>> > we
>> >
>> >> turn on the sandbox) instead?
>> >>
>> >
>> > I tried making the change outside of Chrome. The problem is that:
>> >
>> > src/third_party/mesa/MesaLib/****src/mesa/drivers/dri/i915/**
>>
>> > intel_context.c:
>> >
>> > GLboolean
>> > intelInitContext(struct intel_context *intel,
>> >                  int api,
>> >                  const __GLcontextModes * mesaVis,
>> >                  __DRIcontext * driContextPriv,
>> >                  void *sharedContextPrivate,
>> >                  struct dd_function_table *functions)
>> >
>> > Always calls open() to read /etc/drirc, and does not have any caching
>> > mechanism
>> > whatsoever. Every time a new context is created, we do the call to
>> open();
>> > we
>> > create contexts after the sandbox is on.
>> >
>>
>
>  I think it wouldn't be too hard to move the parsing to intelInitScreen2,
>> which should get called before the sandbox is on.
>>
>
> What's the policy for Mesa changes? Is it OK to carry local changes, or is
> it
> frowned upon?
>

We carry some, but it'd be better to upstream if possible.


>
>
>  >
>>
>
> https://chromiumcodereview.**a**ppspot.com/10690116/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/10690116/<http://appspot.com/10690116/%3Chttps://chromiumcodereview.appspot.com/10690116/>
> >
>
>> >
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/10690116/<https://chromiumcodereview...
>