Re: [PATCH] drm: Release reference from blob lookup after replacing property

[Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>]

On Tue, Oct 25, 2016 at 05:27:21PM -0400, Sean Paul wrote:
> On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >
> > Reported-by: Felix Monninger <felix.monninger@xxxxxxxxx>
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=98420
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >  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);

Not sure. I think the orignal code would have been clearer as

blob = NULL;
if (id) {
	blob = drm_property_lookup_blob(dev, id);
	if (!blob)
		return -ENOENT;

	if (blob->length != expected_size)
		return -EINVAL;
}

i.e. the code currently reports if the blob_id doesn't match an existing
blob, and only removes the current blob if passed in 0.

Otherwise it becomes like:

        struct drm_property_blob *blob;
        int ret = -EINVAL;

        blob = drm_property_lookup_blob(crtc->dev, blob_id);
        if (!blob_id || 
            (blob && (expected_size == 0 || expected_size == blob->length))) {
                drm_atomic_replace_property_blob(old_blob, blob, replaced);
                ret = 0;
        }
        drm_property_unreference_blob(blob);

for which I'm actually favouring the existing code for the extra whitespace.

If we insisted on a single return path:

        struct drm_property_blob *new_blob = NULL;
        int ret = -EINVAL;

        if (blob_id != 0) {
                new_blob = drm_property_lookup_blob(crtc->dev, blob_id);
                if (new_blob == NULL)
                        goto out;

                if (expected_size > 0 && expected_size != new_blob->length)
                        goto out;
        }

        drm_atomic_replace_property_blob(blob, new_blob, replaced);
        ret = 0;

out:
        drm_property_unreference_blob(new_blob);
        return ret;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx