Re: [PATCH-for-6.1 v2] softmmu/physmem: fix wrong assertion in qemu_ram_alloc_internal()

[Peter Maydell]

On Tue, 17 Aug 2021 at 08:14, David Hildenbrand <david@redhat.com> wrote:
>
> On 16.08.21 22:52, Peter Xu wrote:
> > On Thu, Aug 05, 2021 at 11:23:50AM +0200, David Hildenbrand wrote:
> >> When adding RAM_NORESERVE, we forgot to remove the old assertion when
> >> adding the updated one, most probably when reworking the patches or
> >> rebasing. We can easily crash QEMU by adding
> >>    -object memory-backend-ram,id=mem0,size=500G,reserve=off
> >> to the QEMU cmdline:
> >>    qemu-system-x86_64: ../softmmu/physmem.c:2146: qemu_ram_alloc_internal:
> >>    Assertion `(ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC))
> >>    == 0' failed.
> >>
> >> Fix it by removing the old assertion.
> >>
> >> Fixes: 8dbe22c6868b ("memory: Introduce RAM_NORESERVE and wire it up in 
> >> qemu_ram_mmap()")
> >> Reviewed-by: Peter Xu <peterx@redhat.com>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>
> >> v1 -> v2:
> >> - Added rbs
> >> - Tagged for 6.1 inclusion
> >>
> >> ---
> >>   softmmu/physmem.c | 1 -
> >>   1 file changed, 1 deletion(-)
> >>
> >> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> >> index 3c1912a1a0..2e18947598 100644
> >> --- a/softmmu/physmem.c
> >> +++ b/softmmu/physmem.c
> >> @@ -2143,7 +2143,6 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, 
> >> ram_addr_t max_size,
> >>       RAMBlock *new_block;
> >>       Error *local_err = NULL;
> >>
> >> -    assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC)) == 
> >> 0);
> >>       assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC |
> >>                             RAM_NORESERVE)) == 0);
> >>       assert(!host ^ (ram_flags & RAM_PREALLOC));
> >> --
> >> 2.31.1
> >>
> >
> > Today I just noticed this patch is still missing for 6.1. How many users are
> > there with reserve=off?  Would it be worth rc4 or not?
> >
>
> Indeed, I forgot to follow up, thanks for bringing this up.
>
> Libvirt does not support virtio-mem yet and consequently doesn't support
> reserve=off yet. (there are use cases without virtio-mem, but I don't
> think anybody is using it yet)
>
> It's an easy way to crash QEMU, but we could also fix in the -stable
> tree instead.

It is a really obvious right fix, though, so I'm going to apply
it for rc4 anyway.

thanks
-- PMM