[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 02/10] raw: Check byte range uniformly
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v5 02/10] raw: Check byte range uniformly |
Date: |
Thu, 24 May 2018 16:28:25 +0800 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
On Wed, 05/23 13:28, Eric Blake wrote:
> On 05/22/2018 10:04 PM, Fam Zheng wrote:
> > We don't verify the request range against s->size in the I/O callbacks
> > except for raw_co_pwritev. This is wrong (especially for
> > raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.
>
> I'd also mention ...
>
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> > block/raw-format.c | 64
> > +++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 39 insertions(+), 25 deletions(-)
>
> Should this cc: qemu-stable?
>
> Do we have iotests coverage of this?
No for both questions.
This is not reachable from the user interface. In that sense "wrong" is not
accurate in the commit message, "inconsistent" or "confusing" might be more
appropriate.
Out of range requests can only reach us if blk_set_allow_write_beyond_eof(blk,
true) was called, which can only happen when creating image, where the I/O are
all generated internally.
So there is no behavior change in this patch. But verifying and returning error
is still better than asserting, IMO, since we don't want to assert how block
layer uses the driver callbacks. I'll tweak the commit message.
>
> >
> > diff --git a/block/raw-format.c b/block/raw-format.c
> > index fe33693a2d..b69a0674b3 100644
> > --- a/block/raw-format.c
> > +++ b/block/raw-format.c
> > @@ -167,16 +167,37 @@ static void raw_reopen_abort(BDRVReopenState *state)
> > state->opaque = NULL;
> > }
> > +/* Check and adjust the offset, against 'offset' and 'size' options. */
> > +static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
> > + uint64_t bytes, bool is_write)
> > +{
> > + BDRVRawState *s = bs->opaque;
> > +
> > + if (s->has_size && (*offset > s->size || bytes > (s->size - *offset)))
> > {
> > + /* There's not enough space for the write, or the read request is
> > + * out-of-range. Don't read/write anything to prevent leaking out
> > of
> > + * the size specified in options. */
> > + return is_write ? -ENOSPC : -EINVAL;;
> > + }
> > +
> > + if (*offset > INT64_MAX - s->offset) {
> > + return -EINVAL;
>
> ...that this change to a 63-bit check...
>
> > @@ -186,23 +207,11 @@ static int coroutine_fn
> > raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
>
> > - if (offset > UINT64_MAX - s->offset) {
> > - ret = -EINVAL;
> > - goto fail;
> > - }
>
> ...from a previous 64-bit check is intentional.
OK!
>
> With improved commit message, and ideally with followup commits that add
> iotest coverage,
>
> Reviewed-by: Eric Blake <address@hidden>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
Fam
- [Qemu-devel] [PATCH v5 00/10] qemu-img convert with copy offloading, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 01/10] block: Introduce API for copy offloading, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 02/10] raw: Check byte range uniformly, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 03/10] raw: Implement copy offloading, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 04/10] qcow2: Implement copy offloading, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 05/10] file-posix: Implement bdrv_co_copy_range, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 06/10] iscsi: Query and save device designator when opening, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 07/10] iscsi: Create and use iscsi_co_wait_for_task, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 08/10] iscsi: Implement copy offloading, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 09/10] block-backend: Add blk_co_copy_range, Fam Zheng, 2018/05/22
- [Qemu-devel] [PATCH v5 10/10] qemu-img: Convert with copy offloading, Fam Zheng, 2018/05/22