[PATCH 0/8] mdadm static checker fixes

[PATCH 0/8] mdadm static checker fixes

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

I have been running mdadm through Coverity and fixed a number of
issues that were raised in the scan. A couple of them were non issues
related to conditions where we know for sure the kernel will not
return strings longer than a given size, but there were also a number
of potential memory leaks and buffer overflows.

These patches are sitting in my pending queue. If you are on the CC
list, would you mind having a look at the portions touching code you
previously pushed wrote.

Please hollor if you notice I did anything wrong, otherwise I'll push
this set into git within the next couple of days.

Cheers,
Jes


Jes Sorensen (8):
  Manage: Manage_add(): Fix memory leak
  load_sys(): Add a buffer size argument
  Grow: Grow_continue_command() remove dead code
  Grow: Grow_addbitmap(): Add check to quiet down static code checkers
  {platform,super}-intel: Fix two resource leaks
  bitmap: Fix resource leak in bitmap_file_open()
  Manage: Manage_subdevs() fix file descriptor leak
  super1: Fix potential buffer overflows when copying cluster_name

 Detail.c         |  2 +-
 Grow.c           | 11 ++++++++---
 Manage.c         |  8 ++++++--
 bitmap.c         |  1 +
 mdadm.h          |  2 +-
 platform-intel.c |  7 ++++++-
 super-intel.c    | 12 ++++++++----
 super1.c         | 18 ++++++++++++------
 sysfs.c          | 47 ++++++++++++++++++++++++-----------------------
 9 files changed, 67 insertions(+), 41 deletions(-)

-- 
2.5.0

[PATCH 1/8] Manage: Manage_add(): Fix memory leak

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

sysfs_read() allocates and populates a struct mdinfo, however the code
forgot to free it again, before dropping the reference to the pointer.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Manage.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Manage.c b/Manage.c
index a812ba0..9d2e0d0 100644
--- a/Manage.c
+++ b/Manage.c
@@ -944,10 +944,13 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 		}
 
 		if (strncmp(mdp->sysfs_array_state, "readonly", 8) != 0) {
+			sysfs_free(mdp);
 			pr_err("%s is not readonly, cannot add journal.\n", devname);
 			return -1;
 		}
 
