* [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only
@ 2020-07-17 10:54 Kevin Wolf
2020-07-17 10:54 ` [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up Kevin Wolf
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Kevin Wolf @ 2020-07-17 10:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up
2020-07-17 10:54 [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
@ 2020-07-17 10:54 ` Kevin Wolf
2020-07-17 11:45 ` Max Reitz
2020-07-17 10:54 ` [PATCH for-5.1 2/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2020-07-17 10:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH for-5.1 2/3] file-posix: Fix check_hdev_writable() with auto-read-only
2020-07-17 10:54 [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
2020-07-17 10:54 ` [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up Kevin Wolf
@ 2020-07-17 10:54 ` Kevin Wolf
2020-07-17 11:50 ` Max Reitz
2020-07-17 10:54 ` [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path Kevin Wolf
2020-07-17 11:49 ` [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only no-reply
3 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2020-07-17 10:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path
2020-07-17 10:54 [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
2020-07-17 10:54 ` [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up Kevin Wolf
2020-07-17 10:54 ` [PATCH for-5.1 2/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
@ 2020-07-17 10:54 ` Kevin Wolf
2020-07-17 11:55 ` Max Reitz
2020-07-17 11:49 ` [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only no-reply
3 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2020-07-17 10:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up
2020-07-17 10:54 ` [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up Kevin Wolf
@ 2020-07-17 11:45 ` Max Reitz
0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2020-07-17 11:45 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only
2020-07-17 10:54 [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
` (2 preceding siblings ...)
2020-07-17 10:54 ` [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path Kevin Wolf
@ 2020-07-17 11:49 ` no-reply
3 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2020-07-17 11:49 UTC (permalink / raw)
To: kwolf; +Cc: kwolf, qemu-devel, qemu-block, mreitz
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-5.1 2/3] file-posix: Fix check_hdev_writable() with auto-read-only
2020-07-17 10:54 ` [PATCH for-5.1 2/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
@ 2020-07-17 11:50 ` Max Reitz
0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2020-07-17 11:50 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path
2020-07-17 10:54 ` [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path Kevin Wolf
@ 2020-07-17 11:55 ` Max Reitz
0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2020-07-17 11:55 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-17 11:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 10:54 [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
2020-07-17 10:54 ` [PATCH for-5.1 1/3] file-posix: Move check_hdev_writable() up Kevin Wolf
2020-07-17 11:45 ` Max Reitz
2020-07-17 10:54 ` [PATCH for-5.1 2/3] file-posix: Fix check_hdev_writable() with auto-read-only Kevin Wolf
2020-07-17 11:50 ` Max Reitz
2020-07-17 10:54 ` [PATCH for-5.1 3/3] file-posix: Fix leaked fd in raw_open_common() error path Kevin Wolf
2020-07-17 11:55 ` Max Reitz
2020-07-17 11:49 ` [PATCH for-5.1 0/3] file-posix: Fix check_hdev_writable() with auto-read-only no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.