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

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

[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?

# 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

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

[Tejun Heo]

On Fri, Jan 21, 2011 at 10:19:39PM +0900, J. R. Okajima wrote:
> 
> 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.

-- 
tejun

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

[J. R. Okajima]

Tejun Heo:
> 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

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

[Tejun Heo]

Hello,

On Sat, Jan 22, 2011 at 05:32:24PM +0900, J. R. Okajima wrote:
> 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.

-- 
tejun

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

[Tejun Heo]

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 <tj@kernel.org>
Cc: "J. R. Okajima" <hooanon05@yahoo.co.jp>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
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(-)

Index: work/drivers/block/loop.c
===================================================================
--- work.orig/drivers/block/loop.c
+++ 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 {

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

[Tejun Heo]

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 <tj@kernel.org>
Reported-by: "J. R. Okajima" <hooanon05@yahoo.co.jp>
---
 drivers/block/loop.c |    1 +
 1 file changed, 1 insertion(+)

Index: work/drivers/block/loop.c
===================================================================
--- work.orig/drivers/block/loop.c
+++ 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);

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

[J. R. Okajima]

Tejun Heo:
> 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