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

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

[Yuriy M. Kaminskiy]

[-- Attachment #1: Type: text/plain, Size: 57 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fsck-fix-racing-between-unlock-and-unlink.patch --]
[-- Type: text/x-diff, Size: 1016 bytes --]

>From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <yumkam@gmail.com>
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()

Cons: leaves empty (unlocked/harmless) .lock files in /run/fsck/
Signed-off-by: Yuriy M. Kaminskiy <yumkam@gmail.com>
---
 disk-utils/fsck.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
index 05cfbc4..84d2dcc 100644
--- a/disk-utils/fsck.c
+++ b/disk-utils/fsck.c
@@ -410,7 +410,6 @@ static void unlock_disk(struct fsck_instance *inst)
 		printf(_("Unlocking %s.\n"), inst->lockpath);
 
 	close(inst->lock);			/* unlock */
-	unlink(inst->lockpath);
 
 	free(inst->lockpath);
 
-- 
2.1.4

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

[Karel Zak]

On Fri, Apr 08, 2016 at 12:59:29AM +0300, Yuriy M. Kaminskiy wrote:
> Probably, there are better solutions, but I prefer KISS.

> From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
> From: "Yuriy M. Kaminskiy" <yumkam@gmail.com>
> 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()
> 

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


diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
index 05cfbc4..0a7dbc8 100644
--- a/disk-utils/fsck.c
+++ b/disk-utils/fsck.c
@@ -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 */
-		if (verbose &&
-		    (rc = flock(inst->lock, LOCK_EX | LOCK_NB)) != 0 &&
-		    errno == EWOULDBLOCK)
-			printf(_("(waiting) "));
-
-		if (rc != 0 && flock(inst->lock, LOCK_EX) != 0) {
-			close(inst->lock);			/* failed */
-			inst->lock = -1;
+	while (1) {
+		struct stat st_lock, st_file;
+
+		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 */
+			if (verbose &&
+			    (rc = flock(inst->lock, LOCK_EX | LOCK_NB)) != 0 &&
+			    errno == EWOULDBLOCK)
+				printf(_("(waiting) "));
+
+			if (rc != 0 && flock(inst->lock, LOCK_EX) != 0) {
+				close(inst->lock);			/* failed */
+				inst->lock = -1;
+			}
 		}
+
+		/*
+		 * Make sure the locked lockfile really exist on the filesystem
+		 * to avoid race between lock and unlink (see unlock_disk())
+		 */
+		fstat(inst->lock, &st_lock);
+		if (stat(inst->lockpath, &st_file) == 0
+		    && st_file.st_ino == st_lock.st_ino)
+			break;				/* success */
+		close(inst->lock);
 	}
 
+
 	if (verbose)
 		/* TRANSLATORS: These are followups to "Locking disk...". */
 		printf("%s.\n", inst->lock >= 0 ? _("succeeded") : _("failed"));
@@ -409,8 +424,8 @@ static void unlock_disk(struct fsck_instance *inst)
 	if (verbose)
 		printf(_("Unlocking %s.\n"), inst->lockpath);
 
-	close(inst->lock);			/* unlock */
 	unlink(inst->lockpath);
+	close(inst->lock);			/* unlock */
 
 	free(inst->lockpath);
 
 

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

[Yuriy M. Kaminskiy]

Karel Zak <kzak@redhat.com> writes:

> On Fri, Apr 08, 2016 at 12:59:29AM +0300, Yuriy M. Kaminskiy wrote:
>> Probably, there are better solutions, but I prefer KISS.
>
>> From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
>> From: "Yuriy M. Kaminskiy" <yumkam@gmail.com>
>> 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()
>> 
>
> 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;

Requires stable inode numbers (well, probably acceptable assumption for {/var,}/run).

> 		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?

I still prefer KISS. (And I bet it will eat more resources than just
leaving those empty files around.)
(But either way, bug will be fixed, so I do as you like).

[...]

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

[Nir Soffer]

On Mon, Apr 18, 2016 at 1:22 PM, Karel Zak <kzak@redhat.com> wrote:
>
> On Fri, Apr 08, 2016 at 12:59:29AM +0300, Yuriy M. Kaminskiy wrote:
> > Probably, there are better solutions, but I prefer KISS.
>
> > From fcb97ddcbc336bc8860828c47cdaf21c7b1ca655 Mon Sep 17 00:00:00 2001
> > From: "Yuriy M. Kaminskiy" <yumkam@gmail.com>
> > 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()
> >
>
> 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?

When multiple processes blocked on the same lock file, this
ensure that all of them will fail to validate the lock when flock
return, and will have to try again, but this is probably fine for
this use case.

>
>     Karel
>
>
> diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
> index 05cfbc4..0a7dbc8 100644
> --- a/disk-utils/fsck.c
> +++ b/disk-utils/fsck.c
> @@ -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 */
> -               if (verbose &&
> -                   (rc = flock(inst->lock, LOCK_EX | LOCK_NB)) != 0 &&
> -                   errno == EWOULDBLOCK)
> -                       printf(_("(waiting) "));
> -
> -               if (rc != 0 && flock(inst->lock, LOCK_EX) != 0) {
> -                       close(inst->lock);                      /* failed */
> -                       inst->lock = -1;
> +       while (1) {
> +               struct stat st_lock, st_file;

I would rename st_file to st_lockpath.

> +
> +               inst->lock = open(inst->lockpath, O_RDONLY|O_CREAT|O_CLOEXEC,
> +                                           S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH);
> +               if (inst->lock >= 0) {

We can replace this with

    if (inst->lock == -1)
        break;  /* failed */

Simplifying the normal flow, and eliminating the issue of
validating a lock after open failed.

> +                       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) {
> +                               close(inst->lock);                      /* failed */
> +                               inst->lock = -1;
> +                       }
>                 }
> +
> +               /*
> +                * Make sure the locked lockfile really exist on the filesystem
> +                * to avoid race between lock and unlink (see unlock_disk())
> +                */
> +               fstat(inst->lock, &st_lock);

This is going to fail when inst->lock == -1. Then the contents of
st_lock are random junk from the stack.

> +               if (stat(inst->lockpath, &st_file) == 0
> +                   && st_file.st_ino == st_lock.st_ino)

This check is relevant only if inst->lock >= 0

> +                       break;                          /* success */
> +               close(inst->lock);
>         }
>
> +
>         if (verbose)
>                 /* TRANSLATORS: These are followups to "Locking disk...". */
>                 printf("%s.\n", inst->lock >= 0 ? _("succeeded") : _("failed"));
> @@ -409,8 +424,8 @@ static void unlock_disk(struct fsck_instance *inst)
>         if (verbose)
>                 printf(_("Unlocking %s.\n"), inst->lockpath);
>
> -       close(inst->lock);                      /* unlock */

Here we should add a comment about the order and link to
lock_disk() depending on this order.

>         unlink(inst->lockpath);
> +       close(inst->lock);                      /* unlock */
>
>         free(inst->lockpath);

Nir

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

[Karel Zak]

On Fri, Apr 08, 2016 at 12:59:29AM +0300, Yuriy M. Kaminskiy wrote:
> Cons: leaves empty (unlocked/harmless) .lock files in /run/fsck/
> Signed-off-by: Yuriy M. Kaminskiy <yumkam@gmail.com>
> ---
>  disk-utils/fsck.c | 1 -
>  1 file changed, 1 deletion(-)

I have applied this KISS solution.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com