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

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

[Kevin Wolf]

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1828252

Kevin Wolf (3):
  file-posix: Move check_hdev_writable() up
  file-posix: Fix check_hdev_writable() with auto-read-only
  file-posix: Fix leaked fd in raw_open_common() error path

 block/file-posix.c | 96 ++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 42 deletions(-)

-- 
2.25.4

[PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up

[Kevin Wolf]

We'll need to call it in raw_open_common(), so move the function to
avoid a forward declaration.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 66 +++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 8067e238cb..e35e15185a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -401,6 +401,39 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     }
 }
 
+static int check_hdev_writable(BDRVRawState *s)
+{
+#if defined(BLKROGET)
+    /* Linux block devices can be configured "read-only" using blockdev(8).
+     * This is independent of device node permissions 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 Linux block devices 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 void raw_parse_flags(int bdrv_flags, int *open_flags, bool has_writers)
 {
     bool read_write = false;
@@ -3299,39 +3332,6 @@ static int hdev_probe_device(const char *filename)
     return 0;
 }
 
-static int check_hdev_writable(BDRVRawState *s)
-{
-#if defined(BLKROGET)
-    /* Linux block devices can be configured "read-only" using blockdev(8).
-     * This is independent of device node permissions 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 Linux block devices 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 void hdev_parse_filename(const char *filename, QDict *options,
                                 Error **errp)
 {
-- 
2.25.4

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

[Kevin Wolf]

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 <kwolf@redhat.com>
---
 block/file-posix.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

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;
 }
 
-- 
2.25.4

[PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 659f780570..b2ed9d7eb2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -749,6 +749,9 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     ret = 0;
 fail:
+    if (ret < 0 && s->fd != -1) {
+        qemu_close(s->fd);
+    }
     if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
         unlink(filename);
     }
-- 
2.25.4

Re: [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 377 bytes --]

On 17.07.20 12:54, Kevin Wolf wrote:
> We'll need to call it in raw_open_common(), so move the function to
> avoid a forward declaration.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/file-posix.c | 66 +++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200717105426.51134-1-kwolf@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-qtest-x86_64: tests/qtest/qmp-test
  TEST    check-qtest-x86_64: tests/qtest/qmp-cmd-test
**
ERROR:/tmp/qemu-test/src/tests/qtest/libqtest.c:97:init_socket: assertion failed (ret != -1): (-1 != -1)
ERROR qmp-cmd-test - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/libqtest.c:97:init_socket: assertion failed (ret != -1): (-1 != -1)
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 080
  TEST    iotest-qcow2: 086
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=fc3463a17cea454c9a0fcd472358db4f', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-c3bzzbt8/src/docker-src.2020-07-17-07.34.41.28182:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=fc3463a17cea454c9a0fcd472358db4f
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-c3bzzbt8/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m51.065s
user    0m8.978s


The full log is available at
http://patchew.org/logs/20200717105426.51134-1-kwolf@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1062 bytes --]

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 <kwolf@redhat.com>
> ---
>  block/file-posix.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 208 bytes --]

On 17.07.20 12:54, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/file-posix.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]