Closed Bug 1553850 Opened 6 years ago Closed 4 months ago

Consider enabling sandboxing on pgo profiling runs

Categories

(Firefox Build System :: General, enhancement, P3)

enhancement

Tracking

(firefox137 fixed)

RESOLVED FIXED
137 Branch
Tracking Status
firefox137 --- fixed

People

(Reporter: Gijs, Assigned: gerard-majax)

References

Details

(Keywords: in-triage)

Attachments

(6 files)

In bug 1196094 I'm adding support for e10s being enabled during profiling runs. Unfortunately, sandboxing interferes with this (at least locally, the profiling run hangs even just on the run that only opens the data: URI with a quitter call), presumably because the instrumentation code is unhappy when writing to the profile output files fails. I don't really know what the best solution for this is, the simplest seemed to just disable sandboxing while profiling. It's possible this is in fact also the best solution (that is, adding profiling-only product code seems counterproductive in terms of profiling being as close to "real world" as possible), I'm not entirely sure, but I wanted to get this on file so people who know about this can offer opinions/advice...

Adding cc for people on the relevant platforms. I assume this is the sandboxing code prohibiting access on the profile file writes, but maybe there's something more insidious here?

Flags: needinfo?(jld)
Flags: needinfo?(haftandilian)
Flags: needinfo?(bobowencode)

Where does it try to write the profiling data and how much control do we have over it?

Flags: needinfo?(bobowencode)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

Ideally, there'd be a way to get the profiling runtime to open the files in early startup (which may not be a complete answer on Windows, because the process can be started with different permissions from normal, but I don't know all the details there).

Failing that, we'd have to make PGO-specific changes to the sandbox policies. Naively I'd think that that would be better than disabling sandboxing, because we'd still get profiling information on the sandbox code itself — on Linux and Windows we transparently intercept and broker some operations with code that's in our tree and could possibly benefit from PGO — and the changes should be relatively small.

Flags: needinfo?(jld)