+		sysfs_free(mdp);
+
 		tst->ss->getinfo_super(tst, &mdi, NULL);
 		if (mdi.journal_device_required == 0) {
 			pr_err("%s does not support journal device.\n", devname);
-- 
2.5.0

[PATCH 2/8] load_sys(): Add a buffer size argument

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This adds a buffer size argument to load_sys(), rather than relying on
a hard coded buffer size. The old behavior was safe because we knew
the kernel would never return strings overrunning the buffers, however
it was ugly, and would cause code checking tools to spit out warnings.

This caused a Coverity warning over the read into
sra->sysfs_array_state which is only 20 bytes.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Detail.c      |  2 +-
 mdadm.h       |  2 +-
 super-intel.c |  6 +++---
 sysfs.c       | 47 ++++++++++++++++++++++++-----------------------
 4 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/Detail.c b/Detail.c
index 0cfccad..20c4553 100644
--- a/Detail.c
+++ b/Detail.c
@@ -582,7 +582,7 @@ This is pretty boring
 					continue;
 				sprintf(path, "/sys/block/%s/md/metadata_version",
 					de->d_name);
-				if (load_sys(path, vbuf) < 0)
+				if (load_sys(path, vbuf, sizeof(vbuf)) < 0)
 					continue;
 				if (strncmp(vbuf, "external:", 9) != 0 ||
 				    !is_subarray(vbuf+9) ||
diff --git a/mdadm.h b/mdadm.h
index 97aac52..3b96076 100755
--- a/mdadm.h
+++ b/mdadm.h
@@ -631,7 +631,7 @@ extern int sysfs_disk_to_scsi_id(int fd, __u32 *id);
 extern int sysfs_unique_holder(char *devnm, long rdev);
 extern int sysfs_freeze_array(struct mdinfo *sra);
 extern int sysfs_wait(int fd, int *msec);
-extern int load_sys(char *path, char *buf);
+extern int load_sys(char *path, char *buf, int len);
 extern int reshape_prepare_fdlist(char *devname,
 				  struct mdinfo *sra,
 				  int raid_disks,
diff --git a/super-intel.c b/super-intel.c
index ff0506d..158f4e8 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1654,7 +1654,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 			break;
 		}
 		sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
-		if (load_sys(device, buf) != 0) {
+		if (load_sys(device, buf, sizeof(buf)) != 0) {
 			if (verbose > 0)
 				pr_err("failed to read device type for %s\n",
 					path);
@@ -1669,7 +1669,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 			vendor[0] = '\0';
 			model[0] = '\0';
 			sprintf(device, "/sys/dev/block/%d:%d/device/vendor", major, minor);
-			if (load_sys(device, buf) == 0) {
+			if (load_sys(device, buf, sizeof(buf)) == 0) {
 				strncpy(vendor, buf, sizeof(vendor));
 				vendor[sizeof(vendor) - 1] = '\0';
 				c = (char *) &vendor[sizeof(vendor) - 1];
@@ -1678,7 +1678,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 
 			}
 			sprintf(device, "/sys/dev/block/%d:%d/device/model", major, minor);
-			if (load_sys(device, buf) == 0) {
+			if (load_sys(device, buf, sizeof(buf)) == 0) {
 				strncpy(model, buf, sizeof(model));
 				model[sizeof(model) - 1] = '\0';
 				c = (char *) &model[sizeof(model) - 1];
diff --git a/sysfs.c b/sysfs.c
index 2600343..8379ca8 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -27,15 +27,15 @@
 #include	<dirent.h>
 #include	<ctype.h>
 
-int load_sys(char *path, char *buf)
+int load_sys(char *path, char *buf, int len)
 {
 	int fd = open(path, O_RDONLY);
 	int n;
 	if (fd < 0)
 		return -1;
-	n = read(fd, buf, 1024);
+	n = read(fd, buf, len);
 	close(fd);
-	if (n <0 || n >= 1024)
+	if (n <0 || n >= len)
 		return -1;
 	buf[n] = 0;
 	if (n && buf[n-1] == '\n')
@@ -118,7 +118,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 	sra->devs = NULL;
 	if (options & GET_VERSION) {
 		strcpy(base, "metadata_version");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		if (strncmp(buf, "none", 4) == 0) {
 			sra->array.major_version =
@@ -137,31 +137,31 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 	}
 	if (options & GET_LEVEL) {
 		strcpy(base, "level");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->array.level = map_name(pers, buf);
 	}
 	if (options & GET_LAYOUT) {
 		strcpy(base, "layout");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->array.layout = strtoul(buf, NULL, 0);
 	}
 	if (options & GET_DISKS) {
 		strcpy(base, "raid_disks");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->array.raid_disks = strtoul(buf, NULL, 0);
 	}
 	if (options & GET_DEGRADED) {
 		strcpy(base, "degraded");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->array.failed_disks = strtoul(buf, NULL, 0);
 	}
 	if (options & GET_COMPONENT) {
 		strcpy(base, "component_size");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->component_size = strtoull(buf, NULL, 0);
 		/* sysfs reports "K", but we want sectors */
@@ -169,13 +169,13 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 	}
 	if (options & GET_CHUNK) {
 		strcpy(base, "chunk_size");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->array.chunk_size = strtoul(buf, NULL, 0);
 	}
 	if (options & GET_CACHE) {
 		strcpy(base, "stripe_cache_size");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			/* Probably level doesn't support it */
 			sra->cache_size = 0;
 		else
@@ -183,7 +183,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 	}
 	if (options & GET_MISMATCH) {
 		strcpy(base, "mismatch_cnt");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		sra->mismatch_cnt = strtoul(buf, NULL, 0);
 	}
@@ -195,7 +195,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 		size_t len;
 
 		strcpy(base, "safe_mode_delay");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 
 		/* remove a period, and count digits after it */
@@ -218,7 +218,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 	}
 	if (options & GET_BITMAP_LOCATION) {
 		strcpy(base, "bitmap/location");
-		if (load_sys(fname, buf))
+		if (load_sys(fname, buf, sizeof(buf)))
 			goto abort;
 		if (strncmp(buf, "file", 4) == 0)
 			sra->bitmap_offset = 1;
@@ -232,7 +232,8 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 
 	if (options & GET_ARRAY_STATE) {
 		strcpy(base, "array_state");
-		if (load_sys(fname, sra->sysfs_array_state))
+		if (load_sys(fname, sra->sysfs_array_state,
+			     sizeof(sra->sysfs_array_state)))
 			goto abort;
 	} else
 		sra->sysfs_array_state[0] = 0;
@@ -262,7 +263,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 
 		/* Always get slot, major, minor */
 		strcpy(dbase, "slot");
-		if (load_sys(fname, buf)) {
+		if (load_sys(fname, buf, sizeof(buf))) {
 			/* hmm... unable to read 'slot' maybe the device
 			 * is going away?
 			 */
@@ -287,7 +288,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 		if (*ep) dev->disk.raid_disk = -1;
 
 		strcpy(dbase, "block/dev");
-		if (load_sys(fname, buf)) {
+		if (load_sys(fname, buf, sizeof(buf))) {
 			/* assume this is a stale reference to a hot
 			 * removed device
 			 */
@@ -299,7 +300,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 
 		/* special case check for block devices that can go 'offline' */
 		strcpy(dbase, "block/device/state");
-		if (load_sys(fname, buf) == 0 &&
+		if (load_sys(fname, buf, sizeof(buf)) == 0 &&
 		    strncmp(buf, "offline", 7) == 0) {
 			free(dev);
 			continue;
@@ -312,25 +313,25 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 
 		if (options & GET_OFFSET) {
 			strcpy(dbase, "offset");
-			if (load_sys(fname, buf))
+			if (load_sys(fname, buf, sizeof(buf)))
 				goto abort;
 			dev->data_offset = strtoull(buf, NULL, 0);
 			strcpy(dbase, "new_offset");
-			if (load_sys(fname, buf) == 0)
+			if (load_sys(fname, buf, sizeof(buf)) == 0)
 				dev->new_data_offset = strtoull(buf, NULL, 0);
 			else
 				dev->new_data_offset = dev->data_offset;
 		}
 		if (options & GET_SIZE) {
 			strcpy(dbase, "size");
-			if (load_sys(fname, buf))
+			if (load_sys(fname, buf, sizeof(buf)))
 				goto abort;
 			dev->component_size = strtoull(buf, NULL, 0) * 2;
 		}
 		if (options & GET_STATE) {
 			dev->disk.state = 0;
 			strcpy(dbase, "state");
-			if (load_sys(fname, buf))
+			if (load_sys(fname, buf, sizeof(buf)))
 				goto abort;
 			if (strstr(buf, "in_sync"))
 				dev->disk.state |= (1<<MD_DISK_SYNC);
@@ -341,7 +342,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 		}
 		if (options & GET_ERROR) {
 			strcpy(buf, "errors");
-			if (load_sys(fname, buf))
+			if (load_sys(fname, buf, sizeof(buf)))
 				goto abort;
 			dev->errors = strtoul(buf, NULL, 0);
 		}
-- 
2.5.0

[PATCH 3/8] Grow: Grow_continue_command() remove dead code

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

All cases where fd2 is used are completed with a close(fd2) ; fd2 = -1;
so there is no need to check for fd2 > -1 before exiting.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Grow.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Grow.c b/Grow.c
index c4f417e..0fa776d 100755
--- a/Grow.c
+++ b/Grow.c
@@ -4924,8 +4924,6 @@ int Grow_continue_command(char *devname, int fd,
 	ret_val = Grow_continue(fd, st, content, backup_file, 1, 0);
 
 Grow_continue_command_exit:
-	if (fd2 > -1)
-		close(fd2);
 	if (cfd > -1)
 		close(cfd);
 	st->ss->free_super(st);
-- 
2.5.0

[PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
not all static code checkers catch this. This adds a check to quiet
down the false positive warnings.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Grow.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index 0fa776d..c453eb6 100755
--- a/Grow.c
+++ b/Grow.c
@@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
 			"  between different architectures.  Consider upgrading the Linux kernel.\n");
 	}
 
-	if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") == 0)
+	/*
+	 * We only ever get called if s->bitmap_file is != NULL, so this check
+	 * is just here to quiet down static code checkers.
+	 */
+	if (!s->bitmap_file)
+		return 1;
+
+	if (strcmp(s->bitmap_file, "clustered") == 0)
 		major = BITMAP_MAJOR_CLUSTERED;
 
 	if (ioctl(fd, GET_BITMAP_FILE, &bmf) != 0) {
-- 
2.5.0

[PATCH 5/8] {platform,super}-intel: Fix two resource leaks

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

The code did not free 'dir' allocated by opendir(). An additional
benefit is that this simplifies the for() loops.

Fixes: 60f0f54d ("IMSM: Add support for VMD")
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 platform-intel.c | 7 ++++++-
 super-intel.c    | 6 +++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 88818f3..c60fd9e 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
 		return NULL;
 
 	dir = opendir("/sys/bus/pci/drivers/vmd");
+	if (!dir)
+		return NULL;
 
-	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
+	for (ent = readdir(dir); ent; ent = readdir(dir)) {
 		sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
 			ent->d_name);
 
@@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
 
 		if (strncmp(buf, hba->path, strlen(buf)) == 0) {
 			sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
+			closedir(dir);
 			return realpath(path, buf);
 		}
 	}
+
+	closedir(dir);
 	return NULL;
 }
diff --git a/super-intel.c b/super-intel.c
index 158f4e8..e1bee75 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
 	 * this hba
 	 */
 	dir = opendir("/sys/bus/pci/drivers/nvme");
-	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
+	if (!dir)
+		return 1;
+
+	for (ent = readdir(dir); ent; ent = readdir(dir)) {
 		int n;
 
 		/* is 'ent' a device? check that the 'subsystem' link exists and
@@ -1814,6 +1817,7 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
 		free(rp);
 	}
 
+	closedir(dir);
 	return 0;
 }
 
-- 
2.5.0

[PATCH 6/8] bitmap: Fix resource leak in bitmap_file_open()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

The code would leak 'fd' if locate_bitmap() failed.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 bitmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/bitmap.c b/bitmap.c
index 5ad7401..0367d13 100644
--- a/bitmap.c
+++ b/bitmap.c
@@ -224,6 +224,7 @@ int bitmap_file_open(char *filename, struct supertype **stp, int node_num)
 		} else {
 			if (st->ss->locate_bitmap(st, fd, node_num)) {
 				pr_err("%s doesn't have bitmap\n", filename);
+				close(fd);
 				fd = -1;
 			}
 		}
-- 
2.5.0

[PATCH 7/8] Manage: Manage_subdevs() fix file descriptor leak

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Manage.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Manage.c b/Manage.c
index 9d2e0d0..808c2cb 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1511,9 +1511,10 @@ int Manage_subdevs(char *devname, int fd,
 		} else {
 			struct stat stb;
 			tfd = dev_open(dv->devname, O_RDONLY);
-			if (tfd >= 0)
+			if (tfd >= 0) {
 				fstat(tfd, &stb);
-			else {
+				close(tfd);
+			} else {
 				int open_err = errno;
 				if (stat(dv->devname, &stb) != 0) {
 					pr_err("Cannot find %s: %s\n",
-- 
2.5.0

[PATCH 8/8] super1: Fix potential buffer overflows when copying cluster_name

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

cmap_get_string() used to retrieve cluster_name does not restrict it's
size. To prevent buffer overflows use the size of the destination
buffer, not strlen() of the source, and null terminate the copied
string.

Fixes: 0aa2f15b ("mdadm: add the ability to change cluster name)"
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 super1.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/super1.c b/super1.c
index f8a35ac..baa9a96 100644
--- a/super1.c
+++ b/super1.c
@@ -2201,6 +2201,7 @@ add_internal_bitmap1(struct supertype *st,
 	unsigned long long chunk = *chunkp;
 	int room = 0;
 	int creating = 0;
+	int len;
 	struct mdp_superblock_1 *sb = st->sb;
 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
 	int uuid[4];
@@ -2326,9 +2327,11 @@ add_internal_bitmap1(struct supertype *st,
 	if (st->nodes)
 		sb->feature_map = __cpu_to_le32(__le32_to_cpu(sb->feature_map)
 						| MD_FEATURE_BITMAP_VERSIONED);
-	if (st->cluster_name)
-		strncpy((char *)bms->cluster_name,
-			st->cluster_name, strlen(st->cluster_name));
+	if (st->cluster_name) {
+		len = sizeof(bms->cluster_name);
+		strncpy((char *)bms->cluster_name, st->cluster_name, len);
+		bms->cluster_name[len - 1] = '\0';
+	}
 
 	*chunkp = chunk;
 	return 1;
@@ -2366,7 +2369,7 @@ static int write_bitmap1(struct supertype *st, int fd, enum bitmap_update update
 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb)+MAX_SB_SIZE);
 	int rv = 0;
 	void *buf;
-	int towrite, n;
+	int towrite, n, len;
 	struct align_fd afd;
 	unsigned int i = 0;
 	unsigned long long total_bm_space, bm_space_per_node;
@@ -2375,8 +2378,11 @@ static int write_bitmap1(struct supertype *st, int fd, enum bitmap_update update
 	case NameUpdate:
 		/* update cluster name */
 		if (st->cluster_name) {
-			memset((char *)bms->cluster_name, 0, sizeof(bms->cluster_name));
-			strncpy((char *)bms->cluster_name, st->cluster_name, 64);
+			len = sizeof(bms->cluster_name);
+			memset((char *)bms->cluster_name, 0, len);
+			strncpy((char *)bms->cluster_name,
+				st->cluster_name, len);
+			bms->cluster_name[len - 1] = '\0';
 		}
 		break;
 	case NodeNumUpdate:
-- 
2.5.0

Re: [PATCH 3/8] Grow: Grow_continue_command() remove dead code

[NeilBrown]

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

On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> All cases where fd2 is used are completed with a close(fd2) ; fd2 = -1;
> so there is no need to check for fd2 > -1 before exiting.

In that case, you can remove all the assignments so -1 to fd2 - if you
like.

Thanks,
NeilBrown

>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  Grow.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index c4f417e..0fa776d 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -4924,8 +4924,6 @@ int Grow_continue_command(char *devname, int fd,
>  	ret_val = Grow_continue(fd, st, content, backup_file, 1, 0);
>  
>  Grow_continue_command_exit:
> -	if (fd2 > -1)
> -		close(fd2);
>  	if (cfd > -1)
>  		close(cfd);
>  	st->ss->free_super(st);
> -- 
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks

[NeilBrown]

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

On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> The code did not free 'dir' allocated by opendir(). An additional
> benefit is that this simplifies the for() loops.
>
> Fixes: 60f0f54d ("IMSM: Add support for VMD")
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  platform-intel.c | 7 ++++++-
>  super-intel.c    | 6 +++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/platform-intel.c b/platform-intel.c
> index 88818f3..c60fd9e 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>  		return NULL;
>  
>  	dir = opendir("/sys/bus/pci/drivers/vmd");
> +	if (!dir)
> +		return NULL;
>  
> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
>  		sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
>  			ent->d_name);
>  
> @@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>  
>  		if (strncmp(buf, hba->path, strlen(buf)) == 0) {
>  			sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
> +			closedir(dir);
>  			return realpath(path, buf);
>  		}
>  	}
> +
> +	closedir(dir);
>  	return NULL;
>  }
> diff --git a/super-intel.c b/super-intel.c
> index 158f4e8..e1bee75 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
>  	 * this hba
>  	 */
>  	dir = opendir("/sys/bus/pci/drivers/nvme");
> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> +	if (!dir)
> +		return 1;
> +

Returning '1' looks really weird here.  I can see it is consistent with
	if (hba->type != SYS_DEV_VMD)
		return 1;

above, but still....
As the return value is never used, should we just make it 'void' ??

Thanks,
NeilBrown

> +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
>  		int n;
>  
>  		/* is 'ent' a device? check that the 'subsystem' link exists and
> @@ -1814,6 +1817,7 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
>  		free(rp);
>  	}
>  
> +	closedir(dir);
>  	return 0;
>  }
>  
> -- 
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 6/8] bitmap: Fix resource leak in bitmap_file_open()

[NeilBrown]

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

On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> The code would leak 'fd' if locate_bitmap() failed.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  bitmap.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/bitmap.c b/bitmap.c
> index 5ad7401..0367d13 100644
> --- a/bitmap.c
> +++ b/bitmap.c
> @@ -224,6 +224,7 @@ int bitmap_file_open(char *filename, struct supertype **stp, int node_num)
>  		} else {
>  			if (st->ss->locate_bitmap(st, fd, node_num)) {
>  				pr_err("%s doesn't have bitmap\n", filename);
> +				close(fd);
>  				fd = -1;
>  			}
>  		}

Don't you also need a 'close' in

		} else if (!st->ss->locate_bitmap) {
			pr_err("No bitmap possible with %s metadata\n",
				st->ss->name);
			return -1;
??

Thanks,
NeilBrown

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

Re: [PATCH 0/8] mdadm static checker fixes

[NeilBrown]

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

On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Hi,
>
> I have been running mdadm through Coverity and fixed a number of
> issues that were raised in the scan. A couple of them were non issues
> related to conditions where we know for sure the kernel will not
> return strings longer than a given size, but there were also a number
> of potential memory leaks and buffer overflows.
>
> These patches are sitting in my pending queue. If you are on the CC
> list, would you mind having a look at the portions touching code you
> previously pushed wrote.
>
> Please hollor if you notice I did anything wrong, otherwise I'll push
> this set into git within the next couple of days.

All
  Reviewed-by: NeilBrown <neilb@suse.com>

The comments I have made are only possible enhancements, no problems
found.

I must confess that I was generally fairly careless about resource
leakage.  mdadm usually calls 'exit' fairly soon and that releases
everything.
But mdmon and "mdadm --monitor" at long-running so it can pay to be
careful.
And I'm very supportive of silencing warnings from tools that also
provide useful warnings.

Thanks!

NeilBrown

>
> Cheers,
> Jes
>
>
> Jes Sorensen (8):
>   Manage: Manage_add(): Fix memory leak
>   load_sys(): Add a buffer size argument
>   Grow: Grow_continue_command() remove dead code
>   Grow: Grow_addbitmap(): Add check to quiet down static code checkers
>   {platform,super}-intel: Fix two resource leaks
>   bitmap: Fix resource leak in bitmap_file_open()
>   Manage: Manage_subdevs() fix file descriptor leak
>   super1: Fix potential buffer overflows when copying cluster_name
>
>  Detail.c         |  2 +-
>  Grow.c           | 11 ++++++++---
>  Manage.c         |  8 ++++++--
>  bitmap.c         |  1 +
>  mdadm.h          |  2 +-
>  platform-intel.c |  7 ++++++-
>  super-intel.c    | 12 ++++++++----
>  super1.c         | 18 ++++++++++++------
>  sysfs.c          | 47 ++++++++++++++++++++++++-----------------------
>  9 files changed, 67 insertions(+), 41 deletions(-)
>
> -- 
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers

[Jes Sorensen]

Guoqing Jiang <gqjiang@suse.com> writes:
> On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
>> not all static code checkers catch this. This adds a check to quiet
>> down the false positive warnings.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>   Grow.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 0fa776d..c453eb6 100755
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>>   			"  between different architectures.  Consider upgrading the Linux kernel.\n");
>>   	}
>>   -	if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") ==
>> 0)
>> +	/*
>> +	 * We only ever get called if s->bitmap_file is != NULL, so this check
>> +	 * is just here to quiet down static code checkers.
>> +	 */
>> +	if (!s->bitmap_file)
>> +		return 1;
>
> Is it really need to make all static code checkers happy? ;-)
> Otherwise, I would prefer remove above check.
>
> Anyway, I am fine with the changes.

We had a check in one place, but not in the remaining places. I just
made it more consistent.

Making the code checker happy does make some sense because it finds
valid bugs too, and they are easier to find when we don't get a lot of
false warnings.

Thanks for your review :)

Cheers,
Jes

Re: [PATCH 3/8] Grow: Grow_continue_command() remove dead code

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> All cases where fd2 is used are completed with a close(fd2) ; fd2 = -1;
>> so there is no need to check for fd2 > -1 before exiting.
>
> In that case, you can remove all the assignments so -1 to fd2 - if you
> like.

Oh good point - I'll make that change.

Thanks,
Jes

>
> Thanks,
> NeilBrown
>
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  Grow.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index c4f417e..0fa776d 100755
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -4924,8 +4924,6 @@ int Grow_continue_command(char *devname, int fd,
>>  	ret_val = Grow_continue(fd, st, content, backup_file, 1, 0);
>>  
>>  Grow_continue_command_exit:
>> -	if (fd2 > -1)
>> -		close(fd2);
>>  	if (cfd > -1)
>>  		close(cfd);
>>  	st->ss->free_super(st);
>> -- 
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> The code did not free 'dir' allocated by opendir(). An additional
>> benefit is that this simplifies the for() loops.
>>
>> Fixes: 60f0f54d ("IMSM: Add support for VMD")
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  platform-intel.c | 7 ++++++-
>>  super-intel.c    | 6 +++++-
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/platform-intel.c b/platform-intel.c
>> index 88818f3..c60fd9e 100644
>> --- a/platform-intel.c
>> +++ b/platform-intel.c
>> @@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>>  		return NULL;
>>  
>>  	dir = opendir("/sys/bus/pci/drivers/vmd");
>> +	if (!dir)
>> +		return NULL;
>>  
>> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
>> +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
>>  		sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
>>  			ent->d_name);
>>  
>> @@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>>  
>>  		if (strncmp(buf, hba->path, strlen(buf)) == 0) {
>>  			sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
>> +			closedir(dir);
>>  			return realpath(path, buf);
>>  		}
>>  	}
>> +
>> +	closedir(dir);
>>  	return NULL;
>>  }
>> diff --git a/super-intel.c b/super-intel.c
>> index 158f4e8..e1bee75 100644
>> --- a/super-intel.c
>> +++ b/super-intel.c
>> @@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
>>  	 * this hba
>>  	 */
>>  	dir = opendir("/sys/bus/pci/drivers/nvme");
>> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
>> +	if (!dir)
>> +		return 1;
>> +
>
> Returning '1' looks really weird here.  I can see it is consistent with
> 	if (hba->type != SYS_DEV_VMD)
> 		return 1;
>
> above, but still....
> As the return value is never used, should we just make it 'void' ??

Seems reasonable - I'll put that in a separate patch.

Cheers,
Jes

Re: [PATCH 6/8] bitmap: Fix resource leak in bitmap_file_open()

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> The code would leak 'fd' if locate_bitmap() failed.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  bitmap.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/bitmap.c b/bitmap.c
>> index 5ad7401..0367d13 100644
>> --- a/bitmap.c
>> +++ b/bitmap.c
>> @@ -224,6 +224,7 @@ int bitmap_file_open(char *filename, struct supertype **stp, int node_num)
>>  		} else {
>>  			if (st->ss->locate_bitmap(st, fd, node_num)) {
>>  				pr_err("%s doesn't have bitmap\n", filename);
>> +				close(fd);
>>  				fd = -1;
>>  			}
>>  		}
>
> Don't you also need a 'close' in
>
> 		} else if (!st->ss->locate_bitmap) {
> 			pr_err("No bitmap possible with %s metadata\n",
> 				st->ss->name);
> 			return -1;
> ??

I probably need new glasses too :)

Thanks for catching that!

Cheers,
Jes

Re: [PATCH 0/8] mdadm static checker fixes

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Hi,
>>
>> I have been running mdadm through Coverity and fixed a number of
>> issues that were raised in the scan. A couple of them were non issues
>> related to conditions where we know for sure the kernel will not
>> return strings longer than a given size, but there were also a number
>> of potential memory leaks and buffer overflows.
>>
>> These patches are sitting in my pending queue. If you are on the CC
>> list, would you mind having a look at the portions touching code you
>> previously pushed wrote.
>>
>> Please hollor if you notice I did anything wrong, otherwise I'll push
>> this set into git within the next couple of days.
>
> All
>   Reviewed-by: NeilBrown <neilb@suse.com>
>
> The comments I have made are only possible enhancements, no problems
> found.
>
> I must confess that I was generally fairly careless about resource
> leakage.  mdadm usually calls 'exit' fairly soon and that releases
> everything.  But mdmon and "mdadm --monitor" at long-running so it can
> pay to be careful.  And I'm very supportive of silencing warnings from
> tools that also provide useful warnings.

Thanks for the feedback - much appreciated!

Cheers,
Jes


> Thanks!
>
> NeilBrown
>
>>
>> Cheers,
>> Jes
>>
>>
>> Jes Sorensen (8):
>>   Manage: Manage_add(): Fix memory leak
>>   load_sys(): Add a buffer size argument
>>   Grow: Grow_continue_command() remove dead code
>>   Grow: Grow_addbitmap(): Add check to quiet down static code checkers
>>   {platform,super}-intel: Fix two resource leaks
>>   bitmap: Fix resource leak in bitmap_file_open()
>>   Manage: Manage_subdevs() fix file descriptor leak
>>   super1: Fix potential buffer overflows when copying cluster_name
>>
>>  Detail.c         |  2 +-
>>  Grow.c           | 11 ++++++++---
>>  Manage.c         |  8 ++++++--
>>  bitmap.c         |  1 +
>>  mdadm.h          |  2 +-
>>  platform-intel.c |  7 ++++++-
>>  super-intel.c    | 12 ++++++++----
>>  super1.c         | 18 ++++++++++++------
>>  sysfs.c          | 47 ++++++++++++++++++++++++-----------------------
>>  9 files changed, 67 insertions(+), 41 deletions(-)
>>
>> -- 
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers

[Guoqing Jiang]

On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
> not all static code checkers catch this. This adds a check to quiet
> down the false positive warnings.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>   Grow.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Grow.c b/Grow.c
> index 0fa776d..c453eb6 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>   			"  between different architectures.  Consider upgrading the Linux kernel.\n");
>   	}
>   
> -	if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") == 0)
> +	/*
> +	 * We only ever get called if s->bitmap_file is != NULL, so this check
> +	 * is just here to quiet down static code checkers.
> +	 */
> +	if (!s->bitmap_file)
> +		return 1;

Is it really need to make all static code checkers happy? ;-)
Otherwise, I would prefer remove above check.

Anyway, I am fine with the changes.

Thanks,
Guoqing

> +
> +	if (strcmp(s->bitmap_file, "clustered") == 0)
>   		major = BITMAP_MAJOR_CLUSTERED;
>   
>   	if (ioctl(fd, GET_BITMAP_FILE, &bmf) != 0) {

Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers

[NeilBrown]

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

On Thu, Mar 10 2016, Jes Sorensen wrote:

> Guoqing Jiang <gqjiang@suse.com> writes:
>> On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
>>> not all static code checkers catch this. This adds a check to quiet
>>> down the false positive warnings.
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>>   Grow.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Grow.c b/Grow.c
>>> index 0fa776d..c453eb6 100755
>>> --- a/Grow.c
>>> +++ b/Grow.c
>>> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>>>   			"  between different architectures.  Consider upgrading the Linux kernel.\n");
>>>   	}
>>>   -	if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") ==
>>> 0)
>>> +	/*
>>> +	 * We only ever get called if s->bitmap_file is != NULL, so this check
>>> +	 * is just here to quiet down static code checkers.
>>> +	 */
>>> +	if (!s->bitmap_file)
>>> +		return 1;
>>
>> Is it really need to make all static code checkers happy? ;-)
>> Otherwise, I would prefer remove above check.
>>
>> Anyway, I am fine with the changes.
>
> We had a check in one place, but not in the remaining places. I just
> made it more consistent.

I wonder if maybe the checker was only complaining because the code was
inconsistent.
i.e. if we just got rid of the existing test on s->bitmap_file, maybe
that would make the checker happy.
It would be interesting to experiment even if you ultimately decide to
leave the new test there.

Thanks,
NeilBrown

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

Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks

[Baldysiak, Pawel]

On Wed, 2016-03-09 at 11:23 -0500, Jes Sorensen wrote:
> NeilBrown <neilb@suse.de> writes:
> > 
> > On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
> > 
> > > 
> > > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> > > 
> > > The code did not free 'dir' allocated by opendir(). An additional
> > > benefit is that this simplifies the for() loops.
> > > 
> > > Fixes: 60f0f54d ("IMSM: Add support for VMD")
> > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> > > ---
> > >  platform-intel.c | 7 ++++++-
> > >  super-intel.c    | 6 +++++-
> > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/platform-intel.c b/platform-intel.c
> > > index 88818f3..c60fd9e 100644
> > > --- a/platform-intel.c
> > > +++ b/platform-intel.c
> > > @@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
> > >  		return NULL;
> > >  
> > >  	dir = opendir("/sys/bus/pci/drivers/vmd");
> > > +	if (!dir)
> > > +		return NULL;
> > >  
> > > -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> > > +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
> > >  		sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
> > >  			ent->d_name);
> > >  
> > > @@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
> > >  
> > >  		if (strncmp(buf, hba->path, strlen(buf)) == 0) {
> > >  			sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
> > > +			closedir(dir);
> > >  			return realpath(path, buf);
> > >  		}
> > >  	}
> > > +
> > > +	closedir(dir);
> > >  	return NULL;
> > >  }
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 158f4e8..e1bee75 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
> > >  	 * this hba
> > >  	 */
> > >  	dir = opendir("/sys/bus/pci/drivers/nvme");
> > > -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> > > +	if (!dir)
> > > +		return 1;
> > > +
> > Returning '1' looks really weird here.  I can see it is consistent with
> > 	if (hba->type != SYS_DEV_VMD)
> > 		return 1;
> > 
> > above, but still....
> > As the return value is never used, should we just make it 'void' ??
> Seems reasonable - I'll put that in a separate patch.
> 
> Cheers,
> Jes
Hello,
Thanks for the fix Jes.

