Re: [PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot

[Thomas Huth]

On 09/03/2021 15.57, Dr. David Alan Gilbert wrote:
> * Thomas Huth (thuth@redhat.com) wrote:
> > On 09/03/2021 15.05, Keqian Zhu wrote:
> > > On 2021/3/9 21:48, Thomas Huth wrote:
> > > > On 17/12/2020 02.49, Keqian Zhu wrote:
> > > > > The parameters start and size are transfered from QEMU memory
> > > > > emulation layer. It can promise that they are TARGET_PAGE_SIZE
> > > > > aligned. However, KVM needs they are qemu_real_page_size aligned.
> > > > >
> > > > > Though no caller breaks this aligned requirement currently, we'd
> > > > > better add an explicit assert to avoid future breaking.
> > > > >
> > > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > > > ---
> > > > >     accel/kvm/kvm-all.c | 7 +++++++
> > > > >     1 file changed, 7 insertions(+)
> > > > >
> > > > > ---
> > > > > v2
> > > > >     - Address Andrew's commment (Use assert instead of return err).
> > > > >
> > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > > > index f6b16a8df8..73b195cc41 100644
> > > > > --- a/accel/kvm/kvm-all.c
> > > > > +++ b/accel/kvm/kvm-all.c
> > > > > @@ -692,6 +692,10 @@ out:
> > > > >     #define KVM_CLEAR_LOG_ALIGN  (qemu_real_host_page_size << 
> > > > > KVM_CLEAR_LOG_SHIFT)
> > > > >     #define KVM_CLEAR_LOG_MASK   (-KVM_CLEAR_LOG_ALIGN)
> > > > >     +/*
> > > > > + * As the granule of kvm dirty log is qemu_real_host_page_size,
> > > > > + * @start and @size are expected and restricted to align to it.
> > > > > + */
> > > > >     static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
> > > > >                                       uint64_t size)
> > > > >     {
> > > > > @@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, 
> > > > > uint64_t start,
> > > > >         unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> > > > >         int ret;
> > > > >     +    /* Make sure start and size are qemu_real_host_page_size aligned */
> > > > > +    assert(QEMU_IS_ALIGNED(start | size, psize));
> > > >
> > > > Sorry, but that was a bad idea: It triggers and kills my Centos 6 VM:
> > > >
> > > > $ qemu-system-x86_64 -accel kvm -hda ~/virt/images/centos6.qcow2 -m 1G
> > > > qemu-system-x86_64: ../../devel/qemu/accel/kvm/kvm-all.c:690: 
> > > > kvm_log_clear_one_slot: Assertion `QEMU_IS_ALIGNED(start | size, psize)' failed.
> > > > Aborted (core dumped)
> > > Hi Thomas,
> > >
> > > I think this patch is ok, maybe it trigger a potential bug?
> >
> > Well, sure, there is either a bug somewhere else or in this new code. But it's 
> > certainly not normal that the assert() triggers, is it?
> >
> > FWIW, here's a backtrace:
> >
> > #0  0x00007ffff2c1584f in raise () at /lib64/libc.so.6
> > #1  0x00007ffff2bffc45 in abort () at /lib64/libc.so.6
> > #2  0x00007ffff2bffb19 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
> > #3  0x00007ffff2c0de36 in .annobin_assert.c_end () at /lib64/libc.so.6
> > #4  0x0000555555ba25f3 in kvm_log_clear_one_slot
> >      (size=6910080, start=0, as_id=0, mem=0x555556e1ee00)
> >      at ../../devel/qemu/accel/kvm/kvm-all.c:691
> > #5  0x0000555555ba25f3 in kvm_physical_log_clear
> >      (section=0x7fffffffd0b0, section=0x7fffffffd0b0, kml=0x555556dbaac0)
> >      at ../../devel/qemu/accel/kvm/kvm-all.c:843
> > #6  0x0000555555ba25f3 in kvm_log_clear (listener=0x555556dbaac0, 
> > section=0x7fffffffd0b0)
> >      at ../../devel/qemu/accel/kvm/kvm-all.c:1253
> > #7  0x0000555555b023d8 in memory_region_clear_dirty_bitmap
> >      (mr=mr@entry=0x5555573394c0, start=start@entry=0, len=len@entry=6910080)
> >      at ../../devel/qemu/softmmu/memory.c:2132
> > #8  0x0000555555b313d9 in cpu_physical_memory_snapshot_and_clear_dirty
> >      (mr=mr@entry=0x5555573394c0, offset=offset@entry=0, 
> > length=length@entry=6910080, client=client@entry=0) at 
> > ../../devel/qemu/softmmu/physmem.c:1109
> > #9  0x0000555555b02483 in memory_region_snapshot_and_clear_dirty
> >      (mr=mr@entry=0x5555573394c0, addr=addr@entry=0, size=size@entry=6910080, 
> > client=client@entry=0)
> >      at ../../devel/qemu/softmmu/memory.c:2146
>
> Could you please figure out which memory region this is?
> WTH is that size? Is that really the problem that the size is just
> crazy?

The answer is one stack frame below...

> > #10 0x0000555555babe99 in vga_draw_graphic (full_update=0, s=0x5555573394b0)
> >      at ../../devel/qemu/hw/display/vga.c:1661

The vga code basically does this:

    region_start = (s->start_addr * 4);
    region_end = region_start + (ram_addr_t)s->line_offset * height;
    region_end += width * depth / 8; /* scanline length */
    region_end -= s->line_offset;
    ...
    memory_region_snapshot_and_clear_dirty(... region_end - region_start...);

Thus it uses a size that is nowhere guaranteed to be a multiple
of the page size.

A similar usage can be seen in other devices, too (e.g. sm501.c),
so either there is a bug in the assert() statement, or we have
a problem with many devices...

 Thomas