Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modif: populate /run/firejail while holding flock #6307

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

spiiroin
Copy link
Contributor

There are reports of firejail sandboxed applications occasionally taking long time (12 seconds) to start up. When this happens, it affects all sandboxed applications until the device is rebooted.

The reason for the slowdown seems to be a timing hazard in the way remounts under /run/firejail are handled. This gets triggered when multiple firejail processes are launched in parallel as part of user session bring up and results in some, dozens, hundreds, or even thousands of stray /run/firejail/xxx mounts. The amount of mount points then affects every mount operation that is done during sandbox filesystem construction.

To stop this from happening, arrange it so that only one firejail process at time is inspecting and/or modifying mountpoints under /run/firejail by doing:

  1. Create /run/firejail directory using atomic operations
  2. Create and obtain lock for /run/firejail/firejail-run.lock
  3. Setup files, directories and mounts under /run/firejail
  4. Release /run/firejail/firejail-run.lock

@spiiroin
Copy link
Contributor Author

From Saifish OS patch here: sailfishos/firejail#18

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the patch.

@topimiettinen
Copy link
Collaborator

Systemd is using temporary directories with random namse (not predictable and unlike to collide), like systemd-private-f10ec674d0684f66b78990ff81e3153f-auditd.service-jKO6ug/. Perhaps this approach (under /run/firejail/) would be better approach than locking?

src/firejail/preproc.c Outdated Show resolved Hide resolved
@kmk3 kmk3 changed the title Construct /run/firejail while holding flock modif: build /run/firejail while holding flock Apr 13, 2024
@kmk3 kmk3 added this to In progress in Release 0.9.74 via automation Apr 13, 2024
src/firejail/chroot.c Outdated Show resolved Hide resolved
@spiiroin
Copy link
Contributor Author

Systemd is using temporary directories with random namse (not predictable and unlike to collide) ...

AFAIK the difference is that here the directories are meant to be shared and thus must have predictable known names i.e. the problem was not accidental collisions but a technical glitches on the way to the desired end result.

src/firejail/preproc.c Outdated Show resolved Hide resolved
src/firejail/preproc.c Outdated Show resolved Hide resolved
src/firejail/preproc.c Outdated Show resolved Hide resolved
@kmk3 kmk3 changed the title modif: build /run/firejail while holding flock modif: populate /run/firejail while holding flock Apr 17, 2024
Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few more changes and pushed it to a serialize_remounts_v2
branch.

Main changes:

  • Fix misc formatting in the code and commit message
  • Make lock variables global and use them directly only in preproc.c
  • Use the lock/unlock functions also for the network lock (for consistency)
  • Log if the path is already locked/unlocked
  • Split the original commit into one for the flock improvements and another for
    the main fix (to make it easier to understand the scope of the latter)

If it seems good, you can hard-reset to that branch, add the SOB trailer to the
commits if you want and force-push. Let me know if you have any questions.

(By the way, note that git trailers cannot have blank lines between them; a
blank line is what separates the trailers from the rest of the message).

To enable using them outside of src/firejail/main.c.
@spiiroin
Copy link
Contributor Author

If it seems good, you can hard-reset to that branch

Was fine by me, took the branch as-is (plus rebase as there were already new commits in the master branch).

Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks for the improvements.

@kmk3
Copy link
Collaborator

kmk3 commented Apr 24, 2024

If it seems good, you can hard-reset to that branch

Was fine by me, took the branch as-is (plus rebase as there were already new
commits in the master branch).

Misc: On future PRs, please do a separate force-push before or after rebasing
when possible so that a separate timeline event is created on GitHub. This
makes the changes not related to the rebase easier to spot in the main
force-push diff.

spiiroin and others added 2 commits April 25, 2024 09:16
Changes:

* Centralize flock handling in preproc.c
* Add debug and error logging
* Abort if anything fails

Co-authored-by: Kelvin M. Klann <[email protected]>
There are reports of firejail sandboxed applications occasionally
taking a long time (12 seconds) to start up. When this happens, it
affects all sandboxed applications until the device is rebooted.

The reason for the slowdown seems to be a timing hazard in the way
remounts under /run/firejail are handled. This gets triggered when
multiple firejail processes are launched in parallel as part of user
session bring up and results in some, dozens, hundreds, or even
thousands of stray /run/firejail/xxx mounts. The amount of mount
points then affects every mount operation that is done during sandbox
filesystem construction.

To stop this from happening, arrange it so that only one firejail
process at time is inspecting and/or modifying mountpoints under
/run/firejail by doing:

1. Create /run/firejail directory (without locking)
2. Create and obtain a lock for /run/firejail/firejail-run.lock
3. Setup files, directories and mounts under /run/firejail
4. Release /run/firejail/firejail-run.lock
@kmk3 kmk3 merged commit 073a9ef into netblue30:master Apr 25, 2024
13 checks passed
kmk3 added a commit that referenced this pull request Apr 25, 2024
@kmk3 kmk3 moved this from In progress to Done (on RELNOTES) in Release 0.9.74 Apr 25, 2024
@glitsj16 glitsj16 mentioned this pull request May 10, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Release 0.9.74
  
Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants