[PATCH] mdadm: fix dangling file descriptor when calling Manage_ro

[PATCH] mdadm: fix dangling file descriptor when calling Manage_ro

[Abd-Alrhman Masalkhi]

When managing an externally managed array, Manage_ro() closes the file
descriptor and writes the read-only state to the array's array_state
sysfs file. Since the file descriptor is passed by value, the caller
retains a descriptor that refers to a closed file. Any subsequent
attempt to use it will fail.

In such cases, when specifying --stop alongside --readonly, mdadm fails
to stop the array and reports "Cannot get exclusive access"

Fixes: 1b4b73fd535a ("mdadm: Manage.c fix coverity issues")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
 Manage.c | 32 +++++++++++++++++++++++++-------
 mdadm.c  | 18 +++++++++++++-----
 mdadm.h  |  3 ++-
 3 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/Manage.c b/Manage.c
index 40d52cfc913b..b0ba1e31bcb0 100644
--- a/Manage.c
+++ b/Manage.c
@@ -30,7 +30,15 @@
 
 #include <ctype.h>
 
-int Manage_ro(char *devname, int fd, int readonly)
+/*
+ * Switch the array to read-only (readonly > 0) or read-write (readonly < 0).
+ *
+ * Switching to read-only may close *fd: writing "readonly" to array_state
+ * and the control node both need zero openers. *fd is then set to -1.
+ * Switching to read-write never closes *fd, it is better to use Manage_rw()
+ * for it.
+ */
+int Manage_ro(char *devname, int *fd, int readonly)
 {
 	/* switch to readonly or rw
 	 *
@@ -45,7 +53,7 @@ int Manage_ro(char *devname, int fd, int readonly)
 	/* If this is an externally-managed array, we need to modify the
 	 * metadata_version so that mdmon doesn't undo our change.
 	 */
-	mdi = sysfs_read(fd, NULL, GET_LEVEL|GET_VERSION);
+	mdi = sysfs_read(*fd, NULL, GET_LEVEL|GET_VERSION);
 	if (mdi &&
 	    mdi->array.major_version == -1 &&
 	    is_subarray(mdi->text_version)) {
@@ -58,7 +66,7 @@ int Manage_ro(char *devname, int fd, int readonly)
 			vers[MD_VER_BLOCKED_IDX] = '-';
 			sysfs_set_str(mdi, NULL, "metadata_version", vers);
 
-			close_fd(&fd);
+			close_fd(fd);
 			rv = sysfs_set_str(mdi, NULL, "array_state", "readonly");
 
 			if (rv < 0) {
@@ -86,21 +94,21 @@ int Manage_ro(char *devname, int fd, int readonly)
 		goto out;
 	}
 
-	if (!md_array_active(fd)) {
+	if (!md_array_active(*fd)) {
 		pr_err("%s does not appear to be active.\n", devname);
 		rv = 1;
 		goto out;
 	}
 
 	if (readonly > 0) {
-		if (ioctl(fd, STOP_ARRAY_RO, NULL)) {
+		if (ioctl(*fd, STOP_ARRAY_RO, NULL)) {
 			pr_err("failed to set readonly for %s: %s\n",
 				devname, strerror(errno));
 			rv = 1;
 			goto out;
 		}
 	} else if (readonly < 0) {
-		if (ioctl(fd, RESTART_ARRAY_RW, NULL)) {
+		if (ioctl(*fd, RESTART_ARRAY_RW, NULL)) {
 			pr_err("failed to set writable for %s: %s\n",
 				devname, strerror(errno));
 			rv = 1;
@@ -112,6 +120,16 @@ out:
 	return rv;
 }
 
+int Manage_rw(char *devname, int fd)
+{
+	int tmp = fd;
+	int rv = Manage_ro(devname, &tmp, -1);
+
+	/* read-write must not close the caller's fd */
+	assert(tmp == fd);
+	return rv;
+}
+
 static void remove_devices(char *devnm, char *path)
 {
 	/*
@@ -1072,7 +1090,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 		}
 		if (dv->disposition == 'j') {
 			pr_err("Journal added successfully, making %s read-write\n", devname);
-			if (Manage_ro(devname, fd, -1))
+			if (Manage_rw(devname, fd))
 				pr_err("Failed to make %s read-write\n", devname);
 		}
 
diff --git a/mdadm.c b/mdadm.c
index 2517b037c1bc..98f352afb66d 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1453,13 +1453,21 @@ int main(int argc, char *argv[])
 	case MANAGE:
 		/* readonly, add/remove, readwrite, runstop */
 		if (c.readonly > 0)
-			rv = Manage_ro(ident.devname, mdfd, c.readonly);
+			rv = Manage_ro(ident.devname, &mdfd, c.readonly);
+
+		/* Manage_ro() might have closed the md device if needed. */
+		if (!rv && mdfd < 0 && (devs_found > 1 || c.runstop)) {
+			mdfd = open_mddev(ident.devname, 1);
+			if (mdfd < 0)
+				rv = 1;
+		}
+
 		if (!rv && devs_found > 1)
 			rv = Manage_subdevs(ident.devname, mdfd,
 					    devlist->next, c.verbose,
 					    c.test, c.update, c.force);
 		if (!rv && c.readonly < 0)
-			rv = Manage_ro(ident.devname, mdfd, c.readonly);
+			rv = Manage_rw(ident.devname, mdfd);
 		if (!rv && c.runstop > 0)
 			rv = Manage_run(ident.devname, mdfd, &c);
 		if (!rv && c.runstop < 0)
@@ -2013,13 +2021,13 @@ static int misc_list(struct mddev_dev *devlist,
 				rv |= Manage_stop(dv->devname, mdfd, c->verbose, 0);
 				break;
 			case 'o':
-				rv |= Manage_ro(dv->devname, mdfd, 1);
+				rv |= Manage_ro(dv->devname, &mdfd, 1);
 				break;
 			case 'w':
-				rv |= Manage_ro(dv->devname, mdfd, -1);
+				rv |= Manage_rw(dv->devname, mdfd);
 				break;
 			}
-			close(mdfd);
+			close_fd(&mdfd);
 		} else
 			rv |= 1;
 	}
diff --git a/mdadm.h b/mdadm.h
index deac7a56f6f8..e2f725e01f9b 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1427,7 +1427,8 @@ struct stat64;
 
 extern int add_dev(const char *name, const struct stat *stb, int flag, struct FTW *s);
 
-extern int Manage_ro(char *devname, int fd, int readonly);
+extern int Manage_ro(char *devname, int *fd, int readonly);
+extern int Manage_rw(char *devname, int fd);
 extern int Manage_run(char *devname, int fd, struct context *c);
 extern int Manage_stop(char *devname, int fd, int quiet,
 		       int will_retry);
-- 
2.43.0