(In reply to Nathan Froyd [:froydnj] from comment #3)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

We could add the path specified in the env var to the rules on Windows and other platforms I think.
We do a similar thing for MOZ_LOG on Windows.
(Ideally, eventually, we'd like MOZ_LOG and other logging to pass through FDs/HANDLEs to the process, where we have control of things, but I guess for this we wouldn't be able to do that.)

Type: defect → enhancement
Priority: -- → P3

(In reply to Bob Owen (:bobowen) from comment #5)

(In reply to Nathan Froyd [:froydnj] from comment #3)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

We could add the path specified in the env var to the rules on Windows and other platforms I think.
We do a similar thing for MOZ_LOG on Windows.
(Ideally, eventually, we'd like MOZ_LOG and other logging to pass through FDs/HANDLEs to the process, where we have control of things, but I guess for this we wouldn't be able to do that.)

Is that something that can be wrapped in an ifdef MOZ_PROFILE_GENERATE? I think changing when the files get opened would be difficult, and would require us to patch llvm.

(In reply to Michael Shal [:mshal] from comment #6)

(In reply to Bob Owen (:bobowen) from comment #5)

(In reply to Nathan Froyd [:froydnj] from comment #3)

The data is written from inside LLVM's runtime libraries. We can specify an LLVM_PROFILE_FILE environment variable for the filename(s), which would let us control the directory, I suppose?

We could add the path specified in the env var to the rules on Windows and other platforms I think.
We do a similar thing for MOZ_LOG on Windows.
(Ideally, eventually, we'd like MOZ_LOG and other logging to pass through FDs/HANDLEs to the process, where we have control of things, but I guess for this we wouldn't be able to do that.)

Is that something that can be wrapped in an ifdef MOZ_PROFILE_GENERATE? I think changing when the files get opened would be difficult, and would require us to patch llvm.

If this has its own build, I think that opens up more options.
We could either use LLVM_PROFILE_FILE (if this already exists) or if we know where the files are created we could just open up that folder (this could be controlled by our own env var if we like, in case it needs to changed).
Either way we can put this all in an ifdef MOZ_PROFILE_GENERATE, so the normal build isn't affected.

What Bob wrote in comment 7 applies to Mac as well. If profiling runs have MOZ_PROFILE_GENERATE defined, it should be starightforward to whitelist LLVM_PROFILE_FILE in the Mac sandbox rules without affecting normal builds.

Flags: needinfo?(haftandilian)
See Also: → 1417071

Adding needinfo for myself to test Mac PGO build locally and debug sandboxing issue.

Flags: needinfo?(haftandilian)
See Also: → 1663424
See Also: → 1663700

(In reply to Bob Owen (:bobowen) from comment #7)

If this has its own build, I think that opens up more options.
We could either use LLVM_PROFILE_FILE (if this already exists) or if we know where the files are created we could just open up that folder (this could be controlled by our own env var if we like, in case it needs to changed).
Either way we can put this all in an ifdef MOZ_PROFILE_GENERATE, so the normal build isn't affected.

LLVM_PROFILE_FILE has substitution bits in it that get replaced by bits we don't control - can we use wildcards in the things we allow through the sandbox? And could you potentially link me to where we'd make such a change (to apply to all our sandboxed child processes) ? I'd like to work on this considering what we ran into in bug 1663424.

Flags: needinfo?(bobowencode)

IIUC, LLVM_PROFILE_FILE doesn't have to be strictly a filename, it could contain a path to a directory that we use just for profiles, and then open up that directory.

Embarrassing edit: we already do this. :-) https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/testing/mozharness/scripts/android_emulator_pgo.py#199-207

(In reply to :dmajor from comment #11)

IIUC, LLVM_PROFILE_FILE doesn't have to be strictly a filename, it could contain a path to a directory that we use just for profiles, and then open up that directory.

Embarrassing edit: we already do this. :-) https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/testing/mozharness/scripts/android_emulator_pgo.py#199-207

Ah, hm, we do on Android but not on desktop - https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/build/pgo/profileserver.py#132 . But that makes sense, I guess we can make it some directory, update the code collecting them, and safelist the directory for sandbox access. Still, not sure what the best place is to do that... :-)

Ah, right, I was getting confused with the other bug from today, and thought you were only talking about Android.

(In reply to :Gijs (he/him) from comment #10)

(In reply to Bob Owen (:bobowen) from comment #7)

If this has its own build, I think that opens up more options.
We could either use LLVM_PROFILE_FILE (if this already exists) or if we know where the files are created we could just open up that folder (this could be controlled by our own env var if we like, in case it needs to changed).
Either way we can put this all in an ifdef MOZ_PROFILE_GENERATE, so the normal build isn't affected.

LLVM_PROFILE_FILE has substitution bits in it that get replaced by bits we don't control - can we use wildcards in the things we allow through the sandbox? And could you potentially link me to where we'd make such a change (to apply to all our sandboxed child processes) ? I'd like to work on this considering what we ran into in bug 1663424.

We certainly do on Windows pattern rules are here [1], I seem to remember it can't normally start with a wildcard either.
If we wanted to add the same rule for all sandboxed child processes then in SandboxBroker::LaunchApp [2] is probably best.
We already have rules for MOZ_LOGging and DEBUG access to %TEMP% for refcount logging IIRC.

I think we do for Linux and Mac, but jld and haik would be the best people to give details.

[1] https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/security/sandbox/chromium/sandbox/win/src/sandbox_policy.h#229
[2] https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#241

Flags: needinfo?(jld)
Flags: needinfo?(haftandilian)
Flags: needinfo?(bobowencode)
Flags: needinfo?(haftandilian)

For Mac, we could change StartMacSandbox() to pass a directory or file path to each sandbox profile that needs this. This would be similar to how the HOME environment variable is passed[1] to sandbox profiles[2] now via the HOME_PATH param. Then in the policy we could add a new rule to allow read or write access to the path provided in the new param. Regexes are possible. Since this has its own build, the only security consideration would be to make sure the new rules could only be used for MOZ_PROFILE_GENERATE builds.

  1. https://searchfox.org/mozilla-central/rev/8a0745cd346f0cfb89ae71690babbf7bff706113/security/sandbox/mac/Sandbox.mm#297
  2. https://searchfox.org/mozilla-central/rev/8a0745cd346f0cfb89ae71690babbf7bff706113/security/sandbox/mac/SandboxPolicyContent.h#25
Flags: needinfo?(haftandilian)

For Linux, this is in SandboxBrokerPolicyFactor.cpp and it sounds like this will need something with the AddPrefix method, like what we do for XPCOM_MEM_BLOAT_LOG.

Flags: needinfo?(jld)
Severity: normal → S3
See Also: → 1908630

As a by product of bug 1908630 I have been able to get something together that should allow at least on Linux running with the sandbox enabled

(In reply to :gerard-majax from comment #17)

As a by product of bug 1908630 I have been able to get something together that should allow at least on Linux running with the sandbox enabled

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=a2eaf732ab81c606c502dd26d898dc4fd5c29c57&newProject=try&newRevision=4d41a104b0e794326faa78ff51710d098ad5bbb5

Depends on: 1909421

Comment on attachment 9413897 [details]
Bug 1553850 - Add LLVM profiling to Linux Sandbox r=gcp,afinder

Revision D217167 was moved to bug 1909421. Setting attachment 9413897 [details] to obsolete.

Attachment #9413897 - Attachment is obsolete: true
Assignee: nobody → lissyx+mozillians
Attachment #9413897 - Attachment description: WIP: Bug 1553850 - Add LLVM profiling to Linux Sandbox → Bug 1553850 - Add LLVM profiling to Linux Sandbox r?#perftest-reviewers!,gcp!
Attachment #9413897 - Attachment is obsolete: false
Attachment #9417259 - Attachment description: WIP: Bug 1553850 - Use sandbox duing profiling step → WIP: Bug 1553850 - Enable use of sandbox during profiling step
Attachment #9417259 - Attachment description: WIP: Bug 1553850 - Enable use of sandbox during profiling step → Bug 1553850 - Enable use of sandbox during profiling step r?#perftest-reviewers!
Attachment #9417260 - Attachment description: WIP: Bug 1553850 - Add LLVM_PROFILE_FILE directory computation → Bug 1553850 - Add LLVM_PROFILE_FILE directory computation r?bobowen!,haik!
Attachment #9417261 - Attachment description: WIP: Bug 1553850 - Add LLVM profiling to macOS Sandbox → Bug 1553850 - Add LLVM profiling to macOS Sandbox r?haik!
Attachment #9413897 - Attachment description: Bug 1553850 - Add LLVM profiling to Linux Sandbox r?#perftest-reviewers!,gcp! → Bug 1553850 - Add LLVM profiling to Linux Sandbox r?gcp!
Attachment #9417278 - Attachment description: WIP: Bug 1553850 - Add LLVM profiling to Windows Sandbox → Bug 1553850 - Add LLVM profiling to Windows Sandbox r?bobowen!
Pushed by alissy@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7ab954ad623a Enable use of sandbox during profiling step r=perftest-reviewers,afinder,glandium https://hg.mozilla.org/integration/autoland/rev/678defcc87e3 Add LLVM_PROFILE_FILE directory computation r=bobowen,haik https://hg.mozilla.org/integration/autoland/rev/516100dff406 Add LLVM profiling to macOS Sandbox r=haik https://hg.mozilla.org/integration/autoland/rev/ab269021ad5b Add LLVM profiling to Linux Sandbox r=gcp,perftest-reviewers,afinder https://hg.mozilla.org/integration/autoland/rev/ebe866c9c2b1 Add LLVM profiling to Windows Sandbox r=bobowen
Regressions: 1914115
Regressions: 1914428
Regressions: 1914446
Backout by tszentpeteri@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1ceadc1e3298 Backed out 4 changesets as requested by Gerard for causing snap canonical builds bustages. CLOSED TREE

Backed out as requested by Gerard for causing snap canonical builds bustages.

Status: RESOLVED → REOPENED
Flags: needinfo?(lissyx+mozillians)
Resolution: FIXED → ---
Target Milestone: 131 Branch → ---
Flags: needinfo?(lissyx+mozillians)
Attachment #9417259 - Attachment description: Bug 1553850 - Enable use of sandbox during profiling step r?#perftest-reviewers! → Bug 1553850 - Enable use of sandbox during profiling step r=afinder,glandium
Attachment #9417260 - Attachment description: Bug 1553850 - Add LLVM_PROFILE_FILE directory computation r?bobowen!,haik! → Bug 1553850 - Add LLVM_PROFILE_FILE directory computation r=bobowen,haik
Attachment #9417261 - Attachment description: Bug 1553850 - Add LLVM profiling to macOS Sandbox r?haik! → Bug 1553850 - Add LLVM profiling to macOS Sandbox r=haik
Attachment #9413897 - Attachment description: Bug 1553850 - Add LLVM profiling to Linux Sandbox r?gcp! → Bug 1553850 - Add LLVM profiling to Linux Sandbox r=gcp,afinder
Attachment #9417278 - Attachment description: Bug 1553850 - Add LLVM profiling to Windows Sandbox r?bobowen! → Bug 1553850 - Add LLVM profiling to Windows Sandbox r=bobowen

I am not sure yet why, but under some circumstances, I get the Allow() of openat syscall to somehow trigger a crash ; it seems to mess with gfx code as per bug 1914446, but it did not happens on CI, nor on my laptop ; it does happen on my desktop and archlinux/snap builds

(In reply to :gerard-majax from comment #31)

I am not sure yet why, but under some circumstances, I get the Allow() of openat syscall to somehow trigger a crash ; it seems to mess with gfx code as per bug 1914446, but it did not happens on CI, nor on my laptop ; it does happen on my desktop and archlinux/snap builds

somehow i dont understand how it could have worked at all before, but we are hitting the fact we are chroot'd, so open() on the fonts fails because those dont exists. We'll have to go through the broker, which was tricky because of llvm profile data writes after the broken was dead at some point

Attachment #9461460 - Attachment description: WIP: Bug 1553850 - Make the SandboxBroker live until the process exits. → Bug 1553850 - Make the SandboxBroker live until the process exits.
Pushed by alissy@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ac18e6bb1835 Enable use of sandbox during profiling step r=perftest-reviewers,afinder,glandium https://hg.mozilla.org/integration/autoland/rev/f7f8df99ad1f Add LLVM_PROFILE_FILE directory computation r=bobowen,haik https://hg.mozilla.org/integration/autoland/rev/99ff1f091b19 Make the SandboxBroker live until the process exits. r=necko-reviewers,kershaw,jld https://hg.mozilla.org/integration/autoland/rev/a5f077fc1e6a Add LLVM profiling to macOS Sandbox r=haik https://hg.mozilla.org/integration/autoland/rev/2dd45495e717 Add LLVM profiling to Windows Sandbox r=bobowen https://hg.mozilla.org/integration/autoland/rev/281b3750a7f0 Add LLVM profiling to Linux Sandbox r=perftest-reviewers,afinder,jld
Regressions: 1946312

It caused gtest failures.

[task 2025-02-06T05:15:36.019Z] 05:15:36     INFO -  TEST-START | SandboxBrokerMisc.LeakCheck
[task 2025-02-06T05:15:36.050Z] 05:15:36     INFO -  [914] Sandbox: SandboxBroker: thread creation failed: ENOMEM
[task 2025-02-06T05:15:36.050Z] 05:15:36  WARNING -  TEST-UNEXPECTED-FAIL | SandboxBrokerMisc.LeakCheck | Value of: broker != nullptr
[task 2025-02-06T05:15:36.051Z] 05:15:36     INFO -    Actual: false
[task 2025-02-06T05:15:36.052Z] 05:15:36     INFO -  Expected: true
[task 2025-02-06T05:15:36.052Z] 05:15:36     INFO -   @ /builds/worker/checkouts/gecko/security/sandbox/linux/gtest/TestBroker.cpp:681
[task 2025-02-06T05:15:36.053Z] 05:15:36  WARNING -  TEST-UNEXPECTED-FAIL | SandboxBrokerMisc.LeakCheck | test completed (time: 31ms)
[task 2025-02-06T05:15:36.053Z] 05:15:36     INFO -  TEST-START | SandboxBrokerPolicyLookup.Simple
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(lissyx+mozillians)

Backout merged into central.

Reopened as per comment #37.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by alissy@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/982a5ff9aad6 Enable use of sandbox during profiling step r=perftest-reviewers,afinder,glandium https://hg.mozilla.org/integration/autoland/rev/96b6558dff15 Add LLVM_PROFILE_FILE directory computation r=bobowen,haik https://hg.mozilla.org/integration/autoland/rev/68cd156cda7c Make the SandboxBroker live until the process exits. r=necko-reviewers,kershaw,jld https://hg.mozilla.org/integration/autoland/rev/06e90b560e09 Add LLVM profiling to macOS Sandbox r=haik https://hg.mozilla.org/integration/autoland/rev/4847bd33e612 Add LLVM profiling to Windows Sandbox r=bobowen https://hg.mozilla.org/integration/autoland/rev/7d44ca7b4af4 Add LLVM profiling to Linux Sandbox r=perftest-reviewers,afinder,jld https://hg.mozilla.org/integration/autoland/rev/595c92211401 apply code formatting via Lando
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: