[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 2/3] block: Fix NULL deference for unaligned
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL |
Date: |
Wed, 13 May 2015 13:03:01 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, 05/12 13:52, Kevin Wolf wrote:
> Am 12.05.2015 um 08:09 hat Fam Zheng geschrieben:
> > For zero write, callers pass in NULL qiov (qemu-io "write -z" or
> > scsi-disk "write same").
> >
> > Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case
> > for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler
> > fix would be in bdrv_co_do_pwritev which is the NULL dereference point
> > and covers both cases.
> >
> > So don't access it in bdrv_co_do_pwritev in this case, use three aligned
> > writes.
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> > block/io.c | 95
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> >
> > diff --git a/block/io.c b/block/io.c
> > index 4e5a92e..d766220 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1174,6 +1174,97 @@ static int coroutine_fn
> > bdrv_aligned_pwritev(BlockDriverState *bs,
> > return ret;
> > }
> >
> > +static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
> > + int64_t offset,
> > + unsigned int bytes,
> > + BdrvRequestFlags flags)
> > +{
> > + BdrvTrackedRequest req;
> > + uint8_t *buf = NULL;
> > + QEMUIOVector local_qiov;
> > + struct iovec iov;
> > + uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
> > + unsigned int head_padding_bytes, tail_padding_bytes;
> > + int ret;
> > +
> > + head_padding_bytes = offset & (align - 1);
> > + tail_padding_bytes = align - ((offset + bytes) & (align - 1));
>
> Don't we have macros for these calculations?
No, I don't see any.
> > + tracked_request_begin(&req, bs, offset, bytes, true);
>
> Why duplicate this when it would already be the next line in
> bdrv_co_do_pwritev()?
I'll remove the duplication.
>
> > + mark_request_serialising(&req, align);
> > + wait_serialising_requests(&req);
>
> So this patch serialises all zero writes, even if they are perfectly
> aligned? Why?
>
> Actually, even for misaligned requests, I think the part in the middle
> doesn't require any serialisation, only the RMW parts do.
I'll move to branches.
Thanks,
Fam