Construct /run/firejail while holding flock

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 <[email protected]>

src/firejail/chroot.c

@@ -273,7 +273,11 @@ void fs_chroot(const char *rootdir) {
 		errExit("mounting /proc");
 
 	// create all other /run/firejail files and directories
-	preproc_build_firejail_dir();
+	int lockfd_directory = -1;
+	preproc_build_firejail_dir_unlocked();
+	preproc_lock_firejail_dir(&lockfd_directory);
+	preproc_build_firejail_dir_locked();
+	preproc_unlock_firejail_dir(&lockfd_directory);
 
 	// update /var directory in order to support multiple sandboxes running on the same root directory
 	//	if (!arg_private_dev)

src/firejail/firejail.h

@@ -429,7 +429,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_build_firejail_dir(void);
+void preproc_lock_firejail_dir(int *lockfd_ptr);
+void preproc_unlock_firejail_dir(int *lockfd_ptr);
+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);
 

src/firejail/main.c

@@ -1166,19 +1166,14 @@ 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(&lockfd_directory);
+	preproc_build_firejail_dir_locked();
 	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_clean_run();
-		flock(lockfd_directory, LOCK_UN);
-		close(lockfd_directory);
 	}
+	preproc_unlock_firejail_dir(&lockfd_directory);
 
 	delete_run_files(getpid());
 	atexit(clear_atexit);
@@ -3024,21 +3019,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(&lockfd_directory);
 	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(&lockfd_directory);
 	EUID_USER();
 
 #ifdef HAVE_DBUSPROXY

src/firejail/preproc.c

@@ -21,12 +21,61 @@
 #include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <sys/file.h>
 #include <dirent.h>
+#include <fcntl.h>
 
 static int tmpfs_mounted = 0;
 
+static void preproc_lock_file(const char *path, int *lockfd_ptr) {
+	if( *lockfd_ptr == -1 ) {
+		if (arg_debug)
+			fprintf(stderr, "pid=%d: lock %s ...\n", (int)getpid(), path);
+		int lockfd = open(path, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);
+		if( lockfd == -1 )
+			errExit(path);
+
+		if (fchown(lockfd, 0, 0) == -1)
+			fprintf(stderr, "Error: cannot chown root:root %s: %m\n", path);
+
+		if (flock(lockfd, LOCK_EX) == -1) {
+			fprintf(stderr, "Error: cannot lock %s: %m\n", path);
+			close(lockfd);
+		}
+		else {
+			if (arg_debug)
+				fprintf(stderr, "pid=%d: locked %s\n", (int)getpid(), path);
+			*lockfd_ptr = lockfd;
+		}
+	}
+}
+
+static void preproc_unlock_file(const char *path, int *lockfd_ptr) {
+	int lockfd = *lockfd_ptr;
+	if (lockfd != -1) {
+		*lockfd_ptr = -1;
+		if (arg_debug)
+			fprintf(stderr, "pid=%d: unlock %s\n", (int)getpid(), path);
+		if (flock(lockfd, LOCK_UN) == -1)
+			fprintf(stderr, "Error: cannot unlock %s: %m\n", path);
+		if (close(lockfd) == -1)
+			fprintf(stderr, "Error: cannot close %s: %m\n", path);
+	}
+}
+
+void preproc_lock_firejail_dir(int *lockfd_ptr) {
+	preproc_lock_file(RUN_DIRECTORY_LOCK_FILE, lockfd_ptr);
+}
+
+void preproc_unlock_firejail_dir(int *lockfd_ptr) {
+	preproc_unlock_file(RUN_DIRECTORY_LOCK_FILE, lockfd_ptr);
+}
+
 // build /run/firejail directory
-void preproc_build_firejail_dir(void) {
+void preproc_build_firejail_dir_unlocked(void) {
+	// Use atomic / otherwise timing hazard free operations
+	// to create enough of /run/firejail directory hierarchy
+	// to have a place for lock files
 	struct stat s;
 
 	// CentOS 6 doesn't have /run directory
@@ -35,6 +84,15 @@ void preproc_build_firejail_dir(void) {
 	}
 
 	create_empty_dir_as_root(RUN_FIREJAIL_DIR, 0755);
+}
+
+// build directory hierarchy under /run/firejail
+void preproc_build_firejail_dir_locked(void) {
+	// Remounts have timing hazards. This function
+	// should be called only after acquiring directory
+	// lock via preproc_lock_firejail_dir()
+
+	// Continue with directory hierarchy
 	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);