Q. block: check bdev_read_only() from blkdev_get()

PATCH In review J. R. Okajima 2 participants 7 messages Jan 21, 2011 – Jan 24, 2011 Block layer

Share & tools

J. R. Okajima

author

2011-01-21 13:23 UTC #0 lore raw

By the commit 75f1dc0

	2010-11-13 block: check bdev_read_only() from blkdev_get()

bdev_read_only() call is added into blkdev_get().

This check makes the loopback block device unable to be mounted as writable once it is set to readonly, even if the corresponding is detached and the device becomes free.
We may need to re-initialize the readonly/writable status flag somewhere?

# simple tests with loopback

# mount as writable, succeed
+ sudo mount -o loop ./ext2.img /mnt
+ sudo umount /mnt

# mount as readonly, succeed
+ sudo mount -o ro,loop ./ext2.img /mnt
+ sudo umount /mnt

# mount as writable again, fail
+ sudo mount -o loop ./ext2.img /mnt
/dev/loop0: Permission denied

Now /dev/loop0 is fixed as readonly.

J. R. Okajima

Tejun Heo

2011-01-21 13:28 UTC #1 lore raw

Quoted 9 lines · J. R. Okajima

By the commit 75f1dc0
	2010-11-13 block: check bdev_read_only() from blkdev_get()
bdev_read_only() call is added into blkdev_get().
This check makes the loopback block device unable to be mounted as writable once it is set to readonly, even if the corresponding is detached and the device becomes free.
We may need to re-initialize the readonly/writable status flag somewhere?

Yeah, it should definitely reset r/w flag on each mount. Interested in fixing it?

Thanks.

Signature · 1 line

tejun

J. R. Okajima

author

2011-01-22 08:40 UTC #2 lore raw

Tejun Heo:

Quoted 2 lines

Yeah, it should definitely reset r/w flag on each mount. Interested in fixing it?

It must be faster and sure that you (or other bdev people) would fix much more than I do.
I don't think you want me to dive into unfamiliar code.

J. R. Okajima

Tejun Heo

2011-01-22 11:48 UTC #3 lore raw

Hello,

Quoted 3 lines · J. R. Okajima

It must be faster and sure that you (or other bdev people) would fix much more than I do.
I don't think you want me to dive into unfamiliar code.

As you already have figured out most of technical details, I don't think it would have been difficult, and, hey, jumping into unfamiliar code is half of the fun when working on the kernel. Anyways, if you aren't interested, I'll give it a shot.

Thanks.

Signature · 1 line

tejun

Tejun Heo

2011-01-22 14:22 UTC #4 lore raw

PATCH 1/2 loop: guarantee lo->lo_device availability while fd is bound

In reply to J. R. Okajima

loop_set_fd() recorded the attached bdev in lo->lo_device; however, the fd may remain attached while the bdev is detached. This causes several problems.

* lo->lo_device may end up pointing to already reclaimed bdev, so loop_clr_fd() can't use it to clear bdev attributes when called from lo_ioctl().

* As lo_open() doesn't fill in @bdev on open, if the bdev is reclaimed and then recreated, the attributes set on the original bdev are lost.

Fix it by making loop_set_fd() hold onto the bdev using bdgrab(). This allows loop_clr_fd() to rely on lo->lo_device to access the attached bdev. Drop @bdev from loop_clr_fd() and always use lo->lo_device. This guarantees that bdev attributes are cleared no matter how the fd is detached.

Signed-off-by Tejun Heo <[email protected]>

Cc "J. R. Okajima" <[email protected]>

Cc Al Viro <[email protected]>

---
Jens, these two patches fix a regression in loop introduced by block change. Thanks.

 drivers/block/loop.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

drivers/block/loop.c +11 ~~-12~~

