file-posix: Check effective size in truncate operation

Commit Message

zhenwei pi Nov. 26, 2018, 3:57 a.m. UTC

Function raw_co_truncate does not check effective size for BLK device file,
and QEMU may notify guest without any size changing.

Two cases can be reproduced easily by qmp command:
CASE 1:
1, create a logical volume(12M) by LVM, and guest uses this volume as "vdb"
2, run qmp command : virsh qemu-monitor-command INSTANCE '{"execute":
"block_resize", "arguments":{"device":"drive-virtio-disk1","size":12582912}}'

The effective size(12M) is equal to the argument(12M) and the real device file
size(12M). QEMU should ignore this command and has no need to notify guest.

CASE 2:
1, create a logical volume(12M) by LVM, and guest uses this volume as "vdb"
2, resize LV to 16M by lvresize command
3, run qmp command : virsh qemu-monitor-command INSTANCE '{"execute":
"block_resize", "arguments":{"device":"drive-virtio-disk1","size":10485760}}'

The device file size actually grew, but the argument(10M) is less than the
effective size(12M). This command should fail, but QEMU still report success.

Signed-off-by: zhenwei pi <[e-mail]>
---
 block/file-posix.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Max Reitz Jan. 14, 2019, 10:46 a.m. UTC | #1

On 26.11.18 04:57, zhenwei pi wrote:
> Function raw_co_truncate does not check effective size for BLK device file,
> and QEMU may notify guest without any size changing.
> 
> Two cases can be reproduced easily by qmp command:
> CASE 1:
> 1, create a logical volume(12M) by LVM, and guest uses this volume as "vdb"
> 2, run qmp command : virsh qemu-monitor-command INSTANCE '{"execute":
> "block_resize", "arguments":{"device":"drive-virtio-disk1","size":12582912}}'
> 
> The effective size(12M) is equal to the argument(12M) and the real device file
> size(12M). QEMU should ignore this command and has no need to notify guest.

I don't quite see the issue here.  The command is valid, why would it be
an error?  And I don't see the harm in notifying the guest either.

> CASE 2:
> 1, create a logical volume(12M) by LVM, and guest uses this volume as "vdb"
> 2, resize LV to 16M by lvresize command
> 3, run qmp command : virsh qemu-monitor-command INSTANCE '{"execute":
> "block_resize", "arguments":{"device":"drive-virtio-disk1","size":10485760}}'
> 
> The device file size actually grew, but the argument(10M) is less than the
> effective size(12M). This command should fail, but QEMU still report success.

Nor do I see a real problem here.  It isn't dangerous if qemu trusts the
user, but it may break existing use cases to error out if the user
specifies a length that's too short.

In fact, I would suppose if you want to shrink an LVM volume, the
correct way to do it is to first shrink the virtual device in qemu (with
block_resize), and only then shrink the physical device on the host;
otherwise, qemu might issue requests beyond the end of the physical
disk.  This change would break that.

Max

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index 07bbdab953..951d910b0b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1991,6 +1991,7 @@  static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
     BDRVRawState *s = bs->opaque;
     struct stat st;
     int ret;
+    int64_t sectors;
 
     if (fstat(s->fd, &st)) {
         ret = -errno;
@@ -2013,6 +2014,20 @@  static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
             error_setg(errp, "Cannot grow device files");
             return -EINVAL;
         }
+
+        sectors = raw_getlength(bs) >> BDRV_SECTOR_BITS;
+        if (sectors > bs->total_sectors) {
+            /* device size actually grew */
+            if (offset <= bs->total_sectors * BDRV_SECTOR_SIZE) {
+                error_setg(errp, "The effective size of this device is "
+                        "greater than or equal to the argument");
+                return -EINVAL;
+            }
+        } else if (sectors == bs->total_sectors) {
+            /* device size actually not changed */
+            error_setg(errp, "Detect device file size not changing");
+            return -EINVAL;
+        }
     } else {
         error_setg(errp, "Resizing this file is not supported");
         return -ENOTSUP;