All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm: Release reference from blob lookup after replacing property
@ 2016-10-25 19:46 Chris Wilson
  2016-10-25 20:16 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Chris Wilson @ 2016-10-25 19:46 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

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@gmail.com>

Reported-by: Felix Monninger <felix.monninger@gmail.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=98420
Signed-off-by: Chris Wilson <chris@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);
 		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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* ✓ Fi.CI.BAT: success for drm: Release reference from blob lookup after replacing property
  2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson
@ 2016-10-25 20:16 ` Patchwork
  2016-10-25 21:27 ` [Intel-gfx] [PATCH] " Sean Paul
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2016-10-25 20:16 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm: Release reference from blob lookup after replacing property
URL   : https://patchwork.freedesktop.org/series/14357/
State : success

== Summary ==

Series 14357v1 drm: Release reference from blob lookup after replacing property
https://patchwork.freedesktop.org/api/1.0/series/14357/revisions/1/mbox/


fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
fi-ivb-3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-snb-2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 

9e8e859eec7e29c4ef33413560819617a7f9f6bf drm-intel-nightly: 2016y-10m-25d-16h-30m-11s UTC integration manifest
315860c drm: Release reference from blob lookup after replacing property

Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2816/

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2816/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property
  2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson
  2016-10-25 20:16 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2016-10-25 21:27 ` Sean Paul
  2016-10-25 21:45   ` Chris Wilson
  2016-10-25 21:28 ` [PATCH v2] " Chris Wilson
  2016-10-25 21:46 ` ✗ Fi.CI.BAT: warning for drm: Release reference from blob lookup after replacing property (rev2) Patchwork
  3 siblings, 1 reply; 8+ messages in thread
From: Sean Paul @ 2016-10-25 21:27 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel

On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson <chris@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@gmail.com>
>
> Reported-by: Felix Monninger <felix.monninger@gmail.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98420
> Signed-off-by: Chris Wilson <chris@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] drm: Release reference from blob lookup after replacing property
  2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson
  2016-10-25 20:16 ` ✓ Fi.CI.BAT: success for " Patchwork
  2016-10-25 21:27 ` [Intel-gfx] [PATCH] " Sean Paul
@ 2016-10-25 21:28 ` Chris Wilson
  2016-10-26  7:48   ` Ville Syrjälä
  2016-10-25 21:46 ` ✗ Fi.CI.BAT: warning for drm: Release reference from blob lookup after replacing property (rev2) Patchwork
  3 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2016-10-25 21:28 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx, Felix Monninger

From: Felix Monninger <felix.monninger@gmail.com>

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().

v2: update credentials, drm_property_unreference_blob() is NULL safe and
NULL is passed consistently to it throughout drm_atomic.c so do so here.

Reported-by: Felix Monninger <felix.monninger@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98420
Signed-off-by: Felix Monninger <felix.monninger@gmail.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 1b5a32df9a9a..e0760c138355 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -416,18 +416,21 @@ 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);
 		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);
+	drm_property_unreference_blob(new_blob);
 
 	return 0;
 }
-- 
2.10.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property
  2016-10-25 21:27 ` [Intel-gfx] [PATCH] " Sean Paul
@ 2016-10-25 21:45   ` Chris Wilson
  0 siblings, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2016-10-25 21:45 UTC (permalink / raw)
  To: Sean Paul; +Cc: Intel Graphics Development, dri-devel

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@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@gmail.com>
> >
> > Reported-by: Felix Monninger <felix.monninger@gmail.com>
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=98420
> > Signed-off-by: Chris Wilson <chris@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);

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* ✗ Fi.CI.BAT: warning for drm: Release reference from blob lookup after replacing property (rev2)
  2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson
                   ` (2 preceding siblings ...)
  2016-10-25 21:28 ` [PATCH v2] " Chris Wilson
@ 2016-10-25 21:46 ` Patchwork
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2016-10-25 21:46 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm: Release reference from blob lookup after replacing property (rev2)
URL   : https://patchwork.freedesktop.org/series/14357/
State : warning

== Summary ==

Series 14357v2 drm: Release reference from blob lookup after replacing property
https://patchwork.freedesktop.org/api/1.0/series/14357/revisions/2/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-skl-6700hq)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
fi-ivb-3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 

f746a2112fbb563743acc132304075706551d123 drm-intel-nightly: 2016y-10m-25d-20h-02m-34s UTC integration manifest
93b519c drm: Release reference from blob lookup after replacing property

Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2817/

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2817/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] drm: Release reference from blob lookup after replacing property
  2016-10-25 21:28 ` [PATCH v2] " Chris Wilson
@ 2016-10-26  7:48   ` Ville Syrjälä
  2016-10-26  8:29     ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2016-10-26  7:48 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, dri-devel, Felix Monninger

On Tue, Oct 25, 2016 at 10:28:08PM +0100, Chris Wilson wrote:
> From: Felix Monninger <felix.monninger@gmail.com>
> 
> 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().
> 
> v2: update credentials, drm_property_unreference_blob() is NULL safe and
> NULL is passed consistently to it throughout drm_atomic.c so do so here.
> 
> Reported-by: Felix Monninger <felix.monninger@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98420
> Signed-off-by: Felix Monninger <felix.monninger@gmail.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Cc: stable@vger.kernel.org
Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties")
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The single return path bikeshed looked OK to me too, so my r-b can
be applied there as well, if people prefer that version.

> ---
>  drivers/gpu/drm/drm_atomic.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1b5a32df9a9a..e0760c138355 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -416,18 +416,21 @@ 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);
>  		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);
> +	drm_property_unreference_blob(new_blob);
>  
>  	return 0;
>  }
> -- 
> 2.10.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] drm: Release reference from blob lookup after replacing property
  2016-10-26  7:48   ` Ville Syrjälä
@ 2016-10-26  8:29     ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2016-10-26  8:29 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Felix Monninger, dri-devel

On Wed, Oct 26, 2016 at 10:48:07AM +0300, Ville Syrjälä wrote:
> On Tue, Oct 25, 2016 at 10:28:08PM +0100, Chris Wilson wrote:
> > From: Felix Monninger <felix.monninger@gmail.com>
> > 
> > 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().
> > 
> > v2: update credentials, drm_property_unreference_blob() is NULL safe and
> > NULL is passed consistently to it throughout drm_atomic.c so do so here.
> > 
> > Reported-by: Felix Monninger <felix.monninger@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98420
> > Signed-off-by: Felix Monninger <felix.monninger@gmail.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Cc: stable@vger.kernel.org
> Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties")
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The single return path bikeshed looked OK to me too, so my r-b can
> be applied there as well, if people prefer that version.

Applied to drm-misc-fixes (yes I'm trying out something new).
-Daniel

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 1b5a32df9a9a..e0760c138355 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -416,18 +416,21 @@ 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);
> >  		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);
> > +	drm_property_unreference_blob(new_blob);
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.10.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-10-26  8:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson
2016-10-25 20:16 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-10-25 21:27 ` [Intel-gfx] [PATCH] " Sean Paul
2016-10-25 21:45   ` Chris Wilson
2016-10-25 21:28 ` [PATCH v2] " Chris Wilson
2016-10-26  7:48   ` Ville Syrjälä
2016-10-26  8:29     ` Daniel Vetter
2016-10-25 21:46 ` ✗ Fi.CI.BAT: warning for drm: Release reference from blob lookup after replacing property (rev2) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.