Index: work/drivers/block/loop.c
===================================================================
@@ -901,7 +901,7 @@ static int loop_set_fd(struct loop_devic
 	set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
 
 	lo->lo_blocksize = lo_blocksize;
-	lo->lo_device = bdev;
+	lo->lo_device = bdgrab(bdev);
 	lo->lo_flags = lo_flags;
 	lo->lo_backing_file = file;
 	lo->transfer = transfer_none;

@@ -1000,8 +1000,9 @@ loop_init_xfer(struct loop_device *lo, s
 	return err;
 }
 
-static int loop_clr_fd(struct loop_device *lo, struct block_device *bdev)
+static int loop_clr_fd(struct loop_device *lo)
 {
+	struct block_device *bdev = lo->lo_device;
 	struct file *filp = lo->lo_backing_file;
 	gfp_t gfp = lo->old_gfp_mask;

@@ -1036,20 +1037,17 @@ static int loop_clr_fd(struct loop_devic
 	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
-	if (bdev)
-		invalidate_bdev(bdev);
+	invalidate_bdev(bdev);
 	set_capacity(lo->lo_disk, 0);
 	loop_sysfs_exit(lo);
-	if (bdev) {
-		bd_set_size(bdev, 0);
-		/* let user-space know about this change */
-		kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
-	}
+	bd_set_size(bdev, 0);
+	/* let user-space know about this change */
+	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
-	if (max_part > 0 && bdev)
+	if (max_part > 0)
 		ioctl_by_bdev(bdev, BLKRRPART, 0);
 	mutex_unlock(&lo->lo_ctl_mutex);
 	/*

@@ -1059,6 +1057,7 @@ static int loop_clr_fd(struct loop_devic
 	 * bd_mutex which is usually taken before lo_ctl_mutex.
 	 */
 	fput(filp);
+	bdput(bdev);
 	return 0;
 }

@@ -1312,7 +1311,7 @@ static int lo_ioctl(struct block_device
 		break;
 	case LOOP_CLR_FD:
 		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */
-		err = loop_clr_fd(lo, bdev);
+		err = loop_clr_fd(lo);
 		if (!err)
 			goto out_unlocked;
 		break;

@@ -1526,7 +1525,7 @@ static int lo_release(struct gendisk *di
 		 * In autoclear mode, stop the loop thread
 		 * and remove configuration after last close.
 		 */
-		err = loop_clr_fd(lo, NULL);
+		err = loop_clr_fd(lo);
 		if (!err)
 			goto out_unlocked;
 	} else {

Tejun Heo

2011-01-22 14:23 UTC #5 lore raw

PATCH 2/2 block: clear ro on fd detach fd is bound

Commit 75f1dc0d (block: check bdev_read_only() from blkdev_get()) enforced bdev_read_only() check during blkdev_get(). This had an unfortunate side effect on loop because loop didn't clear ro flag on fd detach, so once a loop device was used ro, it can't be used for rw at all.

Fix it by clearing ro flag on fd detach.

Signed-off-by Tejun Heo <[email protected]>

Reported-by "J. R. Okajima" <[email protected]>

---
 drivers/block/loop.c |    1 +
 1 file changed, 1 insertion(+)

drivers/block/loop.c +1 -0

Index: work/drivers/block/loop.c
===================================================================
@@ -1041,6 +1041,7 @@ static int loop_clr_fd(struct loop_devic
 	set_capacity(lo->lo_disk, 0);
 	loop_sysfs_exit(lo);
 	bd_set_size(bdev, 0);
+	set_device_ro(bdev, 0);
 	/* let user-space know about this change */
 	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);

J. R. Okajima

author

2011-01-24 01:32 UTC #6 lore raw

Re: [PATCH 2/2] block: clear ro on fd detach fd is bound

Tejun Heo:

Quoted 7 lines

Commit 75f1dc0d (block: check bdev_read_only() from blkdev_get()) enforced bdev_read_only() check during blkdev_get(). This had an unfortunate side effect on loop because loop didn't clear ro flag on fd detach, so once a loop device was used ro, it can't be used for rw at all.
Fix it by clearing ro flag on fd detach.

Tested and succeeded, thanx.

J. R. Okajima