I thinks that instead of making it "void" we can actually check the return value and print a proper message if something goes wrong.
Also "simplified for() loop" can be applied to ahci_enumerate_ports() as well.

@@ -1624,7 +1624,10 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 	 * this hba
 	 */
 	dir = opendir("/sys/dev/block");
-	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
+	if (!dir)
+		return 1;
+
+	for (ent = readdir(dir); ent; ent = readdir(dir)) {
 		int fd;
 		char model[64];
 		char vendor[64];
@@ -2021,7 +2024,11 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
 					print_imsm_capability(&entry->orom);
 					printf(" I/O Controller : %s (%s)\n",
 						vmd_domain_to_controller(hba, buf), get_sys_dev_type(hba->type));
-					print_vmd_attached_devs(hba);
+					if (print_vmd_attached_devs(hba)) {
+						if (verbose > 0)
+							pr_err("failed to get devices attached to VMD domain.\n");
+						result |= 2;
+					}
 					printf("\n");
 				}
 			}

Is that ok with you? Should I prepare a proper patch with this?

Thanks,
Pawel Baldysiak

Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks

[Jes Sorensen]

"Baldysiak, Pawel" <pawel.baldysiak@intel.com> writes:
> On Wed, 2016-03-09 at 11:23 -0500, Jes Sorensen wrote:
>> NeilBrown <neilb@suse.de> writes:
>> > 
>> > On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>> > Returning '1' looks really weird here.  I can see it is consistent with
>> > 	if (hba->type != SYS_DEV_VMD)
>> > 		return 1;
>> > 
>> > above, but still....
>> > As the return value is never used, should we just make it 'void' ??
>> Seems reasonable - I'll put that in a separate patch.
>> 
>> Cheers,
>> Jes
> Hello,
> Thanks for the fix Jes.
>
> I thinks that instead of making it "void" we can actually check the
> return value and print a proper message if something goes wrong.
> Also "simplified for() loop" can be applied to ahci_enumerate_ports() as well.
>
> @@ -1624,7 +1624,10 @@ static int ahci_enumerate_ports(const char
> *hba_path, int port_count, int host_b
>  	 * this hba
>  	 */
>  	dir = opendir("/sys/dev/block");
> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> +	if (!dir)
> +		return 1;
> +
> +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
>  		int fd;
>  		char model[64];
>  		char vendor[64];
> @@ -2021,7 +2024,11 @@ static int detail_platform_imsm(int verbose,
> int enumerate_only, char *controlle
>  					print_imsm_capability(&entry->orom);
>  					printf(" I/O Controller : %s (%s)\n",
>   vmd_domain_to_controller(hba, buf), get_sys_dev_type(hba->type));
> -					print_vmd_attached_devs(hba);
> +					if (print_vmd_attached_devs(hba)) {
> +						if (verbose > 0)
> + pr_err("failed to get devices attached to VMD domain.\n");
> +						result |= 2;
> +					}
>  					printf("\n");
>  				}
>  			}
>
> Is that ok with you? Should I prepare a proper patch with this?

