Re: [PATCH] stream: Don't crash when node permission is denied

[Vladimir Sementsov-Ogievskiy]

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