[PATCH] blockdev.c: check driver layer errors

Check driver layer errors.

Fix from: "Jun'ichi Nomura" <j-nomura@ce.jp.nec.com>

In blockdevc-check-errors.patch, add_bd_holder() is modified to return error
values when some of its operation failed.  Among them, it returns -EEXIST when
a given bd_holder object already exists in the list.

However, in this case, the function completed its work successfully and need
no action by its caller other than freeing unused bd_holder object.  So I
think it's better to return success after freeing by itself.

Otherwise, bd_claim-ing with same claim pointer will fail.
Typically, lvresize will fails with following message:
  device-mapper: reload ioctl failed: Invalid argument
and you'll see messages like below in kernel log:
  device-mapper: table: 254:13: linear: dm-linear: Device lookup failed
  device-mapper: ioctl: error adding target to table

Similarly, it should not add bd_holder to the list if either one of symlinking
fails.  I don't have a test case for this to happen but it should cause
dereference of freed pointer.

If a matching bd_holder is found in bd_holder_list, add_bd_holder() completes
its job by just incrementing the reference count.  In this case, it should be
considered as success but it used to return 'fail' to let the caller free
temporary bd_holder.  Fixed it to return success and free given object by
itself.

Also, if either one of symlinking fails, the bd_holder should not be added to
the list so that it can be discarded later.  Otherwise, the caller will free
bd_holder which is in the list.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: "Randy.Dunlap" <rdunlap@xenotime.net>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

1 files changed, 22 insertions, 12 deletions

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 045f98854f1..8cc144ffc38 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -543,11 +543,11 @@ static struct kobject *bdev_get_holder(struct block_device *bdev)
 		return kobject_get(bdev->bd_disk->holder_dir);
 }
 
-static void add_symlink(struct kobject *from, struct kobject *to)
+static int add_symlink(struct kobject *from, struct kobject *to)
 {
 	if (!from || !to)
-		return;
-	sysfs_create_link(from, to, kobject_name(to));
+		return 0;
+	return sysfs_create_link(from, to, kobject_name(to));
 }
 
 static void del_symlink(struct kobject *from, struct kobject *to)
@@ -648,30 +648,38 @@ static void free_bd_holder(struct bd_holder *bo)
  * If there is no matching entry with @bo in @bdev->bd_holder_list,
  * add @bo to the list, create symlinks.
  *
- * Returns 1 if @bo was added to the list.
- * Returns 0 if @bo wasn't used by any reason and should be freed.
+ * Returns 0 if symlinks are created or already there.
+ * Returns -ve if something fails and @bo can be freed.
  */
 static int add_bd_holder(struct block_device *bdev, struct bd_holder *bo)
 {
 	struct bd_holder *tmp;
+	int ret;
 
 	if (!bo)
-		return 0;
+		return -EINVAL;
 
 	list_for_each_entry(tmp, &bdev->bd_holder_list, list) {
 		if (tmp->sdir == bo->sdir) {
 			tmp->count++;
+			/* We've already done what we need to do here. */
+			free_bd_holder(bo);
 			return 0;
 		}
 	}
 
 	if (!bd_holder_grab_dirs(bdev, bo))
-		return 0;
+		return -EBUSY;
 
-	add_symlink(bo->sdir, bo->sdev);
-	add_symlink(bo->hdir, bo->hdev);
-	list_add_tail(&bo->list, &bdev->bd_holder_list);
-	return 1;
+	ret = add_symlink(bo->sdir, bo->sdev);
+	if (ret == 0) {
+		ret = add_symlink(bo->hdir, bo->hdev);
+		if (ret)
+			del_symlink(bo->sdir, bo->sdev);
+	}
+	if (ret == 0)
+		list_add_tail(&bo->list, &bdev->bd_holder_list);
+	return ret;
 }
 
 /**
@@ -741,7 +749,9 @@ static int bd_claim_by_kobject(struct block_device *bdev, void *holder,
 
 	mutex_lock_nested(&bdev->bd_mutex, BD_MUTEX_PARTITION);
 	res = bd_claim(bdev, holder);
-	if (res || !add_bd_holder(bdev, bo))
+	if (res == 0)
+		res = add_bd_holder(bdev, bo);
+	if (res)
 		free_bd_holder(bo);
 	mutex_unlock(&bdev->bd_mutex);