Re: [PATCH 3/3] block: fail on open when file size is unaligned to request_alignment

[Vladimir Sementsov-Ogievskiy]

12.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> 11.03.2020 14:06, Max Reitz wrote:
> > On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote:
> > > Prior to the commit the following command lead to crash:
> > >
> > >    ./qemu-io --image-opts -c 'write 0 512' \
> > >    driver=blkdebug,align=4096,image.driver=null-co,image.size=512
> > >
> > > It failes on assertion in bdrv_aligned_pwritev:
> > >    "end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE"
> > >
> > > The problem is obvious: 512 is aligned to 4096 and becomes larger than
> > > file size. And the core bad thing is that file size is unaligned to
> > > request_alignment.
> > >
> > > Let's catch such case on bdrv_open_driver and fail.
> >
> > I think we had a discussion on this before, but I can’t find it right
> > now.  (Although I think that had more to do with something in the
> > file-posix driver, because it wasn’t limited to alignments above 512.)
> >
> > In any case, the file itself is totally valid.  Most importantly, qcow2
> > will regularly create files with unaligned file lengths.
> >
> > So let me create a qcow2 image on a 4k-aligned device:
> >
> > $ truncate 512M fs.img
> > $ sudo losetup -f --show -b 4096 fs.img
> > /dev/loop0
> > $ sudo mkfs.ext4 /dev/loop0
> > [...]
> > $ sudo mount /dev/loop0 /mnt/tmp
> >
> > $ sudo ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 64M
> > Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 size=67108864
> > cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > $ sudo ./qemu-io -t none -c quit /mnt/tmp/foo.qcow2
> > qemu-io: can't open device /mnt/tmp/foo.qcow2: File size is unaligned to
> > request alignment
> >
> > Which is too bad.
>
> What exactly is bad?
>
> Is it correct that create succeeded? Without new error, how would qcow2 driver
> read from unaligned tail of file-posix? It will crash, isn't it?

Hmm, it crashes only on write-part if don't have RESIZE permission.. So for 
qcow2
everything is OK.

And generic read don't care about reading past-EOF because of alignment, read 
is passed
to driver.

> > So the real solution would probably...  Be to align the file size up to
> > the alignment?
>
> On creation, you mean?
>
> > Max
> >
> > > Note, that file size and request_alignment may become out of sync
> > > later, so this commit is not full fix of the problem, but it's better
> > > than nothing.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > ---
> > >   block.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > >
> > > diff --git a/block.c b/block.c
> > > index ecd09dbbfd..4cfc6c33a2 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -1324,6 +1324,13 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> > > BlockDriver *drv,
> > >       assert(bdrv_min_mem_align(bs) != 0);
> > >       assert(is_power_of_2(bs->bl.request_alignment));
> > > +    if (bs->bl.request_alignment > 512 &&
> > > +        !QEMU_IS_ALIGNED(bs->total_sectors, bs->bl.request_alignment / 512))
> > > +    {
> > > +        error_setg(errp, "File size is unaligned to request alignment");
> > > +        return -EINVAL;
> > > +    }
> > > +
> > >       for (i = 0; i < bs->quiesce_counter; i++) {
> > >           if (drv->bdrv_co_drain_begin) {
> > >               drv->bdrv_co_drain_begin(bs);


--
Best regards,
Vladimir