[Qemu-devel] [PATCH v1 0/2] memory: fix alignment checks/asserts

[Qemu-devel] [PATCH v1 0/2] memory: fix alignment checks/asserts

[David Hildenbrand]

We can currently hit two asserts. Let's fix those.

Patch nr. 1 is a result from:
    "[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers"

We treat right now any alignment > 1GB as a violation, as it would
fragment guest memory heavily. Turn the assert into a check.

Also, we run into an assert when using alignments that are not a power of
two.

David Hildenbrand (2):
  memory-device: turn alignment assert into check
  exec: check that alignment is a power of two

 exec.c                 | 4 ++++
 hw/mem/memory-device.c | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.17.0

[Qemu-devel] [PATCH v1 1/2] memory-device: turn alignment assert into check

[David Hildenbrand]

The start of the address space indicates which maximum alignment is
supported by our machine (e.g. ppc, x86 1GB). This is helpful to
catch fragmenting guest physical memory in strange fashions.

Right now we can crash QEMU by e.g. (there might be easier examples)

qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 \
 -object memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M \
 -device pc-dimm,id=dimm1,memdev=mem0

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 3e04f3954e..6de4f70bb4 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -116,9 +116,15 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
-    g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
     g_assert(address_space_end >= address_space_start);
 
+    /* address_space_start indicates the maximum alignment we expect */
+    if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
+        error_setg(errp, "the alignment (0%" PRIx64 ") is not supported",
+                   align);
+        return 0;
+    }
+
     memory_device_check_addable(ms, size, errp);
     if (*errp) {
         return 0;
-- 
2.17.0

[Qemu-devel] [PATCH v1 2/2] exec: check that alignment is a power of two

[David Hildenbrand]

Right now we can crash QEMU using e.g.

qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 \
 -object memory-backend-file,id=mem0,size=12288,mem-path=/dev/zero,align=12288 \
 -device pc-dimm,id=dimm1,memdev=mem0

qemu-system-x86_64: util/mmap-alloc.c:115:
 qemu_ram_mmap: Assertion `is_power_of_2(align)' failed

Fix this by adding a proper check.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 exec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/exec.c b/exec.c
index f6645ede0c..f54c83ac61 100644
--- a/exec.c
+++ b/exec.c
@@ -1681,6 +1681,10 @@ static void *file_ram_alloc(RAMBlock *block,
                    " must be multiples of page size 0x%zx",
                    block->mr->align, block->page_size);
         return NULL;
+    } else if (block->mr->align && !is_power_of_2(block->mr->align)) {
+        error_setg(errp, "alignment 0x%" PRIx64
+                   " must be a power of two", block->mr->align);
+        return NULL;
     }
     block->mr->align = MAX(block->page_size, block->mr->align);
 #if defined(__s390x__)
-- 
2.17.0

Re: [Qemu-devel] [PATCH v1 0/2] memory: fix alignment checks/asserts

[Michael S. Tsirkin]

On Thu, Jun 07, 2018 at 05:47:03PM +0200, David Hildenbrand wrote:
> We can currently hit two asserts. Let's fix those.
> 
> Patch nr. 1 is a result from:
>     "[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers"
> 
> We treat right now any alignment > 1GB as a violation, as it would
> fragment guest memory heavily. Turn the assert into a check.
> 
> Also, we run into an assert when using alignments that are not a power of
> two.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> David Hildenbrand (2):
>   memory-device: turn alignment assert into check
>   exec: check that alignment is a power of two
> 
>  exec.c                 | 4 ++++
>  hw/mem/memory-device.c | 8 +++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> -- 
> 2.17.0

Re: [Qemu-devel] [PATCH v1 1/2] memory-device: turn alignment assert into check

[Igor Mammedov]

On Thu,  7 Jun 2018 17:47:04 +0200
David Hildenbrand <david@redhat.com> wrote:

> The start of the address space indicates which maximum alignment is
> supported by our machine (e.g. ppc, x86 1GB). This is helpful to
> catch fragmenting guest physical memory in strange fashions.
> 
> Right now we can crash QEMU by e.g. (there might be easier examples)
> 
> qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 \
>  -object memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M \
>  -device pc-dimm,id=dimm1,memdev=mem0
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/memory-device.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 3e04f3954e..6de4f70bb4 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -116,9 +116,15 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
>      address_space_start = ms->device_memory->base;
>      address_space_end = address_space_start +
>                          memory_region_size(&ms->device_memory->mr);
> -    g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
>      g_assert(address_space_end >= address_space_start);
>  
> +    /* address_space_start indicates the maximum alignment we expect */
> +    if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
> +        error_setg(errp, "the alignment (0%" PRIx64 ") is not supported",
> +                   align);
> +        return 0;
> +    }
> +
>      memory_device_check_addable(ms, size, errp);
>      if (*errp) {
>          return 0;

Re: [Qemu-devel] [PATCH v1 2/2] exec: check that alignment is a power of two

[Igor Mammedov]

On Thu,  7 Jun 2018 17:47:05 +0200
David Hildenbrand <david@redhat.com> wrote:

> Right now we can crash QEMU using e.g.
> 
> qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 \
>  -object memory-backend-file,id=mem0,size=12288,mem-path=/dev/zero,align=12288 \
>  -device pc-dimm,id=dimm1,memdev=mem0
> 
> qemu-system-x86_64: util/mmap-alloc.c:115:
>  qemu_ram_mmap: Assertion `is_power_of_2(align)' failed
> 
> Fix this by adding a proper check.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index f6645ede0c..f54c83ac61 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1681,6 +1681,10 @@ static void *file_ram_alloc(RAMBlock *block,
>                     " must be multiples of page size 0x%zx",
>                     block->mr->align, block->page_size);
>          return NULL;
> +    } else if (block->mr->align && !is_power_of_2(block->mr->align)) {
> +        error_setg(errp, "alignment 0x%" PRIx64
> +                   " must be a power of two", block->mr->align);
> +        return NULL;
>      }
>      block->mr->align = MAX(block->page_size, block->mr->align);
>  #if defined(__s390x__)

Re: [Qemu-devel] [PATCH v1 0/2] memory: fix alignment checks/asserts

[Paolo Bonzini]

On 07/06/2018 17:47, David Hildenbrand wrote:
> We can currently hit two asserts. Let's fix those.
> 
> Patch nr. 1 is a result from:
>     "[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers"
> 
> We treat right now any alignment > 1GB as a violation, as it would
> fragment guest memory heavily. Turn the assert into a check.
> 
> Also, we run into an assert when using alignments that are not a power of
> two.

Queued, thanks.

Paolo