| Message ID | e3778e44c98a35839de2f4938e5355449fa3aa14.1706626470.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix resource freeing bugs in virtio-gpu-rutabaga | expand |
On Tue, Jan 30, 2024 at 7:00 AM Manos Pitsidianakis < manos.pitsidianakis@linaro.org> wrote: > When the Rutabaga GPU device frees resources, it calls > rutabaga_resource_unref for that resource_id. However, when the generic > VirtIOGPU functions destroys resources, it only removes the > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. > The rutabaga resource associated with that resource_id is then leaked. > > This commit overrides the resource_destroy class method introduced in > the previous commit to fix this. > Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org> > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > hw/display/virtio-gpu-rutabaga.c | 47 ++++++++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 12 deletions(-) > > diff --git a/hw/display/virtio-gpu-rutabaga.c > b/hw/display/virtio-gpu-rutabaga.c > index 9e67f9bd51..17bf701a21 100644 > --- a/hw/display/virtio-gpu-rutabaga.c > +++ b/hw/display/virtio-gpu-rutabaga.c > @@ -148,14 +148,38 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, > } > > static void > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res, > + Error **errp) > +{ > + int32_t result; > + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > + > + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); > + if (result) { > + error_setg_errno(errp, > + (int)result, > + "%s: rutabaga_resource_unref returned %"PRIi32 > + " for resource_id = %"PRIu32, __func__, result, > + res->resource_id); > + } > + > + if (res->image) { > + pixman_image_unref(res->image); > + } > + > + QTAILQ_REMOVE(&g->reslist, res, next); > + g_free(res); > +} + > +static void > rutabaga_cmd_resource_unref(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > - int32_t result; > + int32_t result = 0; > struct virtio_gpu_simple_resource *res; > struct virtio_gpu_resource_unref unref; > - > - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); > + Error *local_err = NULL; > > VIRTIO_GPU_FILL_CMD(unref); > > @@ -164,15 +188,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, > res = virtio_gpu_find_resource(g, unref.resource_id); > CHECK(res, cmd); > > - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); > - CHECK(!result, cmd); > - > - if (res->image) { > - pixman_image_unref(res->image); > + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); > + if (local_err) { > + error_report_err(local_err); > + /* local_err was freed, do not reuse it. */ > + local_err = NULL; > + result = 1; > } > - > - QTAILQ_REMOVE(&g->reslist, res, next); > - g_free(res); > + CHECK(!result, cmd); > } > > static void > @@ -1099,7 +1122,7 @@ static void > virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; > - > + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; > vdc->realize = virtio_gpu_rutabaga_realize; > device_class_set_props(dc, virtio_gpu_rutabaga_properties); > } > -- > γαῖα πυρί μιχθήτω > >
diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c index 9e67f9bd51..17bf701a21 100644 --- a/hw/display/virtio-gpu-rutabaga.c +++ b/hw/display/virtio-gpu-rutabaga.c @@ -148,14 +148,38 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g, } static void +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g, + struct virtio_gpu_simple_resource *res, + Error **errp) +{ + int32_t result; + VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); + + result = rutabaga_resource_unref(vr->rutabaga, res->resource_id); + if (result) { + error_setg_errno(errp, + (int)result, + "%s: rutabaga_resource_unref returned %"PRIi32 + " for resource_id = %"PRIu32, __func__, result, + res->resource_id); + } + + if (res->image) { + pixman_image_unref(res->image); + } + + QTAILQ_REMOVE(&g->reslist, res, next); + g_free(res); +} + +static void rutabaga_cmd_resource_unref(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { - int32_t result; + int32_t result = 0; struct virtio_gpu_simple_resource *res; struct virtio_gpu_resource_unref unref; - - VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g); + Error *local_err = NULL; VIRTIO_GPU_FILL_CMD(unref); @@ -164,15 +188,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g, res = virtio_gpu_find_resource(g, unref.resource_id); CHECK(res, cmd); - result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id); - CHECK(!result, cmd); - - if (res->image) { - pixman_image_unref(res->image); + virtio_gpu_rutabaga_resource_unref(g, res, &local_err); + if (local_err) { + error_report_err(local_err); + /* local_err was freed, do not reuse it. */ + local_err = NULL; + result = 1; } - - QTAILQ_REMOVE(&g->reslist, res, next); - g_free(res); + CHECK(!result, cmd); } static void @@ -1099,7 +1122,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data) vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl; vgc->process_cmd = virtio_gpu_rutabaga_process_cmd; vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor; - + vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref; vdc->realize = virtio_gpu_rutabaga_realize; device_class_set_props(dc, virtio_gpu_rutabaga_properties); }
When the Rutabaga GPU device frees resources, it calls rutabaga_resource_unref for that resource_id. However, when the generic VirtIOGPU functions destroys resources, it only removes the virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list. The rutabaga resource associated with that resource_id is then leaked. This commit overrides the resource_destroy class method introduced in the previous commit to fix this. Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- hw/display/virtio-gpu-rutabaga.c | 47 ++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 12 deletions(-)