[Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property

[Sean Paul]

On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> drm_property_lookup_blob() returns a reference to the returned blob, and
> drm_atomic_replace_property_blob() takes a references to the blob it
> stores, so afterwards we are left owning a reference to the new_blob that
> we never release, and thus leak memory every time we update a property
> such as during drm_atomic_helper_legacy_gamma_set().
>
> Based on a patch by Felix Monninger <felix.monninger at gmail.com>
>
> Reported-by: Felix Monninger <felix.monninger at gmail.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98420
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1b5a32df9a9a..3b35ab793100 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
>                                          ssize_t expected_size,
>                                          bool *replaced)
>  {
> -       struct drm_device *dev = crtc->dev;
>         struct drm_property_blob *new_blob = NULL;
>
>         if (blob_id != 0) {
> -               new_blob = drm_property_lookup_blob(dev, blob_id);
> +               new_blob = drm_property_lookup_blob(crtc->dev, blob_id);

I think this could be further simplified by making use of
drm_property_lookup_blob() returning NULL for blob_id == 0

Then you could do something like:

static int
drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
                                         struct drm_property_blob **old_blob,
                                         uint64_t blob_id,
                                         ssize_t expected_size,
                                         bool *replaced)
{
        struct drm_property_blob *blob = NULL;
        int ret = 0;

        blob = drm_property_lookup_blob(crtc->dev, blob_id);
        if (blob && expected_size > 0 && expected_size != blob->length) {
                        ret = -EINVAL;
                        goto out;
                }
        }

        drm_atomic_replace_property_blob(blob, blob, replaced);
out:
        drm_property_unreference_blob(blob);
        return 0;
}

>                 if (new_blob == NULL)
>                         return -EINVAL;
> -               if (expected_size > 0 && expected_size != new_blob->length)
> +
> +               if (expected_size > 0 && expected_size != new_blob->length) {
> +                       drm_property_unreference_blob(new_blob);
>                         return -EINVAL;
> +               }
>         }
>
>         drm_atomic_replace_property_blob(blob, new_blob, replaced);
>
> +       if (new_blob)
> +               drm_property_unreference_blob(new_blob);
> +
>         return 0;
>  }
>
> --
> 2.10.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx