add_partition silently ignored errors

PATCH In review Dirk Hohndel 5 participants 18 messages Oct 29, 2007 – Nov 6, 2007 Block layer

Share & tools

Dirk Hohndel

author

2007-10-29 12:23 UTC #0 lore raw

PATCH 1/1 add_partition silently ignored errors

Yet another issue where we ignore errors - this needs someone to make sure that I am passing around the right error codes (and am cleaning up correctly)

[PATCH] add_partition silently ignored errors

Signed-off-by Dirk Hohndel <[email protected]>

---
 block/ioctl.c         |    5 ++++-
 fs/partitions/check.c |   26 ++++++++++++++++++++------
 include/linux/genhd.h |    2 +-
 3 files changed, 25 insertions(+), 8 deletions(-)

block/ioctl.c +4 -1

index 52d6385..bb3933e 100644
@@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) {
+				mutex_unlock(&bdev->bd_mutex);
+				return -EBUSY;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +20 -6

index 722e12e..113ecc0 100644
@@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }
 
-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;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -1;
 	
 	p->start_sect = start;
 	p->nr_sects = len;

@@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	if (kobject_add(&p->kobj)) 
+		return -1;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) {
+		kobject_del(&p->kobj);
+		kfree(p);
+		return -1;
+	}
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		if (sysfs_create_file(&p->kobj, &addpartattr)) {
+			kobject_del(&p->kobj);
+			kfree(p);
+			return -1;
+		}
 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+	
+	return 0;
 }
 
 static char *make_block_name(struct gendisk *disk)

@@ -554,8 +565,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (from + size > get_capacity(disk)) {
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
+			return -EBUSY;
+		}
+		if(add_partition(disk, p, from, size, state->parts[p].flags)) {
+			return -EBUSY;
 		}
-		add_partition(disk, p, from, size, state->parts[p].flags);
 #ifdef CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags & ADDPART_FLAG_RAID)
 			md_autodetect_dev(bdev->bd_dev+p);

include/linux/genhd.h +1 -1

index a47b802..ae6af2c 100644
@@ -414,7 +414,7 @@ struct unixware_disklabel {
 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);

Signature · 1 line

gitgui.0.8.4.g8d863

Cornelia Huck

2007-10-29 13:07 UTC #1 lore raw

On Mon, 29 Oct 2007 05:22:11 -0700,
Dirk Hohndel <[email protected]> wrote:

Quoted 5 lines

fs/partitions/check.c +0 -1

@@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);

fs/partitions/check.c void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 401

Quoted 10 lines

fs/partitions/check.c +7 -1

+	if (kobject_add(&p->kobj)) 
+		return -1;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) {
+		kobject_del(&p->kobj);
+		kfree(p);
+		return -1;
+	}

- This is missing a KOBJ_DEL uevent if you did a KOBJ_ADD uevent.
- Calling kfree() after you already registered the kobject via kobject_add() is wrong, since someone else might already have obtained a reference. You must drop your reference after kobject_del() and let the release mechanism take care of it.

fs/partitions/check.c void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 412

Quoted 12 lines

fs/partitions/check.c +5 -1

 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		if (sysfs_create_file(&p->kobj, &addpartattr)) {
+			kobject_del(&p->kobj);
+			kfree(p);
+			return -1;
+		}

Same here. You should probably also delete the link you created above.

Quoted 8 lines

fs/partitions/check.c +2 -0

 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+	
+	return 0;
 }
 
 static char *make_block_name(struct gendisk *disk)

Dirk Hohndel

author

2007-10-29 14:25 UTC #2 lore raw

Quoted 15 lines · Cornelia Huck

On Mon, 29 Oct 2007 05:22:11 -0700,
Dirk Hohndel <[email protected]> wrote:

Quoted 10 lines

(quoted diff) +2 -2

@@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	if (kobject_add(&p->kobj)) 
+		return -1;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");

Quoted 8 lines

Review void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 401
Quoted 5 lines
(quoted diff) +5 -0
+	if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) {
+		kobject_del(&p->kobj);
+		kfree(p);
+		return -1;
+	}
- This is missing a KOBJ_DEL uevent if you did a KOBJ_ADD uevent.

Completely missed that one.

Quoted 4 lines

- Calling kfree() after you already registered the kobject via kobject_add() is wrong, since someone else might already have obtained a reference. You must drop your reference after kobject_del() and let the release mechanism take care of it.

Good point.

Quoted 9 lines

<I think I'm having some kind of deja vu, since it seems I've already pointed that out a couple of times to different people :) >
Quoted 6 lines
(quoted diff) +0 -0
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 

Quoted 8 lines

Review void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 412
Quoted 6 lines
(quoted diff) +5 -1
-		sysfs_create_file(&p->kobj, &addpartattr);
+		if (sysfs_create_file(&p->kobj, &addpartattr)) {
+			kobject_del(&p->kobj);
+			kfree(p);
+			return -1;
+		}
Same here. You should probably also delete the link you created above.

Yep, fixed that as well.

How about the new patch below?

Signed-off-by Dirk Hohndel <[email protected]>

---
 block/ioctl.c         |    5 ++++-
 fs/partitions/check.c |   28 ++++++++++++++++++++++------
 include/linux/genhd.h |    2 +-
 3 files changed, 27 insertions(+), 8 deletions(-)

block/ioctl.c +4 -1

index 52d6385..bb3933e 100644
@@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) {
+				mutex_unlock(&bdev->bd_mutex);
+				return -EBUSY;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +22 -6

index 722e12e..6bdf855 100644
@@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }
 
-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;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -1;
 	
 	p->start_sect = start;
 	p->nr_sects = len;

@@ -390,20 +390,33 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	if (kobject_add(&p->kobj)) 
+		return -1;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) 
+		goto out_cleanup;
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		if (sysfs_create_file(&p->kobj, &addpartattr)) {
+			sysfs_remove_link (&p->kobj, "subsystem");
+			goto out_cleanup;
+		}
 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+	
+	return 0;
+
+out_cleanup:
+	if (!disk->part_uevent_suppress)
+		kobject_uevent(&p->kobj, KOBJ_REMOVE);
+	kobject_del(&p->kobj);
+	return -1;
 }
 
 static char *make_block_name(struct gendisk *disk)

@@ -554,8 +567,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (from + size > get_capacity(disk)) {
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
+			return -EBUSY;
+		}
+		if(add_partition(disk, p, from, size, state->parts[p].flags)) {
+			return -EBUSY;
 		}
-		add_partition(disk, p, from, size, state->parts[p].flags);
 #ifdef CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags & ADDPART_FLAG_RAID)
 			md_autodetect_dev(bdev->bd_dev+p);

include/linux/genhd.h +1 -1

index a47b802..ae6af2c 100644
@@ -414,7 +414,7 @@ struct unixware_disklabel {
 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);

Signature · 1 line

gitgui.0.8.4.g8d863

Cornelia Huck

2007-10-29 14:44 UTC #3 lore raw

fs/partitions/check.c void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 393

Quoted 6 lines · Dirk Hohndel

fs/partitions/check.c +1 -1

@@ -390,20 +390,33 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	if (kobject_add(&p->kobj))

Hm, here the structure needs to be freed as well (via kobject_put()).

fs/partitions/check.c void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 397

Quoted 5 lines

fs/partitions/check.c +2 -1

+		return -1;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"))

Whitespace (if (...)).

Quoted 12 lines

fs/partitions/check.c +5 -1

+		goto out_cleanup;
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		if (sysfs_create_file(&p->kobj, &addpartattr)) {
+			sysfs_remove_link (&p->kobj, "subsystem");
+			goto out_cleanup;
+		}

fs/partitions/check.c void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 418

Quoted 10 lines

fs/partitions/check.c +7 -0

 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+	
+	return 0;
+
+out_cleanup:
+	if (!disk->part_uevent_suppress)
+		kobject_uevent(&p->kobj, KOBJ_REMOVE);
+	kobject_del(&p->kobj);

You need a kobject_put() here to drop the reference you obtained in kobject_init().

Quoted 4 lines

fs/partitions/check.c +1 -0

+	return -1;
 }
 
 static char *make_block_name(struct gendisk *disk)

Dirk Hohndel

author

2007-10-29 15:49 UTC #4 lore raw

Quoted 9 lines · Cornelia Huck

Review void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 393
Quoted 7 lines
(quoted diff) +1 -1
@@ -390,20 +390,33 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	if (kobject_add(&p->kobj)) 
Hm, here the structure needs to be freed as well (via kobject_put()).

done

Quoted 5 lines

Review void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 395
Quoted 3 lines
(quoted diff) +1 -1
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) 
Whitespace (if (...)).

oops.

Quoted 8 lines

Review void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 400
Quoted 5 lines
(quoted diff) +5 -0
+
+out_cleanup:
+	if (!disk->part_uevent_suppress)
+		kobject_uevent(&p->kobj, KOBJ_REMOVE);
+	kobject_del(&p->kobj);
You need a kobject_put() here to drop the reference you obtained in kobject_init().

done

We should be getting closer - thanks for all the help getting this right!

[PATCH] add_partition silently ignored errors

Signed-off-by Dirk Hohndel <[email protected]>

---
 block/ioctl.c         |    5 ++++-
 fs/partitions/check.c |   30 ++++++++++++++++++++++++------
 include/linux/genhd.h |    2 +-
 3 files changed, 29 insertions(+), 8 deletions(-)

block/ioctl.c +4 -1

index 52d6385..bb3933e 100644
@@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) {
+				mutex_unlock(&bdev->bd_mutex);
+				return -EBUSY;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +24 -6

index 722e12e..cd92471 100644
@@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }
 
-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;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -1;
 	
 	p->start_sect = start;
 	p->nr_sects = len;

@@ -390,20 +390,35 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	if (kobject_add(&p->kobj)) 
+		goto out_put;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if (sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) 
+		goto out_cleanup;
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		if (sysfs_create_file(&p->kobj, &addpartattr)) {
+			sysfs_remove_link (&p->kobj, "subsystem");
+			goto out_cleanup;
+		}
 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+	
+	return 0;
+
+out_cleanup:
+	if (!disk->part_uevent_suppress)
+		kobject_uevent(&p->kobj, KOBJ_REMOVE);
+	kobject_del(&p->kobj);
+out_put:
+	kobject_put(&p->kobj);
+	return -1;
 }
 
 static char *make_block_name(struct gendisk *disk)

@@ -554,8 +569,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (from + size > get_capacity(disk)) {
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
+			return -EBUSY;
+		}
+		if(add_partition(disk, p, from, size, state->parts[p].flags)) {
+			return -EBUSY;
 		}
-		add_partition(disk, p, from, size, state->parts[p].flags);
 #ifdef CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags & ADDPART_FLAG_RAID)
 			md_autodetect_dev(bdev->bd_dev+p);

include/linux/genhd.h +1 -1

index a47b802..ae6af2c 100644
@@ -414,7 +414,7 @@ struct unixware_disklabel {
 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);

Signature · 1 line

gitgui.0.8.4.g8d863

Cornelia Huck

2007-10-29 16:47 UTC #5 lore raw

Quoted 9 lines · Dirk Hohndel

[PATCH] add_partition silently ignored errors
Signed-off-by Dirk Hohndel <[email protected]>
---
 block/ioctl.c         |    5 ++++-
 fs/partitions/check.c |   30 ++++++++++++++++++++++++------
 include/linux/genhd.h |    2 +-
 3 files changed, 29 insertions(+), 8 deletions(-)

The kobject stuff looks sane to me now. Others will have to comment on the return code propagation.

Jens Axboe

2007-10-30 08:10 UTC #6 lore raw

In reply to Dirk Hohndel

Quoted 23 lines · Dirk Hohndel

block/ioctl.c +4 -1

index 52d6385..bb3933e 100644
@@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) {
+				mutex_unlock(&bdev->bd_mutex);
+				return -EBUSY;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +0 -0

index 722e12e..cd92471 100644
@@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }

fs/partitions/check.c void delete_partition(struct gendisk *disk, int part) · line 377

Quoted 10 lines

fs/partitions/check.c +2 -2

 
-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;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -1;

Why not return the 'correct' error codes, instead of always -1 and making that -EBUSY at the caller? This one should be -ENOMEM.

IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.

Signature · 1 line

Jens Axboe

Cornelia Huck

2007-10-30 09:10 UTC #7 lore raw

Quoted 30 lines · Jens Axboe

Quoted 29 lines · Dirk Hohndel

block/ioctl.c +4 -1

index 52d6385..bb3933e 100644
@@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) {
+				mutex_unlock(&bdev->bd_mutex);
+				return -EBUSY;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +1 -1

index 722e12e..cd92471 100644
@@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }
 
-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;

Quoted 7 lines

fs/partitions/check.c void delete_partition(struct gendisk *disk, int part) · line 377
Quoted 4 lines
fs/partitions/check.c +1 -1
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -1;
Why not return the 'correct' error codes, instead of always -1 and making that -EBUSY at the caller? This one should be -ENOMEM.

Oops, you're right. I agree.

Quoted 3 lines

IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.

We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question.

Dirk Hohndel

author

2007-10-30 16:56 UTC #8 lore raw

Quoted 17 lines · Cornelia Huck

Quoted 6 lines · Jens Axboe
Quoted 6 lines
fs/partitions/check.c +1 -1
-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;
Quoted 7 lines
fs/partitions/check.c void delete_partition(struct gendisk *disk, int part) · line 377
Quoted 4 lines
fs/partitions/check.c +1 -1
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -1;
Why not return the 'correct' error codes, instead of always -1 and making that -EBUSY at the caller? This one should be -ENOMEM.

Quoted 1 line

Oops, you're right. I agree.

Duh. That was dumb of me.

Quoted 7 lines

Quoted 2 lines
IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.
We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question.

What are you suggesting? I'd love to make the behaviour consistent everywhere (and am willing to go through things in order to make that happen), but what is the consistent behaviour that we'd want?

Cornelia Huck

2007-10-30 17:31 UTC #9 lore raw

Quoted 11 lines · Dirk Hohndel

Quoted 7 lines
Quoted 2 lines
IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.
We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question.
What are you suggesting? I'd love to make the behaviour consistent everywhere (and am willing to go through things in order to make that happen), but what is the consistent behaviour that we'd want?

I'd be fine with just propagating the error after cleanup (that is what for example the driver core usually does), but I don't know the surrounding code well enough for a definitive answer.

Dirk Hohndel

author

2007-10-30 22:57 UTC #10 lore raw

Quoted 5 lines · Cornelia Huck

Quoted 2 lines · Dirk Hohndel
Quoted 2 lines
Quoted 2 lines
IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.

So Al, are you ok with this one?

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question.

Quoted 7 lines

Quoted 3 lines
What are you suggesting? I'd love to make the behaviour consistent everywhere (and am willing to go through things in order to make that happen), but what is the consistent behaviour that we'd want?
I'd be fine with just propagating the error after cleanup (that is what for example the driver core usually does), but I don't know the surrounding code well enough for a definitive answer.

Ok, I think I have it consistent now. I also ran it through checkpatch.pl :-)

[FILESYSTEM] add_partition ignores errors

Signed-off-by Dirk Hohndel <[email protected]>

---
 block/ioctl.c         |    9 +++++++--
 fs/partitions/check.c |   36 +++++++++++++++++++++++++++++-------
 include/linux/genhd.h |    2 +-
 3 files changed, 37 insertions(+), 10 deletions(-)

block/ioctl.c +7 -2

index 52d6385..662ec18 100644
@@ -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, err;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;

@@ -61,7 +61,12 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			err = add_partition(disk, part, start, length,
+						ADDPART_FLAG_NONE)
+			if (err) {
+				mutex_unlock(&bdev->bd_mutex);
+				return err;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +29 -7

index 722e12e..dea79bd 100644
@@ -368,13 +368,14 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }
 
-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;
+	int err = 0;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -ENOMEM;
 	
 	p->start_sect = start;
 	p->nr_sects = len;

@@ -390,20 +391,38 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	err = kobject_add(&p->kobj);
+	if (err)
+		goto out_put;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	err = sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if (err)
+		goto out_cleanup;
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		err = sysfs_create_file(&p->kobj, &addpartattr);
+		if (err) {
+			sysfs_remove_link(&p->kobj, "subsystem");
+			goto out_cleanup;
+		}
 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+
+	return 0;
+
+out_cleanup:
+	if (!disk->part_uevent_suppress)
+		kobject_uevent(&p->kobj, KOBJ_REMOVE);
+	kobject_del(&p->kobj);
+out_put:
+	kobject_put(&p->kobj);
+	return err;
 }
 
 static char *make_block_name(struct gendisk *disk)

@@ -530,7 +549,7 @@ exit:
 int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
-	int p, res;
+	int p, res, err;
 
 	if (bdev->bd_part_count)
 		return -EBUSY;

@@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (from + size > get_capacity(disk)) {
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
+			return -EBUSY;
 		}
-		add_partition(disk, p, from, size, state->parts[p].flags);
+		err = add_partition(disk, p, from, size, state->parts[p].flags);
+		if (err)
+			return err;
 #ifdef CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags & ADDPART_FLAG_RAID)
 			md_autodetect_dev(bdev->bd_dev+p);

include/linux/genhd.h +1 -1

index a47b802..ae6af2c 100644
@@ -414,7 +414,7 @@ struct unixware_disklabel {
 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);

Signature · 1 line

gitgui.0.8.4.g8d863

Cornelia Huck

2007-10-31 09:46 UTC #11 lore raw

Quoted 29 lines · Dirk Hohndel

Quoted 5 lines · Cornelia Huck
Quoted 2 lines · Dirk Hohndel
Quoted 2 lines
Quoted 2 lines
IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.
So Al, are you ok with this one?
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question.
Quoted 7 lines
Quoted 3 lines
What are you suggesting? I'd love to make the behaviour consistent everywhere (and am willing to go through things in order to make that happen), but what is the consistent behaviour that we'd want?
I'd be fine with just propagating the error after cleanup (that is what for example the driver core usually does), but I don't know the surrounding code well enough for a definitive answer.
Ok, I think I have it consistent now. I also ran it through checkpatch.pl :-)
/D
[FILESYSTEM] add_partition ignores errors

Quoted 7 lines

Signed-off-by Dirk Hohndel <[email protected]>

---
 block/ioctl.c         |    9 +++++++--
 fs/partitions/check.c |   36 +++++++++++++++++++++++++++++-------
 include/linux/genhd.h |    2 +-
 3 files changed, 37 insertions(+), 10 deletions(-)

OK, the kobject error handling looks fine to me.

Jens Axboe

2007-11-02 13:04 UTC #12 lore raw

In reply to Dirk Hohndel

Quoted 22 lines · Dirk Hohndel

Quoted 5 lines · Cornelia Huck
Quoted 2 lines · Dirk Hohndel
Quoted 2 lines
Quoted 2 lines
IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.
So Al, are you ok with this one?
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question.
Quoted 7 lines
Quoted 3 lines
What are you suggesting? I'd love to make the behaviour consistent everywhere (and am willing to go through things in order to make that happen), but what is the consistent behaviour that we'd want?
I'd be fine with just propagating the error after cleanup (that is what for example the driver core usually does), but I don't know the surrounding code well enough for a definitive answer.

Quoted 6 lines

Ok, I think I have it consistent now. I also ran it through checkpatch.pl :-)
/D
[FILESYSTEM] add_partition ignores errors

Looks good to me. One final return value note:

fs/partitions/check.c int rescan_partitions(struct gendisk *disk, struct block_device *bdev) · line 577

Quoted 6 lines

fs/partitions/check.c +1 -0

@@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (from + size > get_capacity(disk)) {
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
+			return -EBUSY;
 		}

-EBUSY seems a bit confusing here, although I don't know what the best value to return would be (and it probably doesn't matter). -EOVERFLOW? -ENOSPC?

Signature · 1 line

Jens Axboe

Dirk Hohndel

author

2007-11-02 19:29 UTC #13 lore raw

Quoted 5 lines · Jens Axboe

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me.
So Al, are you ok with this one?

Still haven't seen feedback from Al...

Quoted 4 lines

Quoted 1 line
[FILESYSTEM] add_partition ignores errors
Looks good to me. One final return value note:

Quoted 10 lines

fs/partitions/check.c int rescan_partitions(struct gendisk *disk, struct block_device *bdev) · line 577
Quoted 6 lines
fs/partitions/check.c +1 -0
@@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (from + size > get_capacity(disk)) {
 			printk(" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
+			return -EBUSY;
 		}
-EBUSY seems a bit confusing here, although I don't know what the best value to return would be (and it probably doesn't matter). -EOVERFLOW? -ENOSPC?

I was wondering about that myself - EBUSY seemed to be used in a couple of other cases where there wasn't a clear match, but I think EOVERFLOW actually might make more sense. Opinions?

Bob Copeland

2007-11-02 19:50 UTC #14 lore raw

Quoted 5 lines · Dirk Hohndel

Quoted 5 lines

fs/partitions/check.c +1 -0

@@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
            if (from + size > get_capacity(disk)) {
                    printk(" %s: p%d exceeds device capacity\n",
                            disk->disk_name, p);
+                   return -EBUSY;

[snip]

Quoted 3 lines

I was wondering about that myself - EBUSY seemed to be used in a couple of other cases where there wasn't a clear match, but I think EOVERFLOW actually might make more sense. Opinions?

ISTR that some people wanted to keep going in this case rather than return an error, e.g. for forensic purposes...

.. digging... here's a thread from last year:

http://lkml.org/lkml/2006/5/11/64

-Bob

Dirk Hohndel

author

2007-11-02 20:30 UTC #15 lore raw

Quoted 11 lines · Bob Copeland

Quoted 5 lines · Dirk Hohndel
Quoted 5 lines
Quoted 5 lines
fs/partitions/check.c +1 -0
@@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
            if (from + size > get_capacity(disk)) {
                    printk(" %s: p%d exceeds device capacity\n",
                            disk->disk_name, p);
+                   return -EBUSY;
[snip]
Quoted 3 lines
I was wondering about that myself - EBUSY seemed to be used in a couple of other cases where there wasn't a clear match, but I think EOVERFLOW actually might make more sense. Opinions?

Quoted 6 lines

ISTR that some people wanted to keep going in this case rather than return an error, e.g. for forensic purposes...
.. digging... here's a thread from last year:
http://lkml.org/lkml/2006/5/11/64

Thanks for finding that! I took a different approach than Andries but can appreciate the argument. I'll remove that line from my patch.

Dirk Hohndel

author

2007-11-06 20:03 UTC #16 lore raw

PATCH 1/1 FS: add_partition silently ignored errors

(since there was no more discussion, here's the last version that includes the requested change to not fail if the partition is larger than the disk)

add_partition silently ignored errors
this patch passes meaningful errorcodes back
and processes them in the calling functions

Signed-off-by Dirk Hohndel <[email protected]>

---
 block/ioctl.c         |    9 +++++++--
 fs/partitions/check.c |   35 ++++++++++++++++++++++++++++-------
 include/linux/genhd.h |    2 +-
 3 files changed, 36 insertions(+), 10 deletions(-)

block/ioctl.c +7 -2

index 52d6385..662ec18 100644
@@ -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, err;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;

@@ -61,7 +61,12 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 				}
 			}
 			/* all seems OK */
-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			err = add_partition(disk, part, start, length,
+						ADDPART_FLAG_NONE)
+			if (err) {
+				mutex_unlock(&bdev->bd_mutex);
+				return err;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;
 		case BLKPG_DEL_PARTITION:

fs/partitions/check.c +28 -7

index 722e12e..f802b32 100644
@@ -368,13 +368,14 @@ void delete_partition(struct gendisk *disk, int part)
 	kobject_put(&p->kobj);
 }
 
-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;
+	int err = 0;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (!p)
-		return;
+		return -ENOMEM;
 	
 	p->start_sect = start;
 	p->nr_sects = len;

@@ -390,20 +391,38 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len,
 	p->kobj.parent = &disk->kobj;
 	p->kobj.ktype = &ktype_part;
 	kobject_init(&p->kobj);
-	kobject_add(&p->kobj);
+	err = kobject_add(&p->kobj);
+	if (err)
+		goto out_put;
 	if (!disk->part_uevent_suppress)
 		kobject_uevent(&p->kobj, KOBJ_ADD);
-	sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	err = sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem");
+	if (err)
+		goto out_cleanup;
 	if (flags & ADDPART_FLAG_WHOLEDISK) {
 		static struct attribute addpartattr = {
 			.name = "whole_disk",
 			.mode = S_IRUSR | S_IRGRP | S_IROTH,
 		};
 
-		sysfs_create_file(&p->kobj, &addpartattr);
+		err = sysfs_create_file(&p->kobj, &addpartattr);
+		if (err) {
+			sysfs_remove_link(&p->kobj, "subsystem");
+			goto out_cleanup;
+		}
 	}
 	partition_sysfs_add_subdir(p);
 	disk->part[part-1] = p;
+
+	return 0;
+
+out_cleanup:
+	if (!disk->part_uevent_suppress)
+		kobject_uevent(&p->kobj, KOBJ_REMOVE);
+	kobject_del(&p->kobj);
+out_put:
+	kobject_put(&p->kobj);
+	return err;
 }
 
 static char *make_block_name(struct gendisk *disk)

@@ -530,7 +549,7 @@ exit:
 int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
-	int p, res;
+	int p, res, err;
 
 	if (bdev->bd_part_count)
 		return -EBUSY;

@@ -555,7 +574,9 @@ 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);
+		err = add_partition(disk, p, from, size, state->parts[p].flags);
+		if (err)
+			return err;
 #ifdef CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags & ADDPART_FLAG_RAID)
 			md_autodetect_dev(bdev->bd_dev+p);

include/linux/genhd.h +1 -1

index a47b802..ae6af2c 100644
@@ -414,7 +414,7 @@ struct unixware_disklabel {
 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);

Signature · 1 line

gitgui.0.8.4.g8d863

Linus Torvalds

2007-11-06 21:40 UTC #17 lore raw

Re: [PATCH] FS: add_partition silently ignored errors

Quoted 3 lines · Dirk Hohndel

(since there was no more discussion, here's the last version that includes the requested change to not fail if the partition is larger than the disk)

I'd suggest keeping it in -mm for a while.

I worry about these kinds of "trivial" changes. Quite often, some errors may be normal, and breaking out early can sometimes hurt more than it helps, in that it just makes things not even limp along. That said, with the disk/partition size check removed, this looks more palatable.

block/ioctl.c static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user · line 71

Quoted 9 lines

block/ioctl.c +6 -1

-			add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
+			err = add_partition(disk, part, start, length,
+						ADDPART_FLAG_NONE)
+			if (err) {
+				mutex_unlock(&bdev->bd_mutex);
+				return err;
+			}
 			mutex_unlock(&bdev->bd_mutex);
 			return 0;

However, this is just ugly. The thing is not only apparently totally untested, since it is missing a semicolon, it should also just read

	err = add_partition(disk, part, start, length, ADDPART_FLAG_NONE);
	mutex_unlock(&bdev->bd_mutex);
	return err;

without any unnecessary conditionals (or ugly line-breaks, for that matter).

fs/partitions/check.c void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, · line 411

Quoted 5 lines

fs/partitions/check.c +4 -1

-		sysfs_create_file(&p->kobj, &addpartattr);
+		err = sysfs_create_file(&p->kobj, &addpartattr);
+		if (err) {
+			sysfs_remove_link(&p->kobj, "subsystem");
+			goto out_cleanup;

Wouldn't this be better done as

	if (err)
		goto out_unlink_cleanup;

to match the rest of the error handling?

Linus