-
Notifications
You must be signed in to change notification settings - Fork 4
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[firejail] Construct /run/firejail while holding flock. Fixes JB#52688
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 Signed-off-by: Simo Piiroinen <simo.piiroinen@jolla.com>
- Loading branch information
Showing
4 changed files
with
380 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| From 9ccc64fe71802cf96153285efc00e6c87daa1f3b Mon Sep 17 00:00:00 2001 | ||
| From: "Kelvin M. Klann" <kmk3.code@protonmail.com> | ||
| Date: Wed, 17 Apr 2024 12:56:06 -0300 | ||
| Subject: [PATCH] refactor: make rundir lock variables global | ||
|
|
||
| To enable using them outside of src/firejail/main.c. | ||
| --- | ||
| src/firejail/firejail.h | 2 ++ | ||
| src/firejail/main.c | 4 ++-- | ||
| 2 files changed, 4 insertions(+), 2 deletions(-) | ||
|
|
||
| diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h | ||
| index 13ee573ad..1a8bf1fa9 100644 | ||
| --- a/src/firejail/firejail.h | ||
| +++ b/src/firejail/firejail.h | ||
| @@ -269,6 +269,8 @@ static inline int any_dhcp(void) { | ||
| return any_ip_dhcp() || any_ip6_dhcp(); | ||
| } | ||
|
|
||
| +extern int lockfd_directory; | ||
| +extern int lockfd_network; | ||
| extern int arg_private; // mount private /home | ||
| extern int arg_private_cache; // private home/.cache | ||
| extern int arg_debug; // print debug messages | ||
| diff --git a/src/firejail/main.c b/src/firejail/main.c | ||
| index 18e9ae651..33399a2a8 100644 | ||
| --- a/src/firejail/main.c | ||
| +++ b/src/firejail/main.c | ||
| @@ -63,6 +63,8 @@ gid_t firejail_gid = 0; | ||
| static char child_stack[STACK_SIZE] __attribute__((aligned(STACK_ALIGNMENT))); // space for child's stack | ||
|
|
||
| Config cfg; // configuration | ||
| +int lockfd_directory = -1; | ||
| +int lockfd_network = -1; | ||
| int arg_private = 0; // mount private /home and /tmp directoryu | ||
| int arg_private_cache = 0; // mount private home/.cache | ||
| int arg_debug = 0; // print debug messages | ||
| @@ -1050,8 +1052,6 @@ static int check_postexec(const char *list) { | ||
| int main(int argc, char **argv, char **envp) { | ||
| int i; | ||
| int prog_index = -1; // index in argv where the program command starts | ||
| - int lockfd_network = -1; | ||
| - int lockfd_directory = -1; | ||
| int custom_profile = 0; // custom profile loaded | ||
| int arg_caps_cmdline = 0; // caps requested on command line (used to break out of --chroot) | ||
| char **ptr; | ||
| -- | ||
| 2.25.1 | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,209 @@ | ||
| From d2a1f0773e7f570d8e2cba2cc0289fde1820277b Mon Sep 17 00:00:00 2001 | ||
| From: Simo Piiroinen <simo.piiroinen@jolla.com> | ||
| Date: Wed, 17 Apr 2024 17:02:31 -0300 | ||
| Subject: [PATCH] modif: improve flock handling | ||
|
|
||
| Changes: | ||
|
|
||
| * Centralize flock handling in preproc.c | ||
| * Add debug and error logging | ||
| * Abort if anything fails | ||
|
|
||
| Co-authored-by: Kelvin M. Klann <kmk3.code@protonmail.com> | ||
| --- | ||
| src/firejail/firejail.h | 4 ++ | ||
| src/firejail/main.c | 34 +++-------------- | ||
| src/firejail/preproc.c | 83 +++++++++++++++++++++++++++++++++++++++++ | ||
| 3 files changed, 93 insertions(+), 28 deletions(-) | ||
|
|
||
| diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h | ||
| index 1a8bf1fa9..c7bcf0bac 100644 | ||
| --- a/src/firejail/firejail.h | ||
| +++ b/src/firejail/firejail.h | ||
| @@ -414,6 +414,10 @@ int net_get_mac(const char *ifname, unsigned char mac[6]); | ||
| void net_config_interface(const char *dev, uint32_t ip, uint32_t mask, int mtu); | ||
|
|
||
| // preproc.c | ||
| +void preproc_lock_firejail_dir(void); | ||
| +void preproc_unlock_firejail_dir(void); | ||
| +void preproc_lock_firejail_network_dir(void); | ||
| +void preproc_unlock_firejail_network_dir(void); | ||
| void preproc_build_firejail_dir(void); | ||
| void preproc_mount_mnt_dir(void); | ||
| void preproc_clean_run(void); | ||
| diff --git a/src/firejail/main.c b/src/firejail/main.c | ||
| index 33399a2a8..6832ae745 100644 | ||
| --- a/src/firejail/main.c | ||
| +++ b/src/firejail/main.c | ||
| @@ -1162,15 +1162,9 @@ int main(int argc, char **argv, char **envp) { | ||
| preproc_build_firejail_dir(); | ||
| const char *container_name = env_get("container"); | ||
| if (!container_name || strcmp(container_name, "firejail")) { | ||
| - lockfd_directory = open(RUN_DIRECTORY_LOCK_FILE, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); | ||
| - if (lockfd_directory != -1) { | ||
| - int rv = fchown(lockfd_directory, 0, 0); | ||
| - (void) rv; | ||
| - flock(lockfd_directory, LOCK_EX); | ||
| - } | ||
| + preproc_lock_firejail_dir(); | ||
| preproc_clean_run(); | ||
| - flock(lockfd_directory, LOCK_UN); | ||
| - close(lockfd_directory); | ||
| + preproc_unlock_firejail_dir(); | ||
| } | ||
|
|
||
| delete_run_files(getpid()); | ||
| @@ -2990,12 +2984,7 @@ int main(int argc, char **argv, char **envp) { | ||
| // check and assign an IP address - for macvlan it will be done again in the sandbox! | ||
| if (any_bridge_configured()) { | ||
| EUID_ROOT(); | ||
| - lockfd_network = open(RUN_NETWORK_LOCK_FILE, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); | ||
| - if (lockfd_network != -1) { | ||
| - int rv = fchown(lockfd_network, 0, 0); | ||
| - (void) rv; | ||
| - flock(lockfd_network, LOCK_EX); | ||
| - } | ||
| + preproc_lock_firejail_network_dir(); | ||
|
|
||
| if (cfg.bridge0.configured && cfg.bridge0.arg_ip_none == 0) | ||
| check_network(&cfg.bridge0); | ||
| @@ -3024,21 +3013,13 @@ int main(int argc, char **argv, char **envp) { | ||
|
|
||
| // set name and x11 run files | ||
| EUID_ROOT(); | ||
| - lockfd_directory = open(RUN_DIRECTORY_LOCK_FILE, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); | ||
| - if (lockfd_directory != -1) { | ||
| - int rv = fchown(lockfd_directory, 0, 0); | ||
| - (void) rv; | ||
| - flock(lockfd_directory, LOCK_EX); | ||
| - } | ||
| + preproc_lock_firejail_dir(); | ||
| if (cfg.name) | ||
| set_name_run_file(sandbox_pid); | ||
| int display = x11_display(); | ||
| if (display > 0) | ||
| set_x11_run_file(sandbox_pid, display); | ||
| - if (lockfd_directory != -1) { | ||
| - flock(lockfd_directory, LOCK_UN); | ||
| - close(lockfd_directory); | ||
| - } | ||
| + preproc_unlock_firejail_dir(); | ||
| EUID_USER(); | ||
|
|
||
| #ifdef HAVE_DBUSPROXY | ||
| @@ -3271,10 +3252,7 @@ int main(int argc, char **argv, char **envp) { | ||
| close(parent_to_child_fds[1]); | ||
|
|
||
| EUID_ROOT(); | ||
| - if (lockfd_network != -1) { | ||
| - flock(lockfd_network, LOCK_UN); | ||
| - close(lockfd_network); | ||
| - } | ||
| + preproc_unlock_firejail_network_dir(); | ||
| EUID_USER(); | ||
|
|
||
| // lock netfilter firewall | ||
| diff --git a/src/firejail/preproc.c b/src/firejail/preproc.c | ||
| index 44f82681a..cb19fe526 100644 | ||
| --- a/src/firejail/preproc.c | ||
| +++ b/src/firejail/preproc.c | ||
| @@ -18,13 +18,96 @@ | ||
| * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| */ | ||
| #include "firejail.h" | ||
| +#include <sys/file.h> | ||
| #include <sys/mount.h> | ||
| #include <sys/stat.h> | ||
| #include <sys/types.h> | ||
| #include <dirent.h> | ||
| +#include <fcntl.h> | ||
|
|
||
| static int tmpfs_mounted = 0; | ||
|
|
||
| +static void preproc_lock_file(const char *path, int *lockfd_ptr) { | ||
| + assert(path != NULL); | ||
| + assert(lockfd_ptr != NULL); | ||
| + | ||
| + long pid = (long)getpid(); | ||
| + if (arg_debug) | ||
| + fprintf(stderr, "pid=%ld: locking %s ...\n", pid, path); | ||
| + | ||
| + if (*lockfd_ptr != -1) { | ||
| + if (arg_debug) | ||
| + fprintf(stderr, "pid=%ld: already locked %s ...\n", pid, path); | ||
| + return; | ||
| + } | ||
| + | ||
| + int lockfd = open(path, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); | ||
| + if (lockfd == -1) { | ||
| + fprintf(stderr, "Error: cannot create a lockfile at %s\n", path); | ||
| + errExit("open"); | ||
| + } | ||
| + | ||
| + if (fchown(lockfd, 0, 0) == -1) { | ||
| + fprintf(stderr, "Error: cannot chown root:root %s\n", path); | ||
| + errExit("fchown"); | ||
| + } | ||
| + | ||
| + if (flock(lockfd, LOCK_EX) == -1) { | ||
| + fprintf(stderr, "Error: cannot lock %s\n", path); | ||
| + errExit("flock"); | ||
| + } | ||
| + | ||
| + *lockfd_ptr = lockfd; | ||
| + if (arg_debug) | ||
| + fprintf(stderr, "pid=%ld: locked %s\n", pid, path); | ||
| +} | ||
| + | ||
| +static void preproc_unlock_file(const char *path, int *lockfd_ptr) { | ||
| + assert(path != NULL); | ||
| + assert(lockfd_ptr != NULL); | ||
| + | ||
| + long pid = (long)getpid(); | ||
| + if (arg_debug) | ||
| + fprintf(stderr, "pid=%ld: unlocking %s ...\n", pid, path); | ||
| + | ||
| + int lockfd = *lockfd_ptr; | ||
| + if (lockfd == -1) { | ||
| + if (arg_debug) | ||
| + fprintf(stderr, "pid=%ld: already unlocked %s ...\n", pid, path); | ||
| + return; | ||
| + } | ||
| + | ||
| + if (flock(lockfd, LOCK_UN) == -1) { | ||
| + fprintf(stderr, "Error: cannot unlock %s\n", path); | ||
| + errExit("flock"); | ||
| + } | ||
| + | ||
| + if (close(lockfd) == -1) { | ||
| + fprintf(stderr, "Error: cannot close %s\n", path); | ||
| + errExit("close"); | ||
| + } | ||
| + | ||
| + *lockfd_ptr = -1; | ||
| + if (arg_debug) | ||
| + fprintf(stderr, "pid=%ld: unlocked %s\n", pid, path); | ||
| +} | ||
| + | ||
| +void preproc_lock_firejail_dir(void) { | ||
| + preproc_lock_file(RUN_DIRECTORY_LOCK_FILE, &lockfd_directory); | ||
| +} | ||
| + | ||
| +void preproc_unlock_firejail_dir(void) { | ||
| + preproc_unlock_file(RUN_DIRECTORY_LOCK_FILE, &lockfd_directory); | ||
| +} | ||
| + | ||
| +void preproc_lock_firejail_network_dir(void) { | ||
| + preproc_lock_file(RUN_NETWORK_LOCK_FILE, &lockfd_network); | ||
| +} | ||
| + | ||
| +void preproc_unlock_firejail_network_dir(void) { | ||
| + preproc_unlock_file(RUN_NETWORK_LOCK_FILE, &lockfd_network); | ||
| +} | ||
| + | ||
| // build /run/firejail directory | ||
| void preproc_build_firejail_dir(void) { | ||
| struct stat s; | ||
| -- | ||
| 2.25.1 | ||
|
|
119 changes: 119 additions & 0 deletions
119
rpm/0010-modif-populate-run-firejail-while-holding-flock.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| From 8bf4c1582c2d3f4018ab08266ef41063d0e0ffe8 Mon Sep 17 00:00:00 2001 | ||
| From: Simo Piiroinen <simo.piiroinen@jolla.com> | ||
| Date: Thu, 4 Apr 2024 14:02:29 +0300 | ||
| Subject: [PATCH] modif: populate /run/firejail while holding flock | ||
|
|
||
| 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 | ||
| --- | ||
| src/firejail/chroot.c | 5 ++++- | ||
| src/firejail/firejail.h | 3 ++- | ||
| src/firejail/main.c | 10 +++++----- | ||
| src/firejail/preproc.c | 13 ++++++++++++- | ||
| 4 files changed, 23 insertions(+), 8 deletions(-) | ||
|
|
||
| diff --git a/src/firejail/chroot.c b/src/firejail/chroot.c | ||
| index 72322221c..267004e92 100644 | ||
| --- a/src/firejail/chroot.c | ||
| +++ b/src/firejail/chroot.c | ||
| @@ -273,7 +273,10 @@ void fs_chroot(const char *rootdir) { | ||
| errExit("mounting /proc"); | ||
|
|
||
| // create all other /run/firejail files and directories | ||
| - preproc_build_firejail_dir(); | ||
| + preproc_build_firejail_dir_unlocked(); | ||
| + preproc_lock_firejail_dir(); | ||
| + preproc_build_firejail_dir_locked(); | ||
| + preproc_unlock_firejail_dir(); | ||
|
|
||
| // update /var directory in order to support multiple sandboxes running on the same root directory | ||
| // if (!arg_private_dev) | ||
| diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h | ||
| index c7bcf0bac..53401b5c6 100644 | ||
| --- a/src/firejail/firejail.h | ||
| +++ b/src/firejail/firejail.h | ||
| @@ -418,7 +418,8 @@ void preproc_lock_firejail_dir(void); | ||
| void preproc_unlock_firejail_dir(void); | ||
| void preproc_lock_firejail_network_dir(void); | ||
| void preproc_unlock_firejail_network_dir(void); | ||
| -void preproc_build_firejail_dir(void); | ||
| +void preproc_build_firejail_dir_unlocked(void); | ||
| +void preproc_build_firejail_dir_locked(void); | ||
| void preproc_mount_mnt_dir(void); | ||
| void preproc_clean_run(void); | ||
|
|
||
| diff --git a/src/firejail/main.c b/src/firejail/main.c | ||
| index 6832ae745..61068bd21 100644 | ||
| --- a/src/firejail/main.c | ||
| +++ b/src/firejail/main.c | ||
| @@ -1159,13 +1159,13 @@ int main(int argc, char **argv, char **envp) { | ||
| #endif | ||
|
|
||
| // build /run/firejail directory structure | ||
| - preproc_build_firejail_dir(); | ||
| + preproc_build_firejail_dir_unlocked(); | ||
| + preproc_lock_firejail_dir(); | ||
| + preproc_build_firejail_dir_locked(); | ||
| const char *container_name = env_get("container"); | ||
| - if (!container_name || strcmp(container_name, "firejail")) { | ||
| - preproc_lock_firejail_dir(); | ||
| + if (!container_name || strcmp(container_name, "firejail")) | ||
| preproc_clean_run(); | ||
| - preproc_unlock_firejail_dir(); | ||
| - } | ||
| + preproc_unlock_firejail_dir(); | ||
|
|
||
| delete_run_files(getpid()); | ||
| atexit(clear_atexit); | ||
| diff --git a/src/firejail/preproc.c b/src/firejail/preproc.c | ||
| index cb19fe526..6bbd93ba6 100644 | ||
| --- a/src/firejail/preproc.c | ||
| +++ b/src/firejail/preproc.c | ||
| @@ -109,7 +109,10 @@ void preproc_unlock_firejail_network_dir(void) { | ||
| } | ||
|
|
||
| // build /run/firejail directory | ||
| -void preproc_build_firejail_dir(void) { | ||
| +// | ||
| +// Note: This creates the base directory of the rundir lockfile; | ||
| +// it should be called before preproc_lock_firejail_dir(). | ||
| +void preproc_build_firejail_dir_unlocked(void) { | ||
| struct stat s; | ||
|
|
||
| // CentOS 6 doesn't have /run directory | ||
| @@ -118,6 +121,14 @@ void preproc_build_firejail_dir(void) { | ||
| } | ||
|
|
||
| create_empty_dir_as_root(RUN_FIREJAIL_DIR, 0755); | ||
| +} | ||
| + | ||
| +// build directory hierarchy under /run/firejail | ||
| +// | ||
| +// Note: Remounts have timing hazards. This function should | ||
| +// only be called after acquiring the directory lock via | ||
| +// preproc_lock_firejail_dir(). | ||
| +void preproc_build_firejail_dir_locked(void) { | ||
| create_empty_dir_as_root(RUN_FIREJAIL_NETWORK_DIR, 0755); | ||
| create_empty_dir_as_root(RUN_FIREJAIL_BANDWIDTH_DIR, 0755); | ||
| create_empty_dir_as_root(RUN_FIREJAIL_NAME_DIR, 0755); | ||
| -- | ||
| 2.25.1 | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters