Patchwork block/raw-posix: detect readonly LVM volumes using BLKROGET

login
register
mail settings
Submitter Stefan Hajnoczi
Date Feb. 4, 2013, 2:08 p.m.
Message ID <1359986911-5444-1-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/217934/
State New
Headers show

Comments

Stefan Hajnoczi - Feb. 4, 2013, 2:08 p.m.
LVM volumes can be set read-only with "lvchange --permission r
<volume>".  The device node permissions remain unchanged so the volume
can still be opened O_RDWR.  Actual writes will fail.

This results in odd behavior for QEMU.  bdrv_open() is supposed to fail
if a read-only image is being opened with BDRV_O_RDWR.  By not failing
for LVM volumes, the guest boots up but every write produces an I/O
error.

This patch checks whether the block device is read-only so that LVM
volumes behave like regular files.

Reported-by: Sibiao Luo <sluo@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/raw-posix.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)
Markus Armbruster - Feb. 4, 2013, 2:49 p.m.
Stefan Hajnoczi <stefanha@redhat.com> writes:

> LVM volumes can be set read-only with "lvchange --permission r
> <volume>".  The device node permissions remain unchanged so the volume
> can still be opened O_RDWR.  Actual writes will fail.

Are you sure it's just LVM?  blockdev(8) suggests it's a more general
issue.  Check out --setro there.

> This results in odd behavior for QEMU.  bdrv_open() is supposed to fail
> if a read-only image is being opened with BDRV_O_RDWR.  By not failing
> for LVM volumes, the guest boots up but every write produces an I/O
> error.
>
> This patch checks whether the block device is read-only so that LVM
> volumes behave like regular files.
>
> Reported-by: Sibiao Luo <sluo@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Patch looks good to me.
Stefan Hajnoczi - Feb. 5, 2013, 11:21 a.m.
On Mon, Feb 04, 2013 at 03:49:10PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > LVM volumes can be set read-only with "lvchange --permission r
> > <volume>".  The device node permissions remain unchanged so the volume
> > can still be opened O_RDWR.  Actual writes will fail.
> 
> Are you sure it's just LVM?  blockdev(8) suggests it's a more general
> issue.  Check out --setro there.

You are right.  I tested "blockdev --setro" and got the same behavior.
I'll resend with comments indicating Linux block devices in general,
instead of just LVM.

Stefan

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 8b6b926..2ab4da4 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1257,9 +1257,42 @@  static int hdev_probe_device(const char *filename)
     return 0;
 }
 
+static int check_hdev_writable(BDRVRawState *s)
+{
+#if defined(BLKROGET)
+    /* Linux LVM volumes can be configured "read-only" using lvchange(8).  This
+     * does not change permissions on the device node and therefore open(2)
+     * with O_RDWR succeeds.  Actual writes fail with EPERM.
+     *
+     * bdrv_open() is supposed to fail if the disk is read-only.  Explicitly
+     * check for read-only block devices so that LVM volumes behave properly.
+     */
+    struct stat st;
+    int readonly = 0;
+
+    if (fstat(s->fd, &st)) {
+        return -errno;
+    }
+
+    if (!S_ISBLK(st.st_mode)) {
+        return 0;
+    }
+
+    if (ioctl(s->fd, BLKROGET, &readonly) < 0) {
+        return -errno;
+    }
+
+    if (readonly) {
+        return -EACCES;
+    }
+#endif /* defined(BLKROGET) */
+    return 0;
+}
+
 static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
+    int ret;
 
 #if defined(__APPLE__) && defined(__MACH__)
     if (strstart(filename, "/dev/cdrom", NULL)) {
@@ -1300,7 +1333,20 @@  static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
     }
 #endif
 
-    return raw_open_common(bs, filename, flags, 0);
+    ret = raw_open_common(bs, filename, flags, 0);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (flags & BDRV_O_RDWR) {
+        ret = check_hdev_writable(s);
+        if (ret < 0) {
+            raw_close(bs);
+            return ret;
+        }
+    }
+
+    return ret;
 }
 
 #if defined(__linux__)