Tags Tags

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

February 16th, 2011 - 06:20 pm ET by Chuck Ebbert | Report spam
Help Create a new topic
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);
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
email Follow the discussionReplies 7 repliesReplies Make a reply

Replies

#1 Tejun Heo
February 16th, 2011 - 06:30 pm ET | Report spam
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
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Similar topics

Make your own search :