[Qemu-devel] [PATCH for-3.1?] file-posix: Better checks of 64-bit copy_range

[Qemu-devel] [PATCH for-3.1?] file-posix: Better checks of 64-bit copy_range

[Eric Blake]

file-posix.c was taking a 64-bit bytes in raw_co_copy_range_to(),
passing it through a 32-bit parameter of paio_submit_co_full(),
then widening it back to size_t when assigning into acb->aio_nbytes.

Looking at io.c, I can't quickly tell if bdrv_co_copy_range_internal()
is fragmenting things to honor bs->bl.max_transfer, or if it can
accidentally send requests larger than 2G down to the driver. If
the former, then this is a no-op; if the latter, then someone needs
to find a way to trigger this assertion and patch the block layer
to properly fragment copy_range requests.  Either way, we're better
off with an assertion than the risk of silent data corruption.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/file-posix.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 58c86a01eaa..48ad3bb372a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1821,7 +1821,7 @@ static int aio_worker(void *arg)
 static int paio_submit_co_full(BlockDriverState *bs, int fd,
                                int64_t offset, int fd2, int64_t offset2,
                                QEMUIOVector *qiov,
-                               int bytes, int type)
+                               uint64_t bytes, int type)
 {
     RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
     ThreadPool *pool;
@@ -1832,6 +1832,7 @@ static int paio_submit_co_full(BlockDriverState *bs, int fd,
     acb->aio_fd2 = fd2;
     acb->aio_offset2 = offset2;

+    assert(bytes <= SIZE_MAX);
     acb->aio_nbytes = bytes;
     acb->aio_offset = offset;

@@ -1848,7 +1849,7 @@ static int paio_submit_co_full(BlockDriverState *bs, int fd,

 static inline int paio_submit_co(BlockDriverState *bs, int fd,
                                  int64_t offset, QEMUIOVector *qiov,
-                                 int bytes, int type)
+                                 uint64_t bytes, int type)
 {
     return paio_submit_co_full(bs, fd, offset, -1, 0, qiov, bytes, type);
 }
-- 
2.17.2

Re: [Qemu-devel] [PATCH for-3.1?] file-posix: Better checks of 64-bit copy_range

[Eric Blake]

On 11/14/18 3:05 PM, Eric Blake wrote:
> file-posix.c was taking a 64-bit bytes in raw_co_copy_range_to(),
> passing it through a 32-bit parameter of paio_submit_co_full(),
> then widening it back to size_t when assigning into acb->aio_nbytes.
> 
> Looking at io.c, I can't quickly tell if bdrv_co_copy_range_internal()
> is fragmenting things to honor bs->bl.max_transfer, or if it can
> accidentally send requests larger than 2G down to the driver. If
> the former, then this is a no-op; if the latter, then someone needs
> to find a way to trigger this assertion and patch the block layer
> to properly fragment copy_range requests.  Either way, we're better
> off with an assertion than the risk of silent data corruption.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   block/file-posix.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

This patch is not needed after Kevin's file-posix cleanups for post-release:
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02985.html

The question remains, though, if we still want this in 3.1.

> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 58c86a01eaa..48ad3bb372a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1821,7 +1821,7 @@ static int aio_worker(void *arg)
>   static int paio_submit_co_full(BlockDriverState *bs, int fd,
>                                  int64_t offset, int fd2, int64_t offset2,
>                                  QEMUIOVector *qiov,
> -                               int bytes, int type)
> +                               uint64_t bytes, int type)
>   {
>       RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
>       ThreadPool *pool;
> @@ -1832,6 +1832,7 @@ static int paio_submit_co_full(BlockDriverState *bs, int fd,
>       acb->aio_fd2 = fd2;
>       acb->aio_offset2 = offset2;
> 
> +    assert(bytes <= SIZE_MAX);
>       acb->aio_nbytes = bytes;
>       acb->aio_offset = offset;
> 
> @@ -1848,7 +1849,7 @@ static int paio_submit_co_full(BlockDriverState *bs, int fd,
> 
>   static inline int paio_submit_co(BlockDriverState *bs, int fd,
>                                    int64_t offset, QEMUIOVector *qiov,
> -                                 int bytes, int type)
> +                                 uint64_t bytes, int type)
>   {
>       return paio_submit_co_full(bs, fd, offset, -1, 0, qiov, bytes, type);
>   }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org