Fix unlinking resources from hash table.

Submitted by Gerd Hoffmann on Feb. 27, 2019, 2:07 p.m.

Details

Message ID 20190227140707.22967-1-kraxel@redhat.com
State New
Headers show
Series "Fix unlinking resources from hash table." ( rev: 1 ) in Virgil 3D

Not browsing as part of any series.

Commit Message

Gerd Hoffmann Feb. 27, 2019, 2:07 p.m.
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@redhat.com>
---
 src/vrend_renderer.h |  4 ++--
 src/vrend_renderer.c | 10 ++++------
 2 files changed, 6 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

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);
    }
 

Comments

On Wed, Feb 27, 2019 at 6:07 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> 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().

Looks good.  I think overall there's a few too many layers of ref
counting.  The virglrenderer project has moved to Gitlab -- can you
submit MR there?

https://gitlab.freedesktop.org/virgl/virglrenderer/merge_requests



>
> Signed-off-by: Gerd Hoffmann <kraxel@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
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel