Re: [Qemu-devel] [PATCH v5] file-posix: specify expected filetypes

[Kevin Wolf <kwolf@redhat.com>]

Am 21.03.2018 um 21:01 hat John Snow geschrieben:
> Adjust each caller of raw_open_common to specify if they are expecting
> host and character devices or not. Tighten expectations of file types upon
> open in the common code and refuse types that are not expected.
> 
> This has two effects:
> 
> (1) Character and block devices are now considered deprecated for the
>     'file' driver, which expects only S_IFREG, and
> (2) no file-posix driver (file, host_cdrom, or host_device) can open
>     directories now.
> 
> I don't think there's a legitimate reason to open directories as if
> they were files. This prevents QEMU from opening and attempting to probe
> a directory inode, which can break in exciting ways. One of those ways
> is lseek on ext4/xfs, which will return 0x7fffffffffffffff as the file
> size instead of EISDIR. This can coax QEMU into responding with a
> confusing "file too big" instead of "Hey, that's not a file".
> 
> See: https://bugs.launchpad.net/qemu/+bug/1739304/
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 
> v5: rebase for 2.12.0-rc0
> 
>  block/file-posix.c | 37 +++++++++++++++++++++++++++++--------
>  qemu-doc.texi      |  6 ++++++
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d7fb772c14..31d9afe026 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -420,7 +420,8 @@ static QemuOptsList raw_runtime_opts = {
>  };
>  
>  static int raw_open_common(BlockDriverState *bs, QDict *options,
> -                           int bdrv_flags, int open_flags, Error **errp)
> +                           int bdrv_flags, int open_flags,
> +                           bool device, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
>      QemuOpts *opts;
> @@ -558,10 +559,30 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          error_setg_errno(errp, errno, "Could not stat file");
>          goto fail;
>      }
> -    if (S_ISREG(st.st_mode)) {
> -        s->discard_zeroes = true;
> -        s->has_fallocate = true;
> +
> +    if (!device) {
> +        if (S_ISBLK(st.st_mode)) {
> +            warn_report("Opening a block device as file using 'file' "
> +                        "driver is deprecated");
> +        } else if (S_ISCHR(st.st_mode)) {
> +            warn_report("Opening a character device as file using the 'file' "
> +                        "driver is deprecated");
> +        } else if (!S_ISREG(st.st_mode)) {
> +            error_setg(errp, "A regular file was expected by the 'file' driver, "
> +                       "but something else was given");
> +            goto fail;

ret needs to be set here, otherwise we return success. In my test, I
still got the wrong message: "Could not refresh total sector count:
Invalid argument"

> +        } else {
> +            s->discard_zeroes = true;
> +            s->has_fallocate = true;
> +        }
> +    } else {
> +        if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
> +            error_setg(errp, "host_device/host_cdrom driver expects either "
> +                       "a character or block device");
> +            goto fail;

Same here.

> +        }
>      }

Do we want a qemu-iotests case for this?

Kevin