[PATCH] block: update add_partition() error handling

[PATCH] block: update add_partition() error handling

[Tejun Heo]

add_partition() had some number of holes in the error handling path.
Update it.

* make add_partition() return error code on failure and report failure
  from caller

* make add_partition() check whether the specified slot is already
  occupied instead of doing that in caller

* make add_partition() check error return from device_add()

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 block/ioctl.c         |   17 +++++++++--------
 fs/partitions/check.c |   44 ++++++++++++++++++++++++++++++++++----------
 include/linux/genhd.h |    2 +-
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index f0178c4..ecbdd74 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -16,7 +16,7 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 	struct blkpg_partition p;
 	long long start, length;
 	int part;
-	int i;
+	int i, rc;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -42,12 +42,9 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				    || pstart < 0 || plength < 0)
 					return -EINVAL;
 			}
-			/* partition number in use? */
+
 			mutex_lock(&bdev->bd_mutex);
-			if (disk->part[part - 1]) {
-				mutex_unlock(&bdev->bd_mutex);
-				return -EBUSY;
-			}
+
 			/* overlap? */
 			for (i = 0; i < disk->minors - 1; i++) {
 				struct hd_struct *s = disk->part[i];
@@ -60,10 +57,14 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 					return -EBUSY;
 				}
 			}
+
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			rc = add_partition(disk, part, start, length,
+					   ADDPART_FLAG_NONE);
+
 			mutex_unlock(&bdev->bd_mutex);
-			return 0;
+			return rc;
+
 		case BLKPG_DEL_PARTITION:
 			if (!disk->part[part-1])
 				return -ENXIO;
diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index 3e0bf3f..b75ca08 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -342,19 +342,24 @@ static ssize_t whole_disk_show(struct device *dev,
 static DEVICE_ATTR(whole_disk, S_IRUSR | S_IRGRP | S_IROTH,
 		   whole_disk_show, NULL);
 
-void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags)
+int add_partition(struct gendisk *disk, int part,
+		  sector_t start, sector_t len, int flags)
 {
-	struct hd_struct *p;
+	struct hd_struct *p = NULL;
 	int err;
 
+	if (disk->part[part - 1]) {
+		err = -EBUSY;
+		goto fail;
+	}
+
+	err = -ENOMEM;
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		goto fail;
+	if (!init_part_stats(p))
+		goto fail;
 
-	if (!init_part_stats(p)) {
-		kfree(p);
-		return;
-	}
 	p->start_sect = start;
 	p->nr_sects = len;
 	p->partno = part;
@@ -376,15 +381,29 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 
 	/* delay uevent until 'holders' subdir is created */
 	p->dev.uevent_suppress = 1;
-	device_add(&p->dev);
+	err = device_add(&p->dev);
+	if (err)
+		goto fail;
+
 	partition_sysfs_add_subdir(p);
 	p->dev.uevent_suppress = 0;
-	if (flags & ADDPART_FLAG_WHOLEDISK)
+	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		err = device_create_file(&p->dev, &dev_attr_whole_disk);
+		printk(" %s: failed to create sysfs attrs (%d)\n",
+		       dev_name(&p->dev), err);
+	}
 
 	/* suppress uevent if the disk supresses it */
 	if (!disk->dev.uevent_suppress)
 		kobject_uevent(&p->dev.kobj, KOBJ_ADD);
+
+	return 0;
+
+ fail:
+	if (p)
+		free_part_stats(p);
+	kfree(p);
+	return err;
 }
 
 /* Not exported, helper to add_disk(). */
@@ -484,7 +503,12 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
 		}
-		add_partition(disk, p, from, size, state->parts[p].flags);
+		res = add_partition(disk, p, from, size, state->parts[p].flags);
+		if (res) {
+			printk(" %s: failed to add p%d (%d)\n",
+			       disk->disk_name, p, res);
+			continue;
+		}
 #ifdef CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags & ADDPART_FLAG_RAID)
 			md_autodetect_dev(bdev->bd_dev+p);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index ae7aec3..e485cf0 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -529,7 +529,7 @@ extern dev_t blk_lookup_devt(const char *name, int part);
 extern char *disk_name (struct gendisk *hd, int part, char *buf);
 
 extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev);
-extern void add_partition(struct gendisk *, int, sector_t, sector_t, int);
+extern int add_partition(struct gendisk *, int, sector_t, sector_t, int);
 extern void delete_partition(struct gendisk *, int);
 extern void printk_all_partitions(void);
 
-- 
1.5.4.5

[PATCHSET 1/3 blk-for-2.6.28] block: misc fixes and improvements

[Tejun Heo]

Hello,

This patchset contains the following five patches containing misc
fixes and improvements.

  0001-block-fix-partition-info-printouts.patch
  0002-block-don-t-grab-block_class_lock-unnecessarily.patch
  0003-block-use-class_dev_iterator-instead-of-class_for_e.patch
  0004-block-allow-deleting-zero-length-partition.patch
  0005-block-update-add_partition-error-handling.patch

