* [PATCH] stream: Don't crash when node permission is denied
@ 2021-03-09 17:34 Kevin Wolf
2021-03-09 20:20 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Kevin Wolf @ 2021-03-09 17:34 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, ngu, qemu-devel, mreitz
The image streaming block job restricts shared permissions of the nodes
it accesses. This can obviously fail when other users already got these
permissions. &error_abort is therefore wrong and can crash. Handle these
errors gracefully and just fail starting the block job.
Reported-by: Nini Gu <ngu@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
Based-on: 20210309121814.31078-1-kwolf@redhat.com
('storage-daemon: Call job_cancel_sync_all() on shutdown')
block/stream.c | 15 +++++++++++----
tests/qemu-iotests/tests/qsd-jobs | 20 ++++++++++++++++++++
tests/qemu-iotests/tests/qsd-jobs.out | 10 ++++++++++
3 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 1fa742b0db..97bee482dc 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -206,7 +206,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
const char *filter_node_name,
Error **errp)
{
- StreamBlockJob *s;
+ StreamBlockJob *s = NULL;
BlockDriverState *iter;
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
@@ -214,6 +214,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *cor_filter_bs = NULL;
BlockDriverState *above_base;
QDict *opts;
+ int ret;
assert(!(base && bottom));
assert(!(backing_file_str && bottom));
@@ -303,7 +304,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* queried only at the job start and then cached.
*/
if (block_job_add_bdrv(&s->common, "active node", bs, 0,
- basic_flags | BLK_PERM_WRITE, &error_abort)) {
+ basic_flags | BLK_PERM_WRITE, errp)) {
goto fail;
}
@@ -320,8 +321,11 @@ void stream_start(const char *job_id, BlockDriverState *bs,
for (iter = bdrv_filter_or_cow_bs(bs); iter != base;
iter = bdrv_filter_or_cow_bs(iter))
{
- block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
- basic_flags, &error_abort);
+ ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
+ basic_flags, errp);
+ if (ret < 0) {
+ goto fail;
+ }
}
s->base_overlay = base_overlay;
@@ -337,6 +341,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
return;
fail:
+ if (s) {
+ job_early_fail(&s->common.job);
+ }
if (cor_filter_bs) {
bdrv_cor_filter_drop(cor_filter_bs);
}
diff --git a/tests/qemu-iotests/tests/qsd-jobs b/tests/qemu-iotests/tests/qsd-jobs
index 1a1c534fac..972b6b3898 100755
--- a/tests/qemu-iotests/tests/qsd-jobs
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -30,6 +30,7 @@ status=1 # failure is the default!
_cleanup()
{
_cleanup_test_img
+ rm -f "$SOCK_DIR/nbd.sock"
}
trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -59,6 +60,25 @@ $QSD --chardev stdio,id=stdio --monitor chardev=stdio \
{"execute": "quit"}
EOF
+echo
+echo "=== Streaming can't get permission on base node ==="
+echo
+
+# Just make sure that this doesn't crash
+$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
+ --blockdev node-name=file_base,driver=file,filename="$TEST_IMG.base" \
+ --blockdev node-name=fmt_base,driver=qcow2,file=file_base \
+ --blockdev node-name=file_overlay,driver=file,filename="$TEST_IMG" \
+ --blockdev node-name=fmt_overlay,driver=qcow2,file=file_overlay,backing=fmt_base \
+ --nbd-server addr.type=unix,addr.path="$SOCK_DIR/nbd.sock" \
+ --export type=nbd,id=export1,node-name=fmt_base,writable=on,name=export1 \
+ <<EOF | _filter_qmp
+{"execute": "qmp_capabilities"}
+{"execute": "block-stream",
+ "arguments": {"device": "fmt_overlay", "job-id": "job0"}}
+{"execute": "quit"}
+EOF
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out
index e3a684b34d..05e1165e80 100644
--- a/tests/qemu-iotests/tests/qsd-jobs.out
+++ b/tests/qemu-iotests/tests/qsd-jobs.out
@@ -19,4 +19,14 @@ QMP_VERSION
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
+
+=== Streaming can't get permission on base node ===
+
+QMP_VERSION
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
+{"error": {"class": "GenericError", "desc": "Conflicts with use by a block device as 'root', which uses 'write' on fmt_base"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}}
*** done
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] stream: Don't crash when node permission is denied
2021-03-09 17:34 [PATCH] stream: Don't crash when node permission is denied Kevin Wolf
@ 2021-03-09 20:20 ` Eric Blake
2021-03-10 10:20 ` Alberto Garcia
2021-03-12 10:44 ` Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 4+ messages in thread
From: Eric Blake @ 2021-03-09 20:20 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: ngu, qemu-devel, mreitz
On 3/9/21 11:34 AM, Kevin Wolf wrote:
> The image streaming block job restricts shared permissions of the nodes
> it accesses. This can obviously fail when other users already got these
> permissions. &error_abort is therefore wrong and can crash. Handle these
> errors gracefully and just fail starting the block job.
>
> Reported-by: Nini Gu <ngu@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> Based-on: 20210309121814.31078-1-kwolf@redhat.com
> ('storage-daemon: Call job_cancel_sync_all() on shutdown')
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] stream: Don't crash when node permission is denied
2021-03-09 17:34 [PATCH] stream: Don't crash when node permission is denied Kevin Wolf
2021-03-09 20:20 ` Eric Blake
@ 2021-03-10 10:20 ` Alberto Garcia
2021-03-12 10:44 ` Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 4+ messages in thread
From: Alberto Garcia @ 2021-03-10 10:20 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: kwolf, ngu, qemu-devel, mreitz
On Tue 09 Mar 2021 06:34:51 PM CET, Kevin Wolf <kwolf@redhat.com> wrote:
> The image streaming block job restricts shared permissions of the nodes
> it accesses. This can obviously fail when other users already got these
> permissions. &error_abort is therefore wrong and can crash. Handle these
> errors gracefully and just fail starting the block job.
>
> Reported-by: Nini Gu <ngu@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] stream: Don't crash when node permission is denied
2021-03-09 17:34 [PATCH] stream: Don't crash when node permission is denied Kevin Wolf
2021-03-09 20:20 ` Eric Blake
2021-03-10 10:20 ` Alberto Garcia
@ 2021-03-12 10:44 ` Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 4+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-03-12 10:44 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: ngu, qemu-devel, mreitz
09.03.2021 20:34, Kevin Wolf wrote:
> The image streaming block job restricts shared permissions of the nodes
> it accesses. This can obviously fail when other users already got these
> permissions. &error_abort is therefore wrong and can crash. Handle these
> errors gracefully and just fail starting the block job.
>
> Reported-by: Nini Gu <ngu@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> Based-on: 20210309121814.31078-1-kwolf@redhat.com
> ('storage-daemon: Call job_cancel_sync_all() on shutdown')
>
> block/stream.c | 15 +++++++++++----
> tests/qemu-iotests/tests/qsd-jobs | 20 ++++++++++++++++++++
> tests/qemu-iotests/tests/qsd-jobs.out | 10 ++++++++++
> 3 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 1fa742b0db..97bee482dc 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -206,7 +206,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> const char *filter_node_name,
> Error **errp)
> {
> - StreamBlockJob *s;
> + StreamBlockJob *s = NULL;
> BlockDriverState *iter;
> bool bs_read_only;
> int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> @@ -214,6 +214,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *cor_filter_bs = NULL;
> BlockDriverState *above_base;
> QDict *opts;
> + int ret;
>
> assert(!(base && bottom));
> assert(!(backing_file_str && bottom));
> @@ -303,7 +304,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> * queried only at the job start and then cached.
> */
> if (block_job_add_bdrv(&s->common, "active node", bs, 0,
> - basic_flags | BLK_PERM_WRITE, &error_abort)) {
> + basic_flags | BLK_PERM_WRITE, errp)) {
While being here may also do ", errp) < 0) {", for more usual pattern of checking error..
> goto fail;
> }
>
> @@ -320,8 +321,11 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> for (iter = bdrv_filter_or_cow_bs(bs); iter != base;
> iter = bdrv_filter_or_cow_bs(iter))
> {
> - block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> - basic_flags, &error_abort);
> + ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
> + basic_flags, errp);
> + if (ret < 0) {
> + goto fail;
> + }
> }
>
> s->base_overlay = base_overlay;
> @@ -337,6 +341,9 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> return;
>
> fail:
> + if (s) {
> + job_early_fail(&s->common.job);
> + }
> if (cor_filter_bs) {
> bdrv_cor_filter_drop(cor_filter_bs);
> }
> diff --git a/tests/qemu-iotests/tests/qsd-jobs b/tests/qemu-iotests/tests/qsd-jobs
> index 1a1c534fac..972b6b3898 100755
> --- a/tests/qemu-iotests/tests/qsd-jobs
> +++ b/tests/qemu-iotests/tests/qsd-jobs
> @@ -30,6 +30,7 @@ status=1 # failure is the default!
> _cleanup()
> {
> _cleanup_test_img
> + rm -f "$SOCK_DIR/nbd.sock"
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
>
> @@ -59,6 +60,25 @@ $QSD --chardev stdio,id=stdio --monitor chardev=stdio \
> {"execute": "quit"}
> EOF
>
> +echo
> +echo "=== Streaming can't get permission on base node ==="
> +echo
> +
> +# Just make sure that this doesn't crash
> +$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
> + --blockdev node-name=file_base,driver=file,filename="$TEST_IMG.base" \
> + --blockdev node-name=fmt_base,driver=qcow2,file=file_base \
> + --blockdev node-name=file_overlay,driver=file,filename="$TEST_IMG" \
> + --blockdev node-name=fmt_overlay,driver=qcow2,file=file_overlay,backing=fmt_base \
> + --nbd-server addr.type=unix,addr.path="$SOCK_DIR/nbd.sock" \
> + --export type=nbd,id=export1,node-name=fmt_base,writable=on,name=export1 \
Another option is to use blkdebug with take-child-perms and/or unshare-child-perms set. Just a note, nbd is good too.
> + <<EOF | _filter_qmp
> +{"execute": "qmp_capabilities"}
> +{"execute": "block-stream",
> + "arguments": {"device": "fmt_overlay", "job-id": "job0"}}
> +{"execute": "quit"}
> +EOF
> +
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out
> index e3a684b34d..05e1165e80 100644
> --- a/tests/qemu-iotests/tests/qsd-jobs.out
> +++ b/tests/qemu-iotests/tests/qsd-jobs.out
> @@ -19,4 +19,14 @@ QMP_VERSION
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
> +
> +=== Streaming can't get permission on base node ===
> +
> +QMP_VERSION
> +{"return": {}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
> +{"error": {"class": "GenericError", "desc": "Conflicts with use by a block device as 'root', which uses 'write' on fmt_base"}}
> +{"return": {}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}}
> *** done
>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-12 10:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-09 17:34 [PATCH] stream: Don't crash when node permission is denied Kevin Wolf
2021-03-09 20:20 ` Eric Blake
2021-03-10 10:20 ` Alberto Garcia
2021-03-12 10:44 ` Vladimir Sementsov-Ogievskiy
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.