[for-5.1,2/3] file-posix: Fix check_hdev_writable() with auto-read-only

Commit Message

Kevin Wolf July 17, 2020, 10:54 a.m. UTC

For Linux block devices, being able to open the device read-write
doesn't necessarily mean that the device is actually writable (one
example is a read-only LV, as you get with lvchange -pr <device>). We
have check_hdev_writable() to check this condition and fail opening the
image read-write if it's not actually writable.

However, this check doesn't take auto-read-only into account, but
results in a hard failure instead of downgrading to read-only where
possible.

Fix this and do the writable check not based on BDRV_O_RDWR, but only
when this actually results in opening the file read-write. A second
check is inserted in raw_reconfigure_getfd() to have the same check when
dynamic auto-read-only upgrades an image file from read-only to
read-write.

Signed-off-by: Kevin Wolf <[e-mail]>
---
 block/file-posix.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

Comments

Max Reitz July 17, 2020, 11:50 a.m. UTC | #1

On 17.07.20 12:54, Kevin Wolf wrote:
> For Linux block devices, being able to open the device read-write
> doesn't necessarily mean that the device is actually writable (one
> example is a read-only LV, as you get with lvchange -pr <device>). We
> have check_hdev_writable() to check this condition and fail opening the
> image read-write if it's not actually writable.
> 
> However, this check doesn't take auto-read-only into account, but
> results in a hard failure instead of downgrading to read-only where
> possible.
> 
> Fix this and do the writable check not based on BDRV_O_RDWR, but only
> when this actually results in opening the file read-write. A second
> check is inserted in raw_reconfigure_getfd() to have the same check when
> dynamic auto-read-only upgrades an image file from read-only to
> read-write.
> 
> Signed-off-by: Kevin Wolf <[e-mail]>
> ---
>  block/file-posix.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz <[e-mail]>

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index e35e15185a..659f780570 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -401,7 +401,7 @@  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     }
 }
 
-static int check_hdev_writable(BDRVRawState *s)
+static int check_hdev_writable(int fd)
 {
 #if defined(BLKROGET)
     /* Linux block devices can be configured "read-only" using blockdev(8).
@@ -415,7 +415,7 @@  static int check_hdev_writable(BDRVRawState *s)
     struct stat st;
     int readonly = 0;
 
-    if (fstat(s->fd, &st)) {
+    if (fstat(fd, &st)) {
         return -errno;
     }
 
@@ -423,7 +423,7 @@  static int check_hdev_writable(BDRVRawState *s)
         return 0;
     }
 
-    if (ioctl(s->fd, BLKROGET, &readonly) < 0) {
+    if (ioctl(fd, BLKROGET, &readonly) < 0) {
         return -errno;
     }
 
@@ -618,6 +618,15 @@  static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     s->fd = fd;
 
+    /* Check s->open_flags rather than bdrv_flags due to auto-read-only */
+    if (s->open_flags & O_RDWR) {
+        ret = check_hdev_writable(s->fd);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "The device is not writable");
+            goto fail;
+        }
+    }
+
     s->perm = 0;
     s->shared_perm = BLK_PERM_ALL;
 
@@ -1010,6 +1019,15 @@  static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
         }
     }
 
+    if (fd != -1 && (*open_flags & O_RDWR)) {
+        ret = check_hdev_writable(fd);
+        if (ret < 0) {
+            qemu_close(fd);
+            error_setg_errno(errp, -ret, "The device is not writable");
+            return -1;
+        }
+    }
+
     return fd;
 }
 
@@ -3454,15 +3472,6 @@  hdev_open_Mac_error:
     /* Since this does ioctl the device must be already opened */
     bs->sg = hdev_is_sg(bs);
 
-    if (flags & BDRV_O_RDWR) {
-        ret = check_hdev_writable(s);
-        if (ret < 0) {
-            raw_close(bs);
-            error_setg_errno(errp, -ret, "The device is not writable");
-            return ret;
-        }
-    }
-
     return ret;
 }