Recent block_class iteration updates 5c6f35c5..27f3025 introduced a
few bugs and trivial user visible behavior change.  0001-0003 update
those issues and simplify related code using new class iterator.

0004 fixes deletion of zero sized partitions.

d805dda4 tried to fix error handling path in add_partition() but had a
few problems.  0005 fixes those and moves busy partition check into
add_partition().

This patchset is against

  blk-for-2.6.28 (9abd7c437c02e7448fb1d2d3cfc0b9c1ab77cf2d)
+ [1] klist-dont-iterate-over-deleted-entries
+ [2] use-klist-for-class-device-list-and-implement-iterator

and available in the following git tree.

  http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=block-misc-fixes
  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-misc-fixes

with the following combined diffstat

 block/genhd.c         |  246 +++++++++++++++++++-------------------------------
 block/ioctl.c         |    9 -
 fs/partitions/check.c |   42 ++++----
 3 files changed, 121 insertions(+), 176 deletions(-)

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.kernel/725706
[2] http://article.gmane.org/gmane.linux.kernel/725708

[PATCH] block: update add_partition() error handling

[Tejun Heo]

d805dda4 tried to fix error case handling in add_partition() but had a
few problems.

* disk->part[] entry is set early and left dangling if operation
  fails.

* Once device initialized, the last put_device() is responsible for
  freeing all the resources.  The failure path freed part_stats and p
  regardless of put_device() causing double free.

* holders subdir holds reference to the disk device, so failure path
  should remove it to release resources properly which was missing.

This patch fixes the above problems and while at it move partition
slot busy check into add_partition() for completeness and inlines
holders subdirectory creation.  Using separate function for it just
obfuscates the code.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Abdel Benamrouche <draconux@gmail.com>
---
 block/ioctl.c         |    7 ++-----
 fs/partitions/check.c |   40 +++++++++++++++++++++-------------------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index c722de0..eb046ae 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -43,12 +43,9 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				    || pstart < 0 || plength < 0)
 					return -EINVAL;
 			}
-			/* partition number in use? */
+
 			mutex_lock(&bdev->bd_mutex);
-			if (disk->part[part - 1]) {
-				mutex_unlock(&bdev->bd_mutex);
-				return -EBUSY;
-			}
+
 			/* overlap? */
 			for (i = 0; i < disk->minors - 1; i++) {
 				struct hd_struct *s = disk->part[i];
diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index 1b1808e..43dbfab 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -300,15 +300,6 @@ struct device_type part_type = {
 	.release	= part_release,
 };
 
-static inline void partition_sysfs_add_subdir(struct hd_struct *p)
-{
-	struct kobject *k;
-
-	k = kobject_get(&p->dev.kobj);
-	p->holder_dir = kobject_create_and_add("holders", k);
-	kobject_put(k);
-}
-
 static inline void disk_sysfs_add_subdirs(struct gendisk *disk)
 {
 	struct kobject *k;
@@ -347,13 +338,16 @@ int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	struct hd_struct *p;
 	int err;
 
+	if (disk->part[part - 1])
+		return -EBUSY;
+
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
 	if (!init_part_stats(p)) {
 		err = -ENOMEM;
-		goto out0;
+		goto out_free;
 	}
 	p->start_sect = start;
 	p->nr_sects = len;
@@ -372,34 +366,42 @@ int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->dev.class = &block_class;
 	p->dev.type = &part_type;
 	p->dev.parent = &disk->dev;
-	disk->part[part-1] = p;
 
 	/* delay uevent until 'holders' subdir is created */
 	p->dev.uevent_suppress = 1;
 	err = device_add(&p->dev);
 	if (err)
-		goto out1;
-	partition_sysfs_add_subdir(p);
+		goto out_put;
+
+	err = -ENOMEM;
+	p->holder_dir = kobject_create_and_add("holders", &p->dev.kobj);
+	if (!p->holder_dir)
+		goto out_del;
+
 	p->dev.uevent_suppress = 0;
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		err = device_create_file(&p->dev, &dev_attr_whole_disk);
 		if (err)
-			goto out2;
+			goto out_del;
 	}
 
+	/* everything is up and running, commence */
+	disk->part[part - 1] = p;
+
 	/* suppress uevent if the disk supresses it */
 	if (!disk->dev.uevent_suppress)
 		kobject_uevent(&p->dev.kobj, KOBJ_ADD);
 
 	return 0;
 
-out2:
+out_free:
+	kfree(p);
+	return err;
+out_del:
+	kobject_put(p->holder_dir);
 	device_del(&p->dev);
-out1:
+out_put:
 	put_device(&p->dev);
-	free_part_stats(p);
-out0:
-	kfree(p);
 	return err;
 }
 
-- 
1.5.4.5