Linux: Adjust /proc/pid/oom_adj to sacrifice plugin and renderer processes to...

Linux: Adjust /proc/pid/oom_adj to sacrifice plugin and renderer processes to the OOM killer.

BUG=29752
TEST=During out of memory conditions, Linux kernel picks a plugin/renderer over the browser process.

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

[Lei Zhang, 2009-12-09 02:05:44]

The scores are a bit arbitrary, let me know if you think they should be
adjusted.

The OOM still picks innocent renderers quite often, leaving a trail of sad tabs,
but the browser process survives.

[agl, 2009-12-09 02:10:16]

http://codereview.chromium.org/467058/diff/1/2
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/467058/diff/1/2#newcode260
chrome/app/chrome_dll_main.cc:260: if (process_type == switches::kZygoteProcess
||
Zapping the Zygote is about as bad as zapping the browser.

http://codereview.chromium.org/467058/diff/1/2#newcode262
chrome/app/chrome_dll_main.cc:262: process_type == switches::kRendererProcess) {
This won't actually catch the renderer processes. Renderer processes are Zygote
processes at this point. See line 683.

[Lei Zhang, 2009-12-09 08:36:49]

On 2009/12/09 02:10:16, agl wrote:
> http://codereview.chromium.org/467058/diff/1/2
> File chrome/app/chrome_dll_main.cc (right):
> 
> http://codereview.chromium.org/467058/diff/1/2#newcode260
> chrome/app/chrome_dll_main.cc:260: if (process_type ==
switches::kZygoteProcess
> ||
> Zapping the Zygote is about as bad as zapping the browser.

Setting oom_adj for the renderers but not the zygote in the suid sandbox
requires a bit more ugliness. Sigh.

> http://codereview.chromium.org/467058/diff/1/2#newcode262
> chrome/app/chrome_dll_main.cc:262: process_type == switches::kRendererProcess)
{
> This won't actually catch the renderer processes. Renderer processes are
Zygote
> processes at this point. See line 683.

Same for extensions.

[agl, 2009-12-09 16:03:40]

On Wed, Dec 9, 2009 at 12:36 AM,  <thestig@chromium.org> wrote:
> Setting oom_adj for the renderers but not the zygote in the suid sandbox
> requires a bit more ugliness. Sigh.

You can totally punt on this to keep things simple. Just throw in a
comment and LGTM.


AGL

[Evan Martin, 2009-12-09 16:32:32]

If a renderer starts eating memory I won't the OOM killer will go after the
zygote (as the head of the process tree) first?

[agl, 2009-12-09 17:52:01]

On Wed, Dec 9, 2009 at 8:32 AM,  <evan@chromium.org> wrote:
> If a renderer starts eating memory I won't the OOM killer will go after the
> zygote (as the head of the process tree) first?

There's a decay factor of 0.5, so hopefully not.


AGL

[Lei Zhang, 2009-12-09 20:39:58]

On 2009/12/09 16:32:32, Evan Martin wrote:
> If a renderer starts eating memory I won't the OOM killer will go after the
> zygote (as the head of the process tree) first?

Unfortunately I see this happening. I'd prefer not to punt on this even though
it's a bit ugly. Patch set 3 checks to make sure the process we're adjusting
belongs to the right user.

[agl, 2009-12-09 21:19:14]

LGTM.

The SUID sandbox MUST DIE! (soon)

http://codereview.chromium.org/467058/diff/7001/7005
File sandbox/linux/suid/process_util_linux.c (right):

http://codereview.chromium.org/467058/diff/7001/7005#newcode23
sandbox/linux/suid/process_util_linux.c:23: char oom_adj[PATH_MAX];
Rather than use PATH_MAX here, it would be clearer if we had 6 /* "/proc/" */ +
20 /* log_2(2**64) */ + 9 /* "/oom_adj\0" */

http://codereview.chromium.org/467058/diff/7001/7005#newcode29
sandbox/linux/suid/process_util_linux.c:29: if (getuid() != statbuf.st_uid)
If one was being really careful one would open the directory, check the UID and
use openat to get the oom_adj. However, it's actually not worth it in this case.