[virglrenderer-devel] [PATCH] Fix unlinking resources from hash table.

[Gerd Hoffmann]

Virglrenderer sometimes tries to remove resources from the hash table
twice.  Which will mess up the ressource hash table and reference counts
and therefore and leads to qemu/virglrenderer crashes.

Reproducer:
  (a) guest creates resource foo, id 42.
  (b) guest creates an object bar referencing resource foo.
  (c) guest unreferences resource foo.
      -> resource id 42 is removed from hash.
  (d) guest creates a new resource baz, re-using id 42.
  (e) guest destroys object bar.
      -> resource foo refcount goes down to zero.
      -> resource id 42 gets removed from hash the second time,
         but id 42 entry points to resource baz not foo now.
         Oops.

Note that most linux kernel drivers will never ever re-use resource ids
due to a bug in the virtio-gpu kms driver, in which case this bug
doesn't cause any harm.

Root cause is that vrend_renderer_resource_destroy() may call
vrend_resource_remove(), depending on the call chain.  This is wrong.
Only vrend_renderer_resource_unref() which is called when the guest
unreferences a resource should remove the id from the hash table by
calling vrend_resource_remove().

Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
---
 src/vrend_renderer.h |  4 ++--
 src/vrend_renderer.c | 10 ++++------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h
index 51a4c3ab3301..b73665f74421 100644
--- a/src/vrend_renderer.h
+++ b/src/vrend_renderer.h
@@ -356,7 +356,7 @@ int vrend_renderer_resource_attach_iov(int res_handle, struct iovec *iov,
 void vrend_renderer_resource_detach_iov(int res_handle,
                                         struct iovec **iov_p,
                                         int *num_iovs_p);
-void vrend_renderer_resource_destroy(struct vrend_resource *res, bool remove);
+void vrend_renderer_resource_destroy(struct vrend_resource *res);
 
 static inline void
 vrend_resource_reference(struct vrend_resource **ptr, struct vrend_resource *tex)
@@ -364,7 +364,7 @@ vrend_resource_reference(struct vrend_resource **ptr, struct vrend_resource *tex
    struct vrend_resource *old_tex = *ptr;
 
    if (pipe_reference(&(*ptr)->base.reference, &tex->base.reference))
-      vrend_renderer_resource_destroy(old_tex, true);
+      vrend_renderer_resource_destroy(old_tex);
    *ptr = tex;
 }
 
diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
index 5611ab9d0fd7..f8d2834ade0e 100644
--- a/src/vrend_renderer.c
+++ b/src/vrend_renderer.c
@@ -5957,13 +5957,13 @@ int vrend_renderer_resource_create(struct vrend_renderer_resource_create_args *a
 
    ret = vrend_resource_insert(gr, args->handle);
    if (ret == 0) {
-      vrend_renderer_resource_destroy(gr, true);
+      vrend_renderer_resource_destroy(gr);
       return ENOMEM;
    }
    return 0;
 }
 
-void vrend_renderer_resource_destroy(struct vrend_resource *res, bool remove)
+void vrend_renderer_resource_destroy(struct vrend_resource *res)
 {
    if (res->readback_fb_id)
       glDeleteFramebuffers(1, &res->readback_fb_id);
@@ -5979,8 +5979,6 @@ void vrend_renderer_resource_destroy(struct vrend_resource *res, bool remove)
          glDeleteTextures(1, &res->id);
    }
 
-   if (res->handle && remove)
-      vrend_resource_remove(res->handle);
    free(res);
 }
 
@@ -5989,7 +5987,7 @@ static void vrend_destroy_resource_object(void *obj_ptr)
    struct vrend_resource *res = obj_ptr;
 
    if (pipe_reference(&res->base.reference, NULL))
-       vrend_renderer_resource_destroy(res, false);
+       vrend_renderer_resource_destroy(res);
 }
 
 void vrend_renderer_resource_unref(uint32_t res_handle)
@@ -7546,7 +7544,7 @@ static void vrend_renderer_blit_int(struct vrend_context *ctx,
    glBindFramebuffer(GL_FRAMEBUFFER, ctx->sub->fb_id);
 
    if (make_intermediate_copy) {
-      vrend_renderer_resource_destroy(intermediate_copy, false);
+      vrend_renderer_resource_destroy(intermediate_copy);
       glDeleteFramebuffers(1, &intermediate_fbo);
    }
 
-- 
2.9.3