| Message ID | 20210805092350.31195-1-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-6.1,v2] softmmu/physmem: fix wrong assertion in qemu_ram_alloc_internal() | expand |
> 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 Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com> > >
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?
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. (most probably you and me should also be doing PULL requests for "Memory API", we'll have to discuss with Paolo)
On Tue, Aug 17, 2021 at 09:14:32AM +0200, David Hildenbrand wrote: > 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. I hit this when trying with some extreme bitmap tests then I remembered this patch, but that shouldn't really be used by any real customer for sure. Sounds good then, thanks.
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
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));
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(-)