NeilBrown: Bugfix: mapfile locking is broken/racy

Martin F. Krafft madduck at alioth.debian.org
Sat Aug 28 18:45:19 UTC 2010


Module: mdadm
Branch: build
Commit: 08415c46a867b26743b6b4fc93d5fc6ed027cb07
URL:    http://git.debian.org/?p=pkg-mdadm/mdadm.git;a=commit;h=08415c46a867b26743b6b4fc93d5fc6ed027cb07

Author: NeilBrown <neilb at suse.de>
Date:   Wed Jul 28 17:40:54 2010 +1000

Bugfix: mapfile locking is broken/racy
    
While we attempt to use a lockfile to grant exclusive access to the
mapfile, our implementation is buggy.  Specifically, we create a lockfile,
then lock it, at which point new instances can open the lockfile and
attempt to lock it, which will cause them to block.  However, when we are
ready to unlock it, we unlink the file.  This causes existing lock waiters
to get a lock on an unlinked inode while a different instance may now
create a new lockfile and get an exclusive lock on it.

There are several possible fixes.  The chosen one is to test if
->s_nlink is zero after we get the lock and to retry if it isn't.
This means:
  - failing to unlink a file doesn't leave a stale lock
  - we can block waiting to get a lock rather than busy-waiting
  - we don't need to leave a lock file permanently in place.
    
Reported-by: Doug Ledford <dledford at redhat.com>
Signed-off-by: NeilBrown <neilb at suse.de>

---

 mapfile.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/mapfile.c b/mapfile.c
index 79a6bd8..8e40b6b 100644
--- a/mapfile.c
+++ b/mapfile.c
@@ -119,7 +119,8 @@ static FILE *lf = NULL;
 static int lwhich = 0;
 int map_lock(struct map_ent **melp)
 {
-	if (lf == NULL) {
+	while (lf == NULL) {
+		struct stat buf;
 		lf = open_map(2, &lwhich);
 		if (lf == NULL)
 			return -1;
@@ -128,6 +129,15 @@ int map_lock(struct map_ent **melp)
 			lf = NULL;
 			return -1;
 		}
+		if (fstat(fileno(lf), &buf) != 0 ||
+		    buf.st_nlink == 0) {
+			/* The owner of the lock unlinked it,
+			 * so we have a lock on a stale file,
+			 * try again
+			 */
+			fclose(lf);
+			lf = NULL;
+		}
 	}
 	if (*melp)
 		map_free(*melp);
@@ -138,10 +148,13 @@ int map_lock(struct map_ent **melp)
 void map_unlock(struct map_ent **melp)
 {
 	if (lf) {
-		flock(fileno(lf), LOCK_UN);
+		/* must unlink before closing the file,
+		 * as only the owner of the lock may
+		 * unlink the file
+		 */
+		unlink(mapname[lwhich][2]);
 		fclose(lf);
 	}
-	unlink(mapname[lwhich][2]);
 	lf = NULL;
 }
 




More information about the pkg-mdadm-commits mailing list