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

[Gurchetan Singh]

On Wed, Feb 27, 2019 at 6:07 AM Gerd Hoffmann <kraxel at 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 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
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel