Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Fam Zheng <famz@redhat.com>]

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 <famz@redhat.com>
> > ---
> >  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