[Qemu-devel] [PATCH 0/2] vga: assert to "solve" qxl bug

[Qemu-devel] [PATCH 0/2] vga: assert to "solve" qxl bug

[Radim Krčmář]

This is a first step in untangling the vga initialization.

[1/2] turns a runtime bug into a misconfiguration.
[2/2] will catch the same bug.

What would be nice to do, for starters:
 1) propagate recoverable errors (we might want some kind of fallback,
    but never without notifying the highest relevant level.)
 2) emit human-friendly messages
 3) add setters to do (4)-(7) in a sane way
 4) respect invalid user configurations (and abort)
 5) remove duplicate variables
 6) check numeric range of parameters
 7) couple dependent variables by invariants
    (or, better, just don't mess with them everywhere)

(Disclaimer: not on my TODO.)


Radim Krčmář (2):
  vga: abort instead of shrinking memory
  qxl: surface0 and ram_header should fit into vram

 hw/display/qxl.c |  2 ++
 hw/display/vga.c | 10 +++-------
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Radim Krčmář]

Automatic shrinking of vram_size leads to a segfault, because other
variables depend on being smaller and don't get shrinked.

Implications of shrinking would make the code needlessly complicated;
assert instead.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/display/vga.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 9c62fbf48823..a09dd19a6042 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2122,13 +2122,9 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         expand4to8[i] = v;
     }
 
-    /* valid range: 1 MB -> 256 MB */
-    s->vram_size = 1024 * 1024;
-    while (s->vram_size < (s->vram_size_mb << 20) &&
-           s->vram_size < (256 << 20)) {
-        s->vram_size <<= 1;
-    }
-    s->vram_size_mb = s->vram_size >> 20;
+    assert(1 <= s->vram_size_mb && s->vram_size_mb <= 256);
+
+    s->vram_size = s->vram_size_mb << 20;
     if (!s->vbe_size) {
         s->vbe_size = s->vram_size;
     }
-- 
2.3.0

[Qemu-devel] [PATCH 2/2] qxl: surface0 and ram_header should fit into vram

[Radim Krčmář]

The solution is not perfect, but won't let us do the same error again
and has no overhead.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/display/qxl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 61df47726481..d5e85d033080 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -367,6 +367,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
     num_pages         -= surface0_area_size;
     num_pages          = num_pages / QXL_PAGE_SIZE;
 
+    assert(surface0_area_size + ram_header_size <= d->vga.vram_size);
+
     rom->draw_area_offset   = cpu_to_le32(0);
     rom->surface0_area_size = cpu_to_le32(surface0_area_size);
     rom->pages_offset       = cpu_to_le32(surface0_area_size);
-- 
2.3.0

Re: [Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Gerd Hoffmann]

On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote:
> Automatic shrinking of vram_size leads to a segfault, because other
> variables depend on being smaller and don't get shrinked.

--verbose please.  Which other variables?

> Implications of shrinking would make the code needlessly complicated;
> assert instead.

assert isn't an option.  vram_size_mb may come from the user (via vga
device properties), so we need a friendly error message here.

Also the loop you are removing makes sure vram_size is a power of two,
removing that is not correct too.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] qxl: surface0 and ram_header should fit into vram

[Gerd Hoffmann]

On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote:
> The solution is not perfect, but won't let us do the same error again
> and has no overhead.

How do you get qemu into a configuration where this isn't true?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Radim Krčmář]

2015-02-17 09:00+0100, Gerd Hoffmann:
> On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote:
> > Automatic shrinking of vram_size leads to a segfault, because other
> > variables depend on being smaller and don't get shrinked.
> 
> --verbose please.  Which other variables?

I'm sorry, at least rom->surface0_area_size.
(It is sourced from qxl->vgamem_size.)

rom->surface0_area_size shouldn't be bigger than qxl->vga.vram_size,
because it accesses memory allocated by it.

> > Implications of shrinking would make the code needlessly complicated;
> > assert instead.
> 
> assert isn't an option.  vram_size_mb may come from the user (via vga
> device properties), so we need a friendly error message here.

Agreed, it would require a lot of code to do it though (in cover letter)
... what about just dropping [1/2]? :)  ([2/2] does the same job)

> Also the loop you are removing makes sure vram_size is a power of two,
> removing that is not correct too.

(True, the patch should have asserted that as well.)

Re: [Qemu-devel] [PATCH 2/2] qxl: surface0 and ram_header should fit into vram

[Radim Krčmář]

2015-02-17 09:02+0100, Gerd Hoffmann:
> On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote:
> > The solution is not perfect, but won't let us do the same error again
> > and has no overhead.
> 
> How do you get qemu into a configuration where this isn't true?

Without [1/2], by setting qxl-vga.vgamem_mb > 128.
The segfault happens when qxl-vga.vgamem_mb > 256.

In both cases, qxl->vga.vram_size is rounded to 256, but in the latter
one, we allocate less memory than is later accessed.

Re: [Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Gerd Hoffmann]

On Di, 2015-02-17 at 11:29 +0100, Radim Krčmář wrote:
> 2015-02-17 09:00+0100, Gerd Hoffmann:
> > On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote:
> > > Automatic shrinking of vram_size leads to a segfault, because other
> > > variables depend on being smaller and don't get shrinked.
> > 
> > --verbose please.  Which other variables?
> 
> I'm sorry, at least rom->surface0_area_size.
> (It is sourced from qxl->vgamem_size.)

Which command line triggers it?

In theory qxl_init_ramsize() *should* make sure this can't happen ...

I'd like to find & fix the bug instead of plugging an assert into some
random place.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Radim Krčmář]

2015-02-17 11:37+0100, Gerd Hoffmann:
> On Di, 2015-02-17 at 11:29 +0100, Radim Krčmář wrote:
> > 2015-02-17 09:00+0100, Gerd Hoffmann:
> > > On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote:
> > > > Automatic shrinking of vram_size leads to a segfault, because other
> > > > variables depend on being smaller and don't get shrinked.
> > > 
> > > --verbose please.  Which other variables?
> > 
> > I'm sorry, at least rom->surface0_area_size.
> > (It is sourced from qxl->vgamem_size.)
> 
> Which command line triggers it?

The important subset is:
  -vga qxl -global qxl-vga.vgamem_mb=512

The segfault can then be triggered by any operation that dirties the
memory (pause for example).

> In theory qxl_init_ramsize() *should* make sure this can't happen ...
> 
> I'd like to find & fix the bug instead of plugging an assert into some
> random place.

The bug happened because the init code is ovewriting variables, which
made the code unmanageable.  I added an assert, so we would fix the
callers.
Upper layers should also have no idea that our limit is 256, so we would
ideally return an error from vga_common_init() instead of silently
mangling sizes.

Re: [Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

  Hi,

> > Which command line triggers it?
> 
> The important subset is:
>   -vga qxl -global qxl-vga.vgamem_mb=512

Ah, so the problem is only one place enforces a upper limit, so we can
get an invalid configuration with large values.

Can you try the attached patch?

cheers,
  Gerd


[-- Attachment #2: 0001-spice-fix-qxl-mem-size-checking.patch --]
[-- Type: text/x-patch, Size: 1846 bytes --]

>From 7e5e3f9aa6ccd74ebbf454a0e5e4bddf87978f25 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 17 Feb 2015 11:50:49 +0100
Subject: [PATCH] spice: fix qxl mem size checking

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 4 ++++
 hw/display/vga.c | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 61df477..c8ca645 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1880,6 +1880,9 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
     if (qxl->vgamem_size_mb < 8) {
         qxl->vgamem_size_mb = 8;
     }
+    if (qxl->vgamem_size_mb > 512) {
+        qxl->vgamem_size_mb = 512;
+    }
     qxl->vgamem_size = qxl->vgamem_size_mb * 1024 * 1024;
 
     /* vga ram (bar 0, total) */
@@ -2040,6 +2043,7 @@ static int qxl_init_primary(PCIDevice *dev)
     vga->vbe_size = qxl->vgamem_size;
     vga->vram_size_mb = qxl->vga.vram_size >> 20;
     vga_common_init(vga, OBJECT(dev), true);
+    assert(qxl->vgamem_size < qxl->vga.vram_size);
     vga_init(vga, OBJECT(dev),
              pci_address_space(dev), pci_address_space_io(dev), false);
     portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
diff --git a/hw/display/vga.c b/hw/display/vga.c
index ffcfce3..52e86ce 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2122,10 +2122,10 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         expand4to8[i] = v;
     }
 
-    /* valid range: 1 MB -> 256 MB */
+    /* valid range: 1 MB -> 1024 MB */
     s->vram_size = 1024 * 1024;
     while (s->vram_size < (s->vram_size_mb << 20) &&
-           s->vram_size < (256 << 20)) {
+           s->vram_size < (1024 << 20)) {
         s->vram_size <<= 1;
     }
     s->vram_size_mb = s->vram_size >> 20;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] vga: abort instead of shrinking memory

[Radim Krčmář]

2015-02-17 11:51+0100, Gerd Hoffmann:
>   Hi,
> 
> > > Which command line triggers it?
> > 
> > The important subset is:
> >   -vga qxl -global qxl-vga.vgamem_mb=512
> 
> Ah, so the problem is only one place enforces a upper limit, so we can
> get an invalid configuration with large values.

(I think that hardcoding the limit at two unrelated places is bad --
 nothing in the code has improved since the first bug.)

> Can you try the attached patch?

It doesn't crash, but spice doesn't work when setting vgamem that high,
and there is no reason to anyway, so the attached hunk would be better.

Thanks.


---
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 61df47726481..3c55aa6479d4 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1880,6 +1880,9 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
     if (qxl->vgamem_size_mb < 8) {
         qxl->vgamem_size_mb = 8;
     }
+    if (qxl->vgamem_size_mb > 128) {
+        qxl->vgamem_size_mb = 128;
+    }
     qxl->vgamem_size = qxl->vgamem_size_mb * 1024 * 1024;
 
     /* vga ram (bar 0, total) */