[Patch v2] block: revert block_dev read-only check

[Patch v2] block: revert block_dev read-only check

[Chuck Ebbert]

This reverts commit 75f1dc0d076d1c1168f2115f1941ea627d38bd5a
("block: check bdev_read_only() from blkdev_get()"). That commit added
stricter checking to make sure devices that were being used read-only
were actually opened in that mode.

It turns out that the change breaks a bunch of kernel code that opens
block devices. Affected systems include dm, md, and the loop device.
Because strict checking for read-only opens of block devices was not
done before this, the code that opens the devices was opening them
read-write even if they were being used read-only. Auditing all that
code will take time, and new userspace packages for dm, mdadm, etc.
will also be required.

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>

--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1215,12 +1215,6 @@ int blkdev_get(struct block_device *bdev
 
 	res = __blkdev_get(bdev, mode, 0);
 
-	/* __blkdev_get() may alter read only status, check it afterwards */
-	if (!res && (mode & FMODE_WRITE) && bdev_read_only(bdev)) {
-		__blkdev_put(bdev, mode, 0);
-		res = -EACCES;
-	}
-
 	if (whole) {
 		/* finish claiming */
 		mutex_lock(&bdev->bd_mutex);
@@ -1298,6 +1292,11 @@ struct block_device *blkdev_get_by_path(
 	if (err)
 		return ERR_PTR(err);
 
+	if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
+		blkdev_put(bdev, mode);
+		return ERR_PTR(-EACCES);
+	}
+
 	return bdev;
 }
 EXPORT_SYMBOL(blkdev_get_by_path);

Re: [Patch v2] block: revert block_dev read-only check

[Tejun Heo]

On Wed, Feb 16, 2011 at 06:11:53PM -0500, Chuck Ebbert wrote:
> This reverts commit 75f1dc0d076d1c1168f2115f1941ea627d38bd5a
> ("block: check bdev_read_only() from blkdev_get()"). That commit added
> stricter checking to make sure devices that were being used read-only
> were actually opened in that mode.
> 
> It turns out that the change breaks a bunch of kernel code that opens
> block devices. Affected systems include dm, md, and the loop device.
> Because strict checking for read-only opens of block devices was not
> done before this, the code that opens the devices was opening them
> read-write even if they were being used read-only. Auditing all that
> code will take time, and new userspace packages for dm, mdadm, etc.
> will also be required.

The problem is slightly more complex than that.  Block layer has never
enforced the read only flag during open or actual IOs and those
stacking block drivers and filesystems can open a device marked RO and
then write to it, which is a pretty bad idea (especially in data
rescue/forensic scenarios).

The commit was part of effort to enforce the ro flag.  It at least
makes sure that a device can't be opened RW if marked RO.  loop and dm
showed some problems but fixing the in-kernel part isn't difficult
(fixes pending).

IIUC, the problematic part is dm userland, which reportedly opens
member devices RW even when building a RO device.  The problem is when
a user is trying to build RO dm device from RO member devices.  dm
userland tries to open the member devices RW, which block layer
rejects now thus failing dm assembly.

> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1215,12 +1215,6 @@ int blkdev_get(struct block_device *bdev
>  
>  	res = __blkdev_get(bdev, mode, 0);
>  
> -	/* __blkdev_get() may alter read only status, check it afterwards */
> -	if (!res && (mode & FMODE_WRITE) && bdev_read_only(bdev)) {
> -		__blkdev_put(bdev, mode, 0);
> -		res = -EACCES;
> -	}
> -
>  	if (whole) {
>  		/* finish claiming */
>  		mutex_lock(&bdev->bd_mutex);
> @@ -1298,6 +1292,11 @@ struct block_device *blkdev_get_by_path(
>  	if (err)
>  		return ERR_PTR(err);
>  
> +	if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
> +		blkdev_put(bdev, mode);
> +		return ERR_PTR(-EACCES);
> +	}
> +
>  	return bdev;
>  }
>  EXPORT_SYMBOL(blkdev_get_by_path);

Shouldn't the changes to drivers/usb/gadget/storage_common.c reverted
too?

Thanks.

--
tejun

Re: [Patch v2] block: revert block_dev read-only check

[Linus Torvalds]

On Wed, Feb 16, 2011 at 3:28 PM, Tejun Heo <tj@kernel.org> wrote:
>
> The commit was part of effort to enforce the ro flag.  It at least
> makes sure that a device can't be opened RW if marked RO.  loop and dm
> showed some problems but fixing the in-kernel part isn't difficult
> (fixes pending).

Well, if the thing breaks things, then it needs to be reverted, or
those fixes need to not be "pending".

However:

> IIUC, the problematic part is dm userland, which reportedly opens
> member devices RW even when building a RO device.  The problem is when
> a user is trying to build RO dm device from RO member devices.  dm
> userland tries to open the member devices RW, which block layer
> rejects now thus failing dm assembly.

.. this makes me all the more suspicious. Sounds unfixable in a single
release (or even a few years). So it smells like we should (a) do the
revert and (b) add a warning for the case where somebody opens a
device RW where the low-level device itself is RO.

That said, if the user has permission to open the device RW (and the
normal device node permission checks have obviously always done that
check), I do think it's perfectly ok to do that. And if the user never
writes to it, then the fact that the device isn't writable is
irrelevant.

So maybe even the warning is worthless. But we can try it and see how
annoying it would be.

                      Linus

Re: [Patch v2] block: revert block_dev read-only check

[Tejun Heo]

Helo,

On Wed, Feb 16, 2011 at 03:56:10PM -0800, Linus Torvalds wrote:
> > The commit was part of effort to enforce the ro flag.  It at least
> > makes sure that a device can't be opened RW if marked RO.  loop and dm
> > showed some problems but fixing the in-kernel part isn't difficult
> > (fixes pending).
> 
> Well, if the thing breaks things, then it needs to be reverted, or
> those fixes need to not be "pending".

They should be mergeable once Jens comes back.

> > IIUC, the problematic part is dm userland, which reportedly opens
> > member devices RW even when building a RO device.  The problem is when
> > a user is trying to build RO dm device from RO member devices.  dm
> > userland tries to open the member devices RW, which block layer
> > rejects now thus failing dm assembly.
> 
> .. this makes me all the more suspicious. Sounds unfixable in a single
> release (or even a few years). So it smells like we should (a) do the
> revert and (b) add a warning for the case where somebody opens a
> device RW where the low-level device itself is RO.

Yeap, I'm not against reverting.  That is likely the right thing to do
in this cycle anyway.

> That said, if the user has permission to open the device RW (and the
> normal device node permission checks have obviously always done that
> check), I do think it's perfectly ok to do that. And if the user never
> writes to it, then the fact that the device isn't writable is
> irrelevant.

It has been a while so the details might be a bit off but read/write
permissions on block devices are rather weird.

* RO block devices can be opened RW.

* Block devices can get writes whether the device is opened RO or RW.

* Filesystem journal replay and probably some of stacking block driver
  metadata updates issue writes to RO block devices.

So, IIUC, there already are paths where writes are issued and
processed where they shouldn't be.  Everything is RO but the block
device ends up being modified.

Thanks.

--
tejun

Re: [Patch v2] block: revert block_dev read-only check

[Linus Torvalds]

On Wed, Feb 16, 2011 at 4:23 PM, Tejun Heo <tj@kernel.org> wrote:
>
> It has been a while so the details might be a bit off but read/write
> permissions on block devices are rather weird.
>
> * RO block devices can be opened RW.

Well, yes and no.

If the device node is RW, then that's often the most important part.
Whether the device itself then thinks it is read-only or not is almost
totally irrelevant. The internal "bdev_read_only()" thing is very much
a secondary thing, and has nothing to do with security, and everything
to do with random block device internals. So don't take it too
seriously.

BUT.

Some device drivers have actually done a good job historically, and
check the RW flags at open time. The only one I know of is the
traditional floppy.c, though.

HOWEVER - even then it also checks the FMODE_NDELAY, and skips all the
checks (including for media) if that bit isn't set. Because being too
anal about it is simply _wrong_. You may need to override the RO state
of the device, and you may need to open it writably in order to do so!

Because to make things even more complicated, even if the open
succeeded, the floppy driver will then check the writable bit (that it
might have ignored at open time) for certain ioctl's.

End result: it's almost certainly wrong to think that you can stop RW
open calls based on whether the device is somehow read-only. Even on a
physically read-only device you may need to have write permissions to
do certain operations.

So quite frankly, if you want to enforce read-only, you should
probably do it at command queueing time, not at open() time. Because
at open time, you just don't know enough.

                         Linus