[PATCH] fix: correct unlocking of map file

[PATCH] fix: correct unlocking of map file

[Lukasz Dorau]

1. Three missing map_unlock() calls were added.
2. Map file's lock must be released by an explicit LOCK_UN operation,
   because duplicated (e.g. created by fork()) descriptors
   inherit the lock. As a result child process which inherits open
   descriptors with lock can block another process of mdadm.

Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
---
 Incremental.c |    3 +++
 mapfile.c     |    4 ++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 571d45d..b90089b 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1472,6 +1472,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 					"Cannot activate array(s).\n");
 		/* free container data and exit */
 		sysfs_free(list);
+		map_unlock(&map);
 		return 2;
 	}
 
@@ -1535,6 +1536,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 					fprintf(stderr, Name ": array %s/%s is "
 						"explicitly ignored by mdadm.conf\n",
 						match->container, match->member);
+				map_unlock(&map);
 				return 2;
 			}
 			if (match)
@@ -1550,6 +1552,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 		if (mdfd < 0) {
 			fprintf(stderr, Name ": failed to open %s: %s.\n",
 				chosen_name, strerror(errno));
+			map_unlock(&map);
 			return 2;
 		}
 
diff --git a/mapfile.c b/mapfile.c
index ff1e973..98c0780 100644
--- a/mapfile.c
+++ b/mapfile.c
@@ -149,6 +149,10 @@ int map_lock(struct map_ent **melp)
 void map_unlock(struct map_ent **melp)
 {
 	if (lf) {
+		/* lock must be released by an explicit LOCK_UN operation
+		 * because duplicated descriptors inherit the lock
+		 */
+		flock(fileno(lf), LOCK_UN);
 		/* must unlink before closing the file,
 		 * as only the owner of the lock may
 		 * unlink the file

Re: [PATCH] fix: correct unlocking of map file

[NeilBrown]

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

On Thu, 29 Sep 2011 09:55:20 +0200 Lukasz Dorau <lukasz.dorau@intel.com>
wrote:

> 1. Three missing map_unlock() calls were added.
> 2. Map file's lock must be released by an explicit LOCK_UN operation,
>    because duplicated (e.g. created by fork()) descriptors
>    inherit the lock. As a result child process which inherits open
>    descriptors with lock can block another process of mdadm.

Thanks.  However I don't like the second bit.  I really don't want the lock
fd to be handed down to the children at all - it is messy.

So I've change it to explicitly close the lock fd on fork.

Thanks,
NeilBrown


commit cc700db34f6fb565b37f4edf7fe7fe40a5f2745b
Author: Lukasz Dorau <lukasz.dorau@intel.com>
Date:   Mon Oct 3 08:55:02 2011 +1100

    fix: correct unlocking of map file
    
    1. Three missing map_unlock() calls were added.
    2. Map file must be unlocked on fork, else child will hold lock.
    
    Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Grow.c b/Grow.c
index b7234e4..90b84d7 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2265,6 +2265,7 @@ started:
 	default:
 		return 0;
 	case 0:
+		map_fork();
 		break;
 	}
 
@@ -2421,6 +2422,7 @@ int reshape_container(char *container, char *devname,
 		printf(Name ": multi-array reshape continues in background\n");
 		return 0;
 	case 0: /* child */
+		map_fork();
 		break;
 	}
 
diff --git a/Incremental.c b/Incremental.c
index 791ad85..a3e05a7 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1469,6 +1469,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 					"Cannot activate array(s).\n");
 		/* free container data and exit */
 		sysfs_free(list);
+		map_unlock(&map);
 		return 2;
 	}
 
@@ -1532,6 +1533,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 					fprintf(stderr, Name ": array %s/%s is "
 						"explicitly ignored by mdadm.conf\n",
 						match->container, match->member);
+				map_unlock(&map);
 				return 2;
 			}
 			if (match)
@@ -1547,6 +1549,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 		if (mdfd < 0) {
 			fprintf(stderr, Name ": failed to open %s: %s.\n",
 				chosen_name, strerror(errno));
+			map_unlock(&map);
 			return 2;
 		}
 
diff --git a/mapfile.c b/mapfile.c
index ff1e973..997f095 100644
--- a/mapfile.c
+++ b/mapfile.c
@@ -159,6 +159,18 @@ void map_unlock(struct map_ent **melp)
 	lf = NULL;
 }
 
+void map_fork(void)
+{
+	/* We are forking, so must close the lock file.
+	 * Don't risk flushing anything though.
+	 */
+	if (lf) {
+		close(fileno(lf));
+		fclose(lf);
+		lf = NULL;
+	}
+}
+
 void map_add(struct map_ent **melp,
 	    int devnum, char *metadata, int uuid[4], char *path)
 {
diff --git a/mdadm.h b/mdadm.h
index 8dd37d9..9165f7a 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -427,6 +427,7 @@ extern void map_add(struct map_ent **melp,
 		    int devnum, char *metadata, int uuid[4], char *path);
 extern int map_lock(struct map_ent **melp);
 extern void map_unlock(struct map_ent **melp);
+extern void map_fork(void);
 
 /* various details can be requested */
 enum sysfs_read_flags {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 190 bytes --]