imsm: introduce helpers to manage file descriptors

To avoid direct comparisions define dedicated inlines.
This patch propagates them in super-intel.c. They are declared globally
for future usage outside IMSM.

Additionally, it adds fd check in save_backup_imsm() to remove
code vulnerability and simplifies targets array implementation.

It also propagates pr_vrb() macro instead if (verbose) condidtion.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Signed-off-by: Jes Sorensen <jsorensen@fb.com>

2 files changed, 100 insertions, 92 deletions

diff --git a/mdadm.h b/mdadm.h
index 4e483bfc..54567396 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -783,6 +783,31 @@ static inline char *map_dev(int major, int minor, int create)
 	return map_dev_preferred(major, minor, create, NULL);
 }
 
+/**
+ * is_fd_valid() - check file descriptor.
+ * @fd: file descriptor.
+ *
+ * The function checks if @fd is nonnegative integer and shall be used only
+ * to verify open() result.
+ */
+static inline int is_fd_valid(int fd)
+{
+	return (fd > -1);
+}
+
+/**
+ * close_fd() - verify, close and unset file descriptor.
+ * @fd: pointer to file descriptor.
+ *
+ * The function closes and invalidates file descriptor if appropriative. It
+ * ignores incorrect file descriptor quitely to simplify error handling.
+ */
+static inline void close_fd(int *fd)
+{
+	if (is_fd_valid(*fd) && close(*fd) == 0)
+		*fd = -1;
+}
+
 struct active_array;
 struct metadata_update;
 
diff --git a/super-intel.c b/super-intel.c
index 08c64409..28472a1a 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -691,7 +691,7 @@ static struct sys_dev* find_disk_attached_hba(int fd, const char *devname)
 	if ((list = find_intel_devices()) == NULL)
 		return 0;
 
-	if (fd < 0)
+	if (!is_fd_valid(fd))
 		disk_path  = (char *) devname;
 	else
 		disk_path = diskfd_to_devpath(fd, 1, NULL);
@@ -2406,7 +2406,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 		}
 
 		fd = dev_open(ent->d_name, O_RDONLY);
