fsck -l: fix racing between unlock/unlink and open

From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001

Probably, there are better solutions, but I prefer KISS.

From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email protected]>
Date: Fri, 8 Apr 2016 00:38:56 +0300
Subject: [PATCH] fsck: fix racing between unlock/unlink and open

Process A Process B Process C
open()
[creates file]
lock()
[succeed]

		open()
		[open existing]
		lock()...
running()
close()
		[...succeed]
unlink()
		running()
				open()
				[creates file] {BAD!}
				lock()
				[succeed] {BAD!}
				running() {BAD!}
		close()

Quoted 12 lines · Yuriy M. Kaminskiy

Probably, there are better solutions, but I prefer KISS.

From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email protected]>
Date: Fri, 8 Apr 2016 00:38:56 +0300
Subject: [PATCH] fsck: fix racing between unlock/unlink and open

Process A Process B Process C
open()
[creates file]
lock()
[succeed]

Quoted 15 lines

		open()
		[open existing]
		lock()...
running()
close()
		[...succeed]
unlink()
		running()
				open()
				[creates file] {BAD!}
				lock()
				[succeed] {BAD!}
				running() {BAD!}
		close()

It would be better to check if the file still exist on the filesystem
with the same inode number, if not then re-open. It means that on race
you will create a new lock file. (You have to unlock after unlink.)
See http://stackoverflow.com/questions/17708885/flock-removing-locked-file-without-race-condition

Simple example:

#include <stdio.h>
#include <sys/file.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>

int main(int argc, char **argv)

{
	const char *lockpath = "test.lock";
	int fd;
	struct stat st_lock, st_file;

	while (1) {
		fd = open(lockpath, O_RDONLY|O_CREAT|O_CLOEXEC,
				    S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH);
		fstat(fd, &st_lock);
		flock(fd, LOCK_EX);

		errno = 0;
		if (stat(lockpath, &st_file) == 0 && st_file.st_ino == st_lock.st_ino)
			break;

		if (errno)
			fprintf(stderr, "%d: stat failed: %m\n", getpid());
		else
			fprintf(stderr, "%d: ignore %lu (fs has:%lu)\n", getpid(), st_lock.st_ino, st_file.st_ino);
		close(fd);
	}

	fprintf(stderr, "%d: -->locked ino: %lu\n", getpid(), st_lock.st_ino);
	sleep(10);
	unlink(lockpath);
	close(fd);
	return 0;
}

See untested fsck patch below. Comments?

Quoted 12 lines · Yuriy M. Kaminskiy

Probably, there are better solutions, but I prefer KISS.

From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email protected]>
Date: Fri, 8 Apr 2016 00:38:56 +0300
Subject: [PATCH] fsck: fix racing between unlock/unlink and open

Process A Process B Process C
open()
[creates file]
lock()
[succeed]

Quoted 15 lines

              open()
              [open existing]
              lock()...
running()
close()
              [...succeed]
unlink()
              running()
                              open()
                              [creates file] {BAD!}
                              lock()
                              [succeed] {BAD!}
                              running() {BAD!}
              close()

It would be better to check if the file still exist on the filesystem
with the same inode number, if not then re-open. It means that on race
you will create a new lock file. (You have to unlock after unlink.)
See http://stackoverflow.com/questions/17708885/flock-removing-locked-file-without-race-condition

Simple example:

#include <stdio.h>
#include <sys/file.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>

int main(int argc, char **argv)

{
        const char *lockpath = "test.lock";
        int fd;
        struct stat st_lock, st_file;

        while (1) {
                fd = open(lockpath, O_RDONLY|O_CREAT|O_CLOEXEC,
                                    S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH);
                fstat(fd, &st_lock);
                flock(fd, LOCK_EX);

                errno = 0;
                if (stat(lockpath, &st_file) == 0 && st_file.st_ino == st_lock.st_ino)
                        break;
                if (errno)
                        fprintf(stderr, "%d: stat failed: %m\n", getpid());
                else
                        fprintf(stderr, "%d: ignore %lu (fs has:%lu)\n", getpid(), st_lock.st_ino, st_file.st_ino);
                close(fd);
        }

        fprintf(stderr, "%d: -->locked ino: %lu\n", getpid(), st_lock.st_ino);
        sleep(10);
        unlink(lockpath);
        close(fd);
        return 0;
}

See untested fsck patch below. Comments?

Karel

disk-utils/fsck.c +0 -6

index 05cfbc4..0a7dbc8 100644
@@ -370,23 +370,38 @@ static void lock_disk(struct fsck_instance *inst)
        if (verbose)
                printf(_("Locking disk by %s ... "), inst->lockpath);
 
-       inst->lock = open(inst->lockpath, O_RDONLY|O_CREAT|O_CLOEXEC,
-                                   S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH);
-       if (inst->lock >= 0) {
-               int rc = -1;
-
-               /* inform users that we're waiting on the lock */

disk-utils/fsck.c +9 -0

+                       int rc = -1;
+
+                       /* inform users that we're waiting on the lock */
+                       if (verbose &&
+                           (rc = flock(inst->lock, LOCK_EX | LOCK_NB)) != 0 &&
+                           errno == EWOULDBLOCK)
+                               printf(_("(waiting) "));
+
+                       if (rc != 0 && flock(inst->lock, LOCK_EX) != 0) {

Cons: leaves empty (unlocked/harmless) .lock files in /run/fsck/

Signed-off-by Yuriy M. Kaminskiy <[email protected]>

---
 disk-utils/fsck.c | 1 -
 1 file changed, 1 deletion(-)