LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Q. block: check bdev_read_only() from blkdev_get()
@ 2011-01-21 13:19 J. R. Okajima
  2011-01-21 13:28 ` Tejun Heo
  2011-01-22 14:22 ` [PATCH 1/2] loop: guarantee lo->lo_device availability while fd is bound Tejun Heo
  0 siblings, 2 replies; 7+ messages in thread
From: J. R. Okajima @ 2011-01-21 13:19 UTC (permalink / raw)
  To: Tejun Heo; +Cc: David Brownell, linux-usb, linux-kernel


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Q. block: check bdev_read_only() from blkdev_get()
  2011-01-21 13:19 Q. block: check bdev_read_only() from blkdev_get() J. R. Okajima
@ 2011-01-21 13:28 ` Tejun Heo
  2011-01-22  8:32   ` J. R. Okajima
  2011-01-22 14:22 ` [PATCH 1/2] loop: guarantee lo->lo_device availability while fd is bound Tejun Heo
  1 sibling, 1 reply; 7+ messages in thread
From: Tejun Heo @ 2011-01-21 13:28 UTC (permalink / raw)
  To: J. R. Okajima; +Cc: David Brownell, linux-usb, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Q. block: check bdev_read_only() from blkdev_get()
  2011-01-21 13:28 ` Tejun Heo
@ 2011-01-22  8:32   ` J. R. Okajima
  2011-01-22 11:48     ` Tejun Heo
  0 siblings, 1 reply; 7+ messages in thread
From: J. R. Okajima @ 2011-01-22  8:32 UTC (permalink / raw)
  To: Tejun Heo; +Cc: David Brownell, linux-usb, linux-kernel


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Q. block: check bdev_read_only() from blkdev_get()
  2011-01-22  8:32   ` J. R. Okajima
@ 2011-01-22 11:48     ` Tejun Heo
  0 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2011-01-22 11:48 UTC (permalink / raw)
  To: J. R. Okajima; +Cc: David Brownell, linux-usb, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] loop: guarantee lo->lo_device availability while fd is bound
  2011-01-21 13:19 Q. block: check bdev_read_only() from blkdev_get() J. R. Okajima
  2011-01-21 13:28 ` Tejun Heo
@ 2011-01-22 14:22 ` Tejun Heo
  2011-01-22 14:23   ` [PATCH 2/2] block: clear ro on fd detach " Tejun Heo
  1 sibling, 1 reply; 7+ messages in thread
From: Tejun Heo @ 2011-01-22 14:22 UTC (permalink / raw)
  To: Jens Axboe, J. R. Okajima
  Cc: David Brownell, linux-usb, linux-kernel, Al Viro

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 {

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] block: clear ro on fd detach fd is bound
  2011-01-22 14:22 ` [PATCH 1/2] loop: guarantee lo->lo_device availability while fd is bound Tejun Heo
@ 2011-01-22 14:23   ` Tejun Heo
  2011-01-24  1:28     ` J. R. Okajima
  0 siblings, 1 reply; 7+ messages in thread
From: Tejun Heo @ 2011-01-22 14:23 UTC (permalink / raw)
  To: Jens Axboe, J. R. Okajima
  Cc: David Brownell, linux-usb, linux-kernel, Al Viro

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);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] block: clear ro on fd detach fd is bound
  2011-01-22 14:23   ` [PATCH 2/2] block: clear ro on fd detach " Tejun Heo
@ 2011-01-24  1:28     ` J. R. Okajima
  0 siblings, 0 replies; 7+ messages in thread
From: J. R. Okajima @ 2011-01-24  1:28 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Jens Axboe, David Brownell, linux-usb, linux-kernel, Al Viro


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-01-24  1:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-21 13:19 Q. block: check bdev_read_only() from blkdev_get() J. R. Okajima
2011-01-21 13:28 ` Tejun Heo
2011-01-22  8:32   ` J. R. Okajima
2011-01-22 11:48     ` Tejun Heo
2011-01-22 14:22 ` [PATCH 1/2] loop: guarantee lo->lo_device availability while fd is bound Tejun Heo
2011-01-22 14:23   ` [PATCH 2/2] block: clear ro on fd detach " Tejun Heo
2011-01-24  1:28     ` J. R. Okajima

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).