search

netblue30 / firejail

Linux namespaces and seccomp-bpf sandbox
https://firejail.wordpress.com
GNU General Public License v2.0
5.83k stars 568 forks source link

bugfix: fix startup race condition for /run/firejail directory #6307

Closed spiiroin closed 7 months ago

spiiroin commented 7 months ago

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 commented 7 months ago

From Saifish OS patch here: https://github.com/sailfishos/firejail/pull/18

topimiettinen commented 7 months ago

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?

spiiroin commented 7 months ago

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.

spiiroin commented 7 months ago

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).

kmk3 commented 7 months ago

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.