| Message ID | 20210309173451.45152-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stream: Don't crash when node permission is denied | expand |
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>
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
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>
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
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(-)