Hi Pawel,

I already pushed this into git, but if you want to send me a patch (or
two) for this, I am happy to apply them. Lets make the
ahci_enumerate_ports() change in a separate patch.

Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Thu, Mar 10 2016, Jes Sorensen wrote:
>
>> Guoqing Jiang <gqjiang@suse.com> writes:
>>> On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
>>>> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>>>>   			"  between different architectures.  Consider upgrading the Linux kernel.\n");
>>>>   	}
>>>>   -	if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") ==
>>>> 0)
>>>> +	/*
>>>> +	 * We only ever get called if s->bitmap_file is != NULL, so this check
>>>> +	 * is just here to quiet down static code checkers.
>>>> +	 */
>>>> +	if (!s->bitmap_file)
>>>> +		return 1;
>>>
>>> Is it really need to make all static code checkers happy? ;-)
>>> Otherwise, I would prefer remove above check.
>>>
>>> Anyway, I am fine with the changes.
>>
>> We had a check in one place, but not in the remaining places. I just
>> made it more consistent.
>
> I wonder if maybe the checker was only complaining because the code
> was inconsistent.  i.e. if we just got rid of the existing test on
> s->bitmap_file, maybe that would make the checker happy.  It would be
> interesting to experiment even if you ultimately decide to leave the
> new test there.

I went back and checked the scan logs and it basically complained about
all the cases that didn't check the validity of s->bitmap_file. I think
it's safer to just have the one check at the top, even if we expect it
never to be called with an invalid pointer.

Cheers,
Jes