LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
       [not found] ` <glHQm-1vQ-25@gated-at.bofh.it>
@ 2011-02-13 20:41   ` Bodo Eggert
  0 siblings, 0 replies; 8+ messages in thread
From: Bodo Eggert @ 2011-02-13 20:41 UTC (permalink / raw)
  To: Tao Ma, linux-kernel, Jens Axboe, Tejun Heo

Tao Ma <tm@tao.ma> wrote:

> --- a/drivers/block/loop.c

> -     if (bdev)
> +     if (bdev) {
> +             set_device_ro(bdev, 0);
>  invalidate_bdev(bdev);
> +     }
>  set_capacity(lo->lo_disk, 0);
>  loop_sysfs_exit(lo);
>  if (bdev) {

This looks like set_device_ro() should imply invalidate_bdev().
-- 
E.G.G.E.R.T.: Electronic Guardian Generated for 
              Efficient Repair and Troubleshooting
        -- http://www.brunching.com/toys/toy-cyborger.html (down)
Friß, Spammer: PanIhbEe3@y.7eggert.dyndns.org iNEC@zy1oe.7eggert.dyndns.org


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

* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-14 11:47           ` Milan Broz
@ 2011-02-14 14:07             ` Tejun Heo
  0 siblings, 0 replies; 8+ messages in thread
From: Tejun Heo @ 2011-02-14 14:07 UTC (permalink / raw)
  To: Milan Broz; +Cc: Tao Ma, linux-kernel, Jens Axboe, device-mapper development

Hello, Milan.

On Mon, Feb 14, 2011 at 12:47:48PM +0100, Milan Broz wrote:
> With patch below (loop cannot be built as module) it fixes the loop problem.

Okay.

> But it doesn't fix the read-only snapshot issue and I guess there will be
> the same problem with read-only MD code too.
> (so the 2) issue here https://lkml.org/lkml/2011/2/12/209).
> 
> If the call is changed intentionally, we have to fix unconditional blkdev open
> calls with read-write flag in this code.
> Before doing that I would like to know if it was intentional change or not...

Yeap, the change was intentional.  It was part of effort to make bdev
usages more consistent as before there was no mechanism enforcing ro.
It's still problematic as bdev users can clear ro without consulting
the actual device driver.  Device driver's ->open() is called w/ ro
flag but the resulting bdev can be used rw.  I wanted to remove that
too but filesystems depend on it so maybe later.

Thanks.

-- 
tejun

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

* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-14 10:30         ` Tejun Heo
@ 2011-02-14 11:47           ` Milan Broz
  2011-02-14 14:07             ` Tejun Heo
  0 siblings, 1 reply; 8+ messages in thread
From: Milan Broz @ 2011-02-14 11:47 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Tao Ma, linux-kernel, Jens Axboe, device-mapper development


On 02/14/2011 11:30 AM, Tejun Heo wrote:
> Umm... This was reported some time ago and patches were already
> posted.  Milan, can you test whether the following two patches fix the
> problems you're seeing?  Jens, what's the status of these patches?
>
>   http://thread.gmane.org/gmane.linux.kernel/1090211/focus=1090666
>
With patch below (loop cannot be built as module) it fixes the loop problem.

But it doesn't fix the read-only snapshot issue and I guess there will be
the same problem with read-only MD code too.
(so the 2) issue here https://lkml.org/lkml/2011/2/12/209).

If the call is changed intentionally, we have to fix unconditional blkdev open
calls with read-write flag in this code.
Before doing that I would like to know if it was intentional change or not...

You can simple try this reproducer (works on older kernel, second readonly
snapshot create fails now with permission denied)
+ dmsetup create x --readonly --table '0 131072 snapshot /dev/loop0 /dev/loop1 p 8'
device-mapper: reload ioctl failed: Permission denied

#!/bin/bash -x
modprobe loop

dd if=/dev/zero of=/x.img bs=1M count=64
dd if=/dev/zero of=/xs.img bs=1M count=64
losetup /dev/loop0 /x.img
losetup /dev/loop1 /xs.img
sync
dmsetup create x --table "0 131072 snapshot /dev/loop0 /dev/loop1 p 8"
udevadm settle
dmsetup remove x

losetup -d /dev/loop0
losetup -d /dev/loop1
losetup -r /dev/loop0 /x.img
losetup -r /dev/loop1 /xs.img
dmsetup create x --readonly --table "0 131072 snapshot /dev/loop0 /dev/loop1 p 8"
dmsetup table

dmsetup remove x
losetup -d /dev/loop0
losetup -d /dev/loop1

Milan

--
Export bdgrap to allow loop module build 

Signed-off-by: Milan Broz <mbroz@redhat.com> 
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 333a7bb..c9cf9f7 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -574,6 +574,7 @@ struct block_device *bdgrab(struct block_device *bdev)
 	ihold(bdev->bd_inode);
 	return bdev;
 }
+EXPORT_SYMBOL(bdgrab);
 
 long nr_blockdev_pages(void)
 {



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

* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-13 16:44       ` Milan Broz
@ 2011-02-14 10:30         ` Tejun Heo
  2011-02-14 11:47           ` Milan Broz
  0 siblings, 1 reply; 8+ messages in thread
From: Tejun Heo @ 2011-02-14 10:30 UTC (permalink / raw)
  To: Milan Broz; +Cc: Tao Ma, linux-kernel, Jens Axboe

On Sun, Feb 13, 2011 at 05:44:59PM +0100, Milan Broz wrote:
> On 02/13/2011 04:05 PM, Tao Ma wrote:
> > On 02/13/2011 10:11 PM, Milan Broz wrote:
> >> On 02/13/2011 11:58 AM, Tao Ma wrote:
> >>    
> >>> From: Tao Ma<boyu.mt@taobao.com>
> >>>
> >>> In 75f1dc0, we check bdev_read_only() from blkdev_get().
> >>> But the loop_clr_fd doesn't clear the read only flag.
> >>> What cause a error if we changing a loop device from
> >>> read only to writable.
> >>>      
> >> No, sorry, this is not proper/complete fix. It fixes it for loop
> >> (and even not completely - you are missing some error
> >> paths and ignoring autoclear mode where you have bdev NULL.)
> >> (And yes, I tried the same as workaround.)
> >>    
> > aha, sorry, I don't know you are more familiar with loop than me. ;)
> > I just did a quick test and sent the patch. So could you please tell me
> > a little more about how we use autoclear mode?

Umm... This was reported some time ago and patches were already
posted.  Milan, can you test whether the following two patches fix the
problems you're seeing?  Jens, what's the status of these patches?

  http://thread.gmane.org/gmane.linux.kernel/1090211/focus=1090666

Thanks.

-- 
tejun

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

* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-13 15:05     ` Tao Ma
@ 2011-02-13 16:44       ` Milan Broz
  2011-02-14 10:30         ` Tejun Heo
  0 siblings, 1 reply; 8+ messages in thread
From: Milan Broz @ 2011-02-13 16:44 UTC (permalink / raw)
  To: Tao Ma; +Cc: linux-kernel, Jens Axboe, Tejun Heo

On 02/13/2011 04:05 PM, Tao Ma wrote:
> On 02/13/2011 10:11 PM, Milan Broz wrote:
>> On 02/13/2011 11:58 AM, Tao Ma wrote:
>>    
>>> From: Tao Ma<boyu.mt@taobao.com>
>>>
>>> In 75f1dc0, we check bdev_read_only() from blkdev_get().
>>> But the loop_clr_fd doesn't clear the read only flag.
>>> What cause a error if we changing a loop device from
>>> read only to writable.
>>>      
>> No, sorry, this is not proper/complete fix. It fixes it for loop
>> (and even not completely - you are missing some error
>> paths and ignoring autoclear mode where you have bdev NULL.)
>> (And yes, I tried the same as workaround.)
>>    
> aha, sorry, I don't know you are more familiar with loop than me. ;)
> I just did a quick test and sent the patch. So could you please tell me
> a little more about how we use autoclear mode?

When the autoclear flag is set, the loop device is deallocated with
the last close. So you can mount device over loop and after umount
the loop is automatically cleared (no need for losetup -d).
(I think this flag was not exported to losetup yet, so you need to use
ioctl flag yourself.)

> I will try to update my patch with a V2 when I get familiar with the whole stuff.

I would like Tejun tell us what the intention was here.
There are some paths which are not so clear (this one in loop device is easy),
so that code need to be audited.

> Actually I don't think it is Tejun's patch that causes the bug.

It is quite possible. But it worked before and this patch did not
fix these problems, so it is regression.

> Say loop, it sets ro when it get read  only flags, but doesn't clear it when it is detached.

You can very easily create another bug here if you set device read-write
too early (while udev is still processing change/remove events).

Milan

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

* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-13 14:11   ` Milan Broz
@ 2011-02-13 15:05     ` Tao Ma
  2011-02-13 16:44       ` Milan Broz
  0 siblings, 1 reply; 8+ messages in thread
From: Tao Ma @ 2011-02-13 15:05 UTC (permalink / raw)
  To: Milan Broz; +Cc: linux-kernel, Jens Axboe, Tejun Heo

On 02/13/2011 10:11 PM, Milan Broz wrote:
> On 02/13/2011 11:58 AM, Tao Ma wrote:
>    
>> From: Tao Ma<boyu.mt@taobao.com>
>>
>> In 75f1dc0, we check bdev_read_only() from blkdev_get().
>> But the loop_clr_fd doesn't clear the read only flag.
>> What cause a error if we changing a loop device from
>> read only to writable.
>>      
> No, sorry, this is not proper/complete fix. It fixes it for loop
> (and even not completely - you are missing some error
> paths and ignoring autoclear mode where you have bdev NULL.)
> (And yes, I tried the same as workaround.)
>    
aha, sorry, I don't know you are more familiar with loop than me. ;)
I just did a quick test and sent the patch. So could you please tell me
a little more about how we use autoclear mode? I just googled but can't
find some helpful information. I will try to update my patch with a V2 when
I get familiar with the whole stuff.
> And it will not help for DM case (and possibly others).
>    
Actually I don't think it is Tejun's patch that causes the bug. What his 
patch do
is to expose some bugs that already exist.  Say loop, it sets ro when it 
get read
only flags, but doesn't clear it when it is detached. It should be 
loop's problem,
not blkdev's. As for the DM case, I guess it should also be related to 
DM part.

Regards,
Tao

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

* Re: [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-13 10:58 ` [PATCH] loop: clear read-only flag in loop_clr_fd Tao Ma
@ 2011-02-13 14:11   ` Milan Broz
  2011-02-13 15:05     ` Tao Ma
  0 siblings, 1 reply; 8+ messages in thread
From: Milan Broz @ 2011-02-13 14:11 UTC (permalink / raw)
  To: Tao Ma; +Cc: linux-kernel, Jens Axboe, Tejun Heo

On 02/13/2011 11:58 AM, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> In 75f1dc0, we check bdev_read_only() from blkdev_get().
> But the loop_clr_fd doesn't clear the read only flag.
> What cause a error if we changing a loop device from
> read only to writable.

No, sorry, this is not proper/complete fix. It fixes it for loop
(and even not completely - you are missing some error
paths and ignoring autoclear mode where you have bdev NULL.)
(And yes, I tried the same as workaround.)

And it will not help for DM case (and possibly others).

Milan

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

* [PATCH] loop: clear read-only flag in loop_clr_fd.
  2011-02-13  2:02 block device read-only handling regression in 2.6.38-rc4 (bisected) Milan Broz
@ 2011-02-13 10:58 ` Tao Ma
  2011-02-13 14:11   ` Milan Broz
  0 siblings, 1 reply; 8+ messages in thread
From: Tao Ma @ 2011-02-13 10:58 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jens Axboe, Tejun Heo

From: Tao Ma <boyu.mt@taobao.com>

In 75f1dc0, we check bdev_read_only() from blkdev_get().
But the loop_clr_fd doesn't clear the read only flag.
What cause a error if we changing a loop device from
read only to writable.

A simple test to reproduce the error reported by Milan[1]:
touch /x1.img
losetup -r /dev/loop0 /x1.img
losetup -d /dev/loop0
losetup /dev/loop0 /x1.img
/dev/loop0: Permission denied

1: http://marc.info/?l=linux-kernel&m=129756258222642&w=2

Reported-by: Milan Broz <mbroz@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 drivers/block/loop.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 44e18c0..0d24579 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1036,8 +1036,10 @@ static int loop_clr_fd(struct loop_device *lo, struct block_device *bdev)
 	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)
+	if (bdev) {
+		set_device_ro(bdev, 0);
 		invalidate_bdev(bdev);
+	}
 	set_capacity(lo->lo_disk, 0);
 	loop_sysfs_exit(lo);
 	if (bdev) {
-- 
1.6.3.GIT


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

end of thread, other threads:[~2011-02-14 14:08 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <glzzr-4Jt-1@gated-at.bofh.it>
     [not found] ` <glHQm-1vQ-25@gated-at.bofh.it>
2011-02-13 20:41   ` [PATCH] loop: clear read-only flag in loop_clr_fd Bodo Eggert
2011-02-13  2:02 block device read-only handling regression in 2.6.38-rc4 (bisected) Milan Broz
2011-02-13 10:58 ` [PATCH] loop: clear read-only flag in loop_clr_fd Tao Ma
2011-02-13 14:11   ` Milan Broz
2011-02-13 15:05     ` Tao Ma
2011-02-13 16:44       ` Milan Broz
2011-02-14 10:30         ` Tejun Heo
2011-02-14 11:47           ` Milan Broz
2011-02-14 14:07             ` Tejun Heo

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