* [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.