-		if (fd < 0)
+		if (!is_fd_valid(fd))
 			printf("          Port%d : - disk info unavailable -\n", port);
 		else {
 			fd2devname(fd, buf);
@@ -2455,7 +2455,7 @@ static int print_nvme_info(struct sys_dev *hba)
 			goto skip;
 
 		fd = open_dev(ent->d_name);
-		if (fd < 0)
+		if (!is_fd_valid(fd))
 			goto skip;
 
 		if (!diskfd_to_devpath(fd, 0, ns_path) ||
@@ -2481,8 +2481,7 @@ static int print_nvme_info(struct sys_dev *hba)
 			printf("()\n");
 
 skip:
-		if (fd > -1)
-			close(fd);
+		close_fd(&fd);
 	}
 
 	closedir(dir);
@@ -3171,10 +3170,11 @@ static int load_imsm_migr_rec(struct intel_super *super)
 		if (slot > 1 || slot < 0)
 			continue;
 
-		if (dl->fd < 0) {
+		if (!is_fd_valid(dl->fd)) {
 			sprintf(nm, "%d:%d", dl->major, dl->minor);
 			fd = dev_open(nm, O_RDONLY);
-			if (fd >= 0) {
+
+			if (is_fd_valid(fd)) {
 				keep_fd = 0;
 				break;
 			}
@@ -3184,7 +3184,7 @@ static int load_imsm_migr_rec(struct intel_super *super)
 		}
 	}
 
-	if (fd < 0)
+	if (!is_fd_valid(fd))
 		return retval;
 	retval = read_imsm_migr_rec(fd, super);
 	if (!keep_fd)
@@ -4542,10 +4542,10 @@ load_and_parse_mpb(int fd, struct intel_super *super, char *devname, int keep_fd
 	return err;
 }
 
-static void __free_imsm_disk(struct dl *d, int close_fd)
+static void __free_imsm_disk(struct dl *d, int do_close)
 {
-	if (close_fd && d->fd > -1)
-		close(d->fd);
+	if (do_close)
+		close_fd(&d->fd);
 	if (d->devname)
 		free(d->devname);
 	if (d->e)
@@ -4650,12 +4650,12 @@ static int find_intel_hba_capability(int fd, struct intel_super *super, char *de
 	struct sys_dev *hba_name;
 	int rv = 0;
 
-	if (fd >= 0 && test_partition(fd)) {
+	if (is_fd_valid(fd) && test_partition(fd)) {
 		pr_err("imsm: %s is a partition, cannot be used in IMSM\n",
 		       devname);
 		return 1;
 	}
-	if (fd < 0 || check_env("IMSM_NO_PLATFORM")) {
+	if (!is_fd_valid(fd) || check_env("IMSM_NO_PLATFORM")) {
 		super->orom = NULL;
 		super->hba = NULL;
 		return 0;
@@ -5064,7 +5064,7 @@ static int load_super_imsm_all(struct supertype *st, int fd, void **sbp,
 	int err = 0;
 	int i = 0;
 
-	if (fd >= 0)
+	if (is_fd_valid(fd))
 		/* 'fd' is an opened container */
 		err = get_sra_super_block(fd, &super_list, devname, &i, keep_fd);
 	else
@@ -5121,7 +5121,7 @@ static int load_super_imsm_all(struct supertype *st, int fd, void **sbp,
 		return err;
 
 	*sbp = super;
-	if (fd >= 0)
+	if (is_fd_valid(fd))
 		strcpy(st->container_devnm, fd2devnm(fd));
 	else
 		st->container_devnm[0] = 0;
@@ -5147,7 +5147,7 @@ get_devlist_super_block(struct md_list *devlist, struct intel_super **super_list
 		if (tmpdev->container == 1) {
 			int lmax = 0;
 			int fd = dev_open(tmpdev->devname, O_RDONLY|O_EXCL);
-			if (fd < 0) {
+			if (!is_fd_valid(fd)) {
 				pr_err("cannot open device %s: %s\n",
 					tmpdev->devname, strerror(errno));
 				err = 8;
@@ -5199,7 +5199,7 @@ static int get_super_block(struct intel_super **super_list, char *devnm, char *d
 
 	sprintf(nm, "%d:%d", major, minor);
 	dfd = dev_open(nm, O_RDWR);
-	if (dfd < 0) {
+	if (!is_fd_valid(dfd)) {
 		err = 2;
 		goto error;
 	}
@@ -5226,11 +5226,10 @@ static int get_super_block(struct intel_super **super_list, char *devnm, char *d
 	} else {
 		if (s)
 			free_imsm(s);
-		if (dfd >= 0)
-			close(dfd);
+		close_fd(&dfd);
 	}
-	if (dfd >= 0 && !keep_fd)
-		close(dfd);
+	if (!keep_fd)
+		close_fd(&dfd);
 	return err;
 
 }
@@ -5707,7 +5706,7 @@ static int drive_validate_sector_size(struct intel_super *super, struct dl *dl)
 {
 	unsigned int member_sector_size;
 
-	if (dl->fd < 0) {
+	if (!is_fd_valid(dl->fd)) {
 		pr_err("Invalid file descriptor for %s\n", dl->devname);
 		return 0;
 	}
@@ -5739,7 +5738,7 @@ static int add_to_super_imsm_volume(struct supertype *st, mdu_disk_info_t *dk,
 		return 1;
 	}
 
-	if (fd == -1) {
+	if (!is_fd_valid(fd)) {
 		/* we're doing autolayout so grab the pre-marked (in
 		 * validate_geometry) raid_disk
 		 */
@@ -6115,10 +6114,8 @@ static int write_super_imsm_spares(struct intel_super *super, int doclose)
 		if (write_super_imsm_spare(super, d))
 			return 1;
 
-		if (doclose) {
-			close(d->fd);
-			d->fd = -1;
-		}
+		if (doclose)
+			close_fd(&d->fd);
 	}
 
 	return 0;
@@ -6232,10 +6229,8 @@ static int write_super_imsm(struct supertype *st, int doclose)
 				d->major, d->minor,
 				d->fd, strerror(errno));
 
-		if (doclose) {
-			close(d->fd);
-			d->fd = -1;
-		}
+		if (doclose)
+			close_fd(&d->fd);
 	}
 
 	if (spares)
@@ -6666,10 +6661,8 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
 		return 1;
 
 	fd = dev_open(dev, O_RDONLY|O_EXCL);
-	if (fd < 0) {
-		if (verbose > 0)
-			pr_err("imsm: Cannot open %s: %s\n",
-				dev, strerror(errno));
+	if (!is_fd_valid(fd)) {
+		pr_vrb("imsm: Cannot open %s: %s\n", dev, strerror(errno));
 		return 0;
 	}
 	if (!get_dev_size(fd, dev, &ldsize))
@@ -6878,12 +6871,12 @@ active_arrays_by_format(char *name, char* hba, struct md_list **devlist,
 		    memb->members) {
 			struct dev_member *dev = memb->members;
 			int fd = -1;
-			while(dev && (fd < 0)) {
+			while (dev && !is_fd_valid(fd)) {
 				char *path = xmalloc(strlen(dev->name) + strlen("/dev/") + 1);
 				num = sprintf(path, "%s%s", "/dev/", dev->name);
 				if (num > 0)
 					fd = open(path, O_RDONLY, 0);
-				if (num <= 0 || fd < 0) {
+				if (num <= 0 || !is_fd_valid(fd)) {
 					pr_vrb("Cannot open %s: %s\n",
 					       dev->name, strerror(errno));
 				}
@@ -6891,7 +6884,7 @@ active_arrays_by_format(char *name, char* hba, struct md_list **devlist,
 				dev = dev->next;
 			}
 			found = 0;
-			if (fd >= 0 && disk_attached_to_hba(fd, hba)) {
+			if (is_fd_valid(fd) && disk_attached_to_hba(fd, hba)) {
 				struct mdstat_ent *vol;
 				for (vol = mdstat ; vol ; vol = vol->next) {
 					if (vol->active > 0 &&
@@ -6911,8 +6904,7 @@ active_arrays_by_format(char *name, char* hba, struct md_list **devlist,
 					*devlist = dv;
 				}
 			}
-			if (fd >= 0)
-				close(fd);
+			close_fd(&fd);
 		}
 	}
 	free_mdstat(mdstat);
@@ -6973,7 +6965,7 @@ get_devices(const char *hba_path)
 		free(path);
 		path = NULL;
 		fd = dev_open(ent->d_name, O_RDONLY);
-		if (fd >= 0) {
+		if (is_fd_valid(fd)) {
 			fd2devname(fd, buf);
 			close(fd);
 		} else {
@@ -7032,7 +7024,7 @@ count_volumes_list(struct md_list *devlist, char *homehost,
 		}
 		tmpdev->container = 0;
 		dfd = dev_open(devname, O_RDONLY|O_EXCL);
-		if (dfd < 0) {
+		if (!is_fd_valid(dfd)) {
 			dprintf("cannot open device %s: %s\n",
 				devname, strerror(errno));
 			tmpdev->used = 2;
@@ -7069,8 +7061,8 @@ count_volumes_list(struct md_list *devlist, char *homehost,
 				tmpdev->used = 2;
 			}
 		}
-		if (dfd >= 0)
-			close(dfd);
+		close_fd(&dfd);
+
 		if (tmpdev->used == 2 || tmpdev->used == 4) {
 			/* Ignore unrecognised devices during auto-assembly */
 			goto loop;
@@ -7643,26 +7635,26 @@ static int validate_geometry_imsm(struct supertype *st, int level, int layout,
 
 	/* This device needs to be a device in an 'imsm' container */
 	fd = open(dev, O_RDONLY|O_EXCL, 0);
-	if (fd >= 0) {
-		if (verbose)
-			pr_err("Cannot create this array on device %s\n",
-			       dev);
+
+	if (is_fd_valid(fd)) {
+		pr_vrb("Cannot create this array on device %s\n", dev);
 		close(fd);
 		return 0;
 	}
-	if (errno != EBUSY || (fd = open(dev, O_RDONLY, 0)) < 0) {
-		if (verbose)
-			pr_err("Cannot open %s: %s\n",
-				dev, strerror(errno));
+	if (errno == EBUSY)
+		fd = open(dev, O_RDONLY, 0);
+
+	if (!is_fd_valid(fd)) {
+		pr_vrb("Cannot open %s: %s\n", dev, strerror(errno));
 		return 0;
 	}
+
 	/* Well, it is in use by someone, maybe an 'imsm' container. */
 	cfd = open_container(fd);
-	close(fd);
-	if (cfd < 0) {
-		if (verbose)
-			pr_err("Cannot use %s: It is busy\n",
-				dev);
+	close_fd(&fd);
+
+	if (!is_fd_valid(cfd)) {
+		pr_vrb("Cannot use %s: It is busy\n", dev);
 		return 0;
 	}
 	sra = sysfs_read(cfd, NULL, GET_VERSION);
@@ -8937,7 +8929,7 @@ static struct dl *imsm_add_spare(struct intel_super *super, int slot,
 	for (dl = super->disks; dl; dl = dl->next) {
 		/* If in this array, skip */
 		for (d = a->info.devs ; d ; d = d->next)
-			if (d->state_fd >= 0 &&
+			if (is_fd_valid(d->state_fd) &&
 			    d->disk.major == dl->major &&
 			    d->disk.minor == dl->minor) {
 				dprintf("%x:%x already in array\n",
@@ -9097,13 +9089,15 @@ static struct mdinfo *imsm_activate_spare(struct active_array *a,
 	int i;
 	int allowed;
 
-	for (d = a->info.devs ; d ; d = d->next) {
-		if ((d->curr_state & DS_FAULTY) &&
-			d->state_fd >= 0)
+	for (d = a->info.devs ; d; d = d->next) {
+		if (!is_fd_valid(d->state_fd))
+			continue;
+
+		if (d->curr_state & DS_FAULTY)
 			/* wait for Removal to happen */
 			return NULL;
-		if (d->state_fd >= 0)
-			failed--;
+
+		failed--;
 	}
 
 	dprintf("imsm: activate spare: inst=%d failed=%d (%d) level=%d\n",
@@ -9159,7 +9153,7 @@ static struct mdinfo *imsm_activate_spare(struct active_array *a,
 			if (d->disk.raid_disk == i)
 				break;
 		dprintf("found %d: %p %x\n", i, d, d?d->curr_state:0);
-		if (d && (d->state_fd >= 0))
+		if (d && is_fd_valid(d->state_fd))
 			continue;
 
 		/*
@@ -10893,26 +10887,22 @@ int save_backup_imsm(struct supertype *st,
 {
 	int rv = -1;
 	struct intel_super *super = st->sb;
-	unsigned long long *target_offsets;
-	int *targets;
 	int i;
 	struct imsm_map *map_dest = get_imsm_map(dev, MAP_0);
 	int new_disks = map_dest->num_members;
 	int dest_layout = 0;
-	int dest_chunk;
-	unsigned long long start;
+	int dest_chunk, targets[new_disks];
+	unsigned long long start, target_offsets[new_disks];
 	int data_disks = imsm_num_data_members(map_dest);
 
-	targets = xmalloc(new_disks * sizeof(int));
-
 	for (i = 0; i < new_disks; i++) {
 		struct dl *dl_disk = get_imsm_dl_disk(super, i);
-
-		targets[i] = dl_disk->fd;
+		if (dl_disk && is_fd_valid(dl_disk->fd))
+			targets[i] = dl_disk->fd;
+		else
+			goto abort;
 	}
 
-	target_offsets = xcalloc(new_disks, sizeof(unsigned long long));
-
 	start = info->reshape_progress * 512;
 	for (i = 0; i < new_disks; i++) {
 		target_offsets[i] = migr_chkp_area_pba(super->migr_rec) * 512;
@@ -10944,11 +10934,6 @@ int save_backup_imsm(struct supertype *st,
 	rv = 0;
 
 abort:
-	if (targets) {
-		free(targets);
-	}
-	free(target_offsets);
-
 	return rv;
 }
 
@@ -11068,7 +11053,7 @@ int recover_backup_imsm(struct supertype *st, struct mdinfo *info)
 		if (dl_disk->index < 0)
 			continue;
 
-		if (dl_disk->fd < 0) {
+		if (!is_fd_valid(dl_disk->fd)) {
 			skipped_disks++;
 			continue;
 		}
@@ -11992,7 +11977,7 @@ int wait_for_reshape_imsm(struct mdinfo *sra, int ndata)
 	unsigned long long to_complete = sra->reshape_progress;
 	unsigned long long position_to_set = to_complete / ndata;
 
-	if (fd < 0) {
+	if (!is_fd_valid(fd)) {
 		dprintf("cannot open reshape_position\n");
 		return 1;
 	}
@@ -12079,6 +12064,7 @@ int check_degradation_change(struct mdinfo *info,
 				continue;
 			if (sd->disk.state & (1<<MD_DISK_SYNC)) {
 				char sbuf[100];
+				int raid_disk = sd->disk.raid_disk;
 
 				if (sysfs_get_str(info,
 					sd, "state", sbuf, sizeof(sbuf)) < 0 ||
@@ -12086,13 +12072,8 @@ int check_degradation_change(struct mdinfo *info,
 					strstr(sbuf, "in_sync") == NULL) {
 					/* this device is dead */
 					sd->disk.state = (1<<MD_DISK_FAULTY);
-					if (sd->disk.raid_disk >= 0 &&
-					    sources[sd->disk.raid_disk] >= 0) {
-						close(sources[
-							sd->disk.raid_disk]);
-						sources[sd->disk.raid_disk] =
-							-1;
-					}
+					if (raid_disk >= 0)
+						close_fd(&sources[raid_disk]);
 					new_degraded++;
 				}
 			}
@@ -12483,9 +12464,10 @@ static int validate_internal_bitmap_for_drive(struct supertype *st,
 		return -1;
 
 	fd = d->fd;
-	if (fd < 0) {
+	if (!is_fd_valid(fd)) {
 		fd = open(d->devname, O_RDONLY, 0);
-		if (fd < 0) {
+
+		if (!is_fd_valid(fd)) {
 			dprintf("cannot open the device %s\n", d->devname);
 			goto abort;
 		}
@@ -12509,8 +12491,9 @@ static int validate_internal_bitmap_for_drive(struct supertype *st,
 
 	ret = 0;
 abort:
-	if ((d->fd < 0) && (fd >= 0))
-		close(fd);
+	if (!is_fd_valid(d->fd))
+		close_fd(&fd);
+
 	if (read_buf)
 		free(read_buf);