Re: [PATCH] stream: Don't crash when node permission is denied
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
Signed-off-by: Kevin Wolf
---
Based-on: 20210309121814.31078-1-kw...@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.
+<
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [PATCH] stream: Don't crash when node permission is denied
On Tue 09 Mar 2021 06:34:51 PM CET, 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 > Signed-off-by: Kevin Wolf Reviewed-by: Alberto Garcia Berto
Re: [PATCH] stream: Don't crash when node permission is denied
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
> Signed-off-by: Kevin Wolf
> ---
> Based-on: 20210309121814.31078-1-kw...@redhat.com
> ('storage-daemon: Call job_cancel_sync_all() on shutdown')
>
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH] stream: Don't crash when node permission is denied
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
Signed-off-by: Kevin Wolf
---
Based-on: 20210309121814.31078-1-kw...@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 \
+<
