* [PATCH] drm/of: free the iterator object on failure
@ 2021-07-12 15:57 ` Steven Price
0 siblings, 0 replies; 20+ messages in thread
From: Steven Price @ 2021-07-12 15:57 UTC (permalink / raw)
To: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: Steven Price, dri-devel, linux-kernel, Biju Das, Laurent Pinchart
When bailing out due to the sanity check the iterator value needs to be
freed because the early return prevents for_each_child_of_node() from
doing the dereference itself.
Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug")
Signed-off-by: Steven Price <steven.price@arm.com>
---
drivers/gpu/drm/drm_of.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Daniel's email[1] made me take a look at this function and it appears
that for_each_child_of_node()'s interface had caused a bad bug fix due
to the hidden reference counting in the iterator.
[1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 197c57477344..997b8827fed2 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type(
* configurations by passing the endpoints explicitly to
* drm_of_lvds_get_dual_link_pixel_order().
*/
- if (!current_pt || pixels_type != current_pt)
+ if (!current_pt || pixels_type != current_pt) {
+ of_node_put(endpoint);
return -EINVAL;
+ }
}
return pixels_type;
--
2.20.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH] drm/of: free the iterator object on failure @ 2021-07-12 15:57 ` Steven Price 0 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-12 15:57 UTC (permalink / raw) To: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Biju Das, Laurent Pinchart, linux-kernel, dri-devel, Steven Price When bailing out due to the sanity check the iterator value needs to be freed because the early return prevents for_each_child_of_node() from doing the dereference itself. Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") Signed-off-by: Steven Price <steven.price@arm.com> --- drivers/gpu/drm/drm_of.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Daniel's email[1] made me take a look at this function and it appears that for_each_child_of_node()'s interface had caused a bad bug fix due to the hidden reference counting in the iterator. [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 197c57477344..997b8827fed2 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( * configurations by passing the endpoints explicitly to * drm_of_lvds_get_dual_link_pixel_order(). */ - if (!current_pt || pixels_type != current_pt) + if (!current_pt || pixels_type != current_pt) { + of_node_put(endpoint); return -EINVAL; + } } return pixels_type; -- 2.20.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: [PATCH] drm/of: free the iterator object on failure 2021-07-12 15:57 ` Steven Price @ 2021-07-12 16:03 ` Biju Das -1 siblings, 0 replies; 20+ messages in thread From: Biju Das @ 2021-07-12 16:03 UTC (permalink / raw) To: Steven Price, Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: dri-devel, linux-kernel, Laurent Pinchart, Pavel Machek + Pavel > -----Original Message----- > From: Steven Price <steven.price@arm.com> > Sent: 12 July 2021 16:58 > To: Daniel Vetter <daniel@ffwll.ch>; David Airlie <airlied@linux.ie>; > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de> > Cc: Steven Price <steven.price@arm.com>; dri-devel@lists.freedesktop.org; > linux-kernel@vger.kernel.org; Biju Das <biju.das.jz@bp.renesas.com>; > Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Subject: [PATCH] drm/of: free the iterator object on failure > > When bailing out due to the sanity check the iterator value needs to be > freed because the early return prevents for_each_child_of_node() from > doing the dereference itself. > > Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > Signed-off-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/drm_of.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Daniel's email[1] made me take a look at this function and it appears that > for_each_child_of_node()'s interface had caused a bad bug fix due to the > hidden reference counting in the iterator. > > [1] > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker > nel.org%2Fr%2FYOxQ5TbkNrqCGBDJ%2540phenom.ffwll.local&data=04%7C01%7Cb > iju.das.jz%40bp.renesas.com%7Cb0570b66e03a43fb8fa008d9454dddaa%7C53d82571d > a1947e49cb4625a166a4a2a%7C0%7C0%7C637617023006562683%7CUnknown%7CTWFpbGZsb > 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10 > 00&sdata=cQICPYJzmgNkFsNw1wMH9Or73rohnCCGH1DrUg3hvhw%3D&reserved=0 > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index > 197c57477344..997b8827fed2 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > * configurations by passing the endpoints explicitly to > * drm_of_lvds_get_dual_link_pixel_order(). > */ > - if (!current_pt || pixels_type != current_pt) > + if (!current_pt || pixels_type != current_pt) { > + of_node_put(endpoint); > return -EINVAL; > + } > } > > return pixels_type; > -- > 2.20.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH] drm/of: free the iterator object on failure @ 2021-07-12 16:03 ` Biju Das 0 siblings, 0 replies; 20+ messages in thread From: Biju Das @ 2021-07-12 16:03 UTC (permalink / raw) To: Steven Price, Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Pavel Machek, linux-kernel, dri-devel, Laurent Pinchart + Pavel > -----Original Message----- > From: Steven Price <steven.price@arm.com> > Sent: 12 July 2021 16:58 > To: Daniel Vetter <daniel@ffwll.ch>; David Airlie <airlied@linux.ie>; > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de> > Cc: Steven Price <steven.price@arm.com>; dri-devel@lists.freedesktop.org; > linux-kernel@vger.kernel.org; Biju Das <biju.das.jz@bp.renesas.com>; > Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Subject: [PATCH] drm/of: free the iterator object on failure > > When bailing out due to the sanity check the iterator value needs to be > freed because the early return prevents for_each_child_of_node() from > doing the dereference itself. > > Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > Signed-off-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/drm_of.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Daniel's email[1] made me take a look at this function and it appears that > for_each_child_of_node()'s interface had caused a bad bug fix due to the > hidden reference counting in the iterator. > > [1] > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker > nel.org%2Fr%2FYOxQ5TbkNrqCGBDJ%2540phenom.ffwll.local&data=04%7C01%7Cb > iju.das.jz%40bp.renesas.com%7Cb0570b66e03a43fb8fa008d9454dddaa%7C53d82571d > a1947e49cb4625a166a4a2a%7C0%7C0%7C637617023006562683%7CUnknown%7CTWFpbGZsb > 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10 > 00&sdata=cQICPYJzmgNkFsNw1wMH9Or73rohnCCGH1DrUg3hvhw%3D&reserved=0 > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index > 197c57477344..997b8827fed2 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > * configurations by passing the endpoints explicitly to > * drm_of_lvds_get_dual_link_pixel_order(). > */ > - if (!current_pt || pixels_type != current_pt) > + if (!current_pt || pixels_type != current_pt) { > + of_node_put(endpoint); > return -EINVAL; > + } > } > > return pixels_type; > -- > 2.20.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure 2021-07-12 15:57 ` Steven Price @ 2021-07-12 16:50 ` Laurent Pinchart -1 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-12 16:50 UTC (permalink / raw) To: Steven Price Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das Hi Steven, Thank you for the patch. On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: > When bailing out due to the sanity check the iterator value needs to be > freed because the early return prevents for_each_child_of_node() from > doing the dereference itself. > > Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") I don't think the Fixes tag is correct, the issue was already present before 4ee48cc5586b. The fix looks right though. > Signed-off-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/drm_of.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Daniel's email[1] made me take a look at this function and it appears > that for_each_child_of_node()'s interface had caused a bad bug fix due > to the hidden reference counting in the iterator. > > [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 197c57477344..997b8827fed2 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > * configurations by passing the endpoints explicitly to > * drm_of_lvds_get_dual_link_pixel_order(). > */ > - if (!current_pt || pixels_type != current_pt) > + if (!current_pt || pixels_type != current_pt) { > + of_node_put(endpoint); > return -EINVAL; > + } > } > > return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure @ 2021-07-12 16:50 ` Laurent Pinchart 0 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-12 16:50 UTC (permalink / raw) To: Steven Price Cc: David Airlie, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das Hi Steven, Thank you for the patch. On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: > When bailing out due to the sanity check the iterator value needs to be > freed because the early return prevents for_each_child_of_node() from > doing the dereference itself. > > Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") I don't think the Fixes tag is correct, the issue was already present before 4ee48cc5586b. The fix looks right though. > Signed-off-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/drm_of.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Daniel's email[1] made me take a look at this function and it appears > that for_each_child_of_node()'s interface had caused a bad bug fix due > to the hidden reference counting in the iterator. > > [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 197c57477344..997b8827fed2 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > * configurations by passing the endpoints explicitly to > * drm_of_lvds_get_dual_link_pixel_order(). > */ > - if (!current_pt || pixels_type != current_pt) > + if (!current_pt || pixels_type != current_pt) { > + of_node_put(endpoint); > return -EINVAL; > + } > } > > return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure 2021-07-12 16:50 ` Laurent Pinchart @ 2021-07-12 21:31 ` Steven Price -1 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-12 21:31 UTC (permalink / raw) To: Laurent Pinchart Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das On 12/07/2021 17:50, Laurent Pinchart wrote: > Hi Steven, > > Thank you for the patch. > > On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: >> When bailing out due to the sanity check the iterator value needs to be >> freed because the early return prevents for_each_child_of_node() from >> doing the dereference itself. >> >> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > > I don't think the Fixes tag is correct, the issue was already present > before 4ee48cc5586b. The fix looks right though. I'm not sure quite what you mean by "already present". As I understand it the timeline was: 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order The function was originally added. This made the mistake twice of calling of_node_put() on the wrong variable (remote_port rather than endpoint). 2. 4ee48cc5586b drm: of: Fix double-free bug One of the of_node_put() calls was removed as it was a double-free. This left the first incorrect of_node_put() in place, and the second is now a straight leak. 3. b557a5f8da57 drm/of: free the right object This (correctly) fixes the first of_node_put() to free endpoint. And the post from Daniel was what caused me to look. 4. This patch Reintroduces the of_node_put() removed in (2) but putting endpoint rather than remote_port. I've put (2) in the Fixes line as this patch is fixing the leak introduced by that patch, but that in itself was of course 'fixing' the double free of the original patch. Steve >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> drivers/gpu/drm/drm_of.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> Daniel's email[1] made me take a look at this function and it appears >> that for_each_child_of_node()'s interface had caused a bad bug fix due >> to the hidden reference counting in the iterator. >> >> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local >> >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >> index 197c57477344..997b8827fed2 100644 >> --- a/drivers/gpu/drm/drm_of.c >> +++ b/drivers/gpu/drm/drm_of.c >> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( >> * configurations by passing the endpoints explicitly to >> * drm_of_lvds_get_dual_link_pixel_order(). >> */ >> - if (!current_pt || pixels_type != current_pt) >> + if (!current_pt || pixels_type != current_pt) { >> + of_node_put(endpoint); >> return -EINVAL; >> + } >> } >> >> return pixels_type; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure @ 2021-07-12 21:31 ` Steven Price 0 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-12 21:31 UTC (permalink / raw) To: Laurent Pinchart Cc: David Airlie, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das On 12/07/2021 17:50, Laurent Pinchart wrote: > Hi Steven, > > Thank you for the patch. > > On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: >> When bailing out due to the sanity check the iterator value needs to be >> freed because the early return prevents for_each_child_of_node() from >> doing the dereference itself. >> >> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > > I don't think the Fixes tag is correct, the issue was already present > before 4ee48cc5586b. The fix looks right though. I'm not sure quite what you mean by "already present". As I understand it the timeline was: 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order The function was originally added. This made the mistake twice of calling of_node_put() on the wrong variable (remote_port rather than endpoint). 2. 4ee48cc5586b drm: of: Fix double-free bug One of the of_node_put() calls was removed as it was a double-free. This left the first incorrect of_node_put() in place, and the second is now a straight leak. 3. b557a5f8da57 drm/of: free the right object This (correctly) fixes the first of_node_put() to free endpoint. And the post from Daniel was what caused me to look. 4. This patch Reintroduces the of_node_put() removed in (2) but putting endpoint rather than remote_port. I've put (2) in the Fixes line as this patch is fixing the leak introduced by that patch, but that in itself was of course 'fixing' the double free of the original patch. Steve >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> drivers/gpu/drm/drm_of.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> Daniel's email[1] made me take a look at this function and it appears >> that for_each_child_of_node()'s interface had caused a bad bug fix due >> to the hidden reference counting in the iterator. >> >> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local >> >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >> index 197c57477344..997b8827fed2 100644 >> --- a/drivers/gpu/drm/drm_of.c >> +++ b/drivers/gpu/drm/drm_of.c >> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( >> * configurations by passing the endpoints explicitly to >> * drm_of_lvds_get_dual_link_pixel_order(). >> */ >> - if (!current_pt || pixels_type != current_pt) >> + if (!current_pt || pixels_type != current_pt) { >> + of_node_put(endpoint); >> return -EINVAL; >> + } >> } >> >> return pixels_type; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure 2021-07-12 21:31 ` Steven Price @ 2021-07-12 21:55 ` Laurent Pinchart -1 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-12 21:55 UTC (permalink / raw) To: Steven Price Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das Hi Steven, On Mon, Jul 12, 2021 at 10:31:52PM +0100, Steven Price wrote: > On 12/07/2021 17:50, Laurent Pinchart wrote: > > On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: > >> When bailing out due to the sanity check the iterator value needs to be > >> freed because the early return prevents for_each_child_of_node() from > >> doing the dereference itself. > >> > >> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > > > > I don't think the Fixes tag is correct, the issue was already present > > before 4ee48cc5586b. The fix looks right though. > > I'm not sure quite what you mean by "already present". As I understand > it the timeline was: > > 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order > The function was originally added. This made the mistake twice of > calling of_node_put() on the wrong variable (remote_port rather than > endpoint). Correct. > 2. 4ee48cc5586b drm: of: Fix double-free bug > One of the of_node_put() calls was removed as it was a double-free. > This left the first incorrect of_node_put() in place, and the second > is now a straight leak. That's right, but this commit didn't introduce the leak, it was already there in 6529007522de (in addition to the double-free). > 3. b557a5f8da57 drm/of: free the right object > This (correctly) fixes the first of_node_put() to free endpoint. And > the post from Daniel was what caused me to look. > > 4. This patch > Reintroduces the of_node_put() removed in (2) but putting endpoint > rather than remote_port. > > I've put (2) in the Fixes line as this patch is fixing the leak > introduced by that patch, but that in itself was of course 'fixing' the > double free of the original patch. > > >> Signed-off-by: Steven Price <steven.price@arm.com> > >> --- > >> drivers/gpu/drm/drm_of.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> Daniel's email[1] made me take a look at this function and it appears > >> that for_each_child_of_node()'s interface had caused a bad bug fix due > >> to the hidden reference counting in the iterator. > >> > >> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local > >> > >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > >> index 197c57477344..997b8827fed2 100644 > >> --- a/drivers/gpu/drm/drm_of.c > >> +++ b/drivers/gpu/drm/drm_of.c > >> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > >> * configurations by passing the endpoints explicitly to > >> * drm_of_lvds_get_dual_link_pixel_order(). > >> */ > >> - if (!current_pt || pixels_type != current_pt) > >> + if (!current_pt || pixels_type != current_pt) { > >> + of_node_put(endpoint); > >> return -EINVAL; > >> + } > >> } > >> > >> return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure @ 2021-07-12 21:55 ` Laurent Pinchart 0 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-12 21:55 UTC (permalink / raw) To: Steven Price Cc: David Airlie, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das Hi Steven, On Mon, Jul 12, 2021 at 10:31:52PM +0100, Steven Price wrote: > On 12/07/2021 17:50, Laurent Pinchart wrote: > > On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: > >> When bailing out due to the sanity check the iterator value needs to be > >> freed because the early return prevents for_each_child_of_node() from > >> doing the dereference itself. > >> > >> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > > > > I don't think the Fixes tag is correct, the issue was already present > > before 4ee48cc5586b. The fix looks right though. > > I'm not sure quite what you mean by "already present". As I understand > it the timeline was: > > 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order > The function was originally added. This made the mistake twice of > calling of_node_put() on the wrong variable (remote_port rather than > endpoint). Correct. > 2. 4ee48cc5586b drm: of: Fix double-free bug > One of the of_node_put() calls was removed as it was a double-free. > This left the first incorrect of_node_put() in place, and the second > is now a straight leak. That's right, but this commit didn't introduce the leak, it was already there in 6529007522de (in addition to the double-free). > 3. b557a5f8da57 drm/of: free the right object > This (correctly) fixes the first of_node_put() to free endpoint. And > the post from Daniel was what caused me to look. > > 4. This patch > Reintroduces the of_node_put() removed in (2) but putting endpoint > rather than remote_port. > > I've put (2) in the Fixes line as this patch is fixing the leak > introduced by that patch, but that in itself was of course 'fixing' the > double free of the original patch. > > >> Signed-off-by: Steven Price <steven.price@arm.com> > >> --- > >> drivers/gpu/drm/drm_of.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> Daniel's email[1] made me take a look at this function and it appears > >> that for_each_child_of_node()'s interface had caused a bad bug fix due > >> to the hidden reference counting in the iterator. > >> > >> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local > >> > >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > >> index 197c57477344..997b8827fed2 100644 > >> --- a/drivers/gpu/drm/drm_of.c > >> +++ b/drivers/gpu/drm/drm_of.c > >> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > >> * configurations by passing the endpoints explicitly to > >> * drm_of_lvds_get_dual_link_pixel_order(). > >> */ > >> - if (!current_pt || pixels_type != current_pt) > >> + if (!current_pt || pixels_type != current_pt) { > >> + of_node_put(endpoint); > >> return -EINVAL; > >> + } > >> } > >> > >> return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure 2021-07-12 21:55 ` Laurent Pinchart @ 2021-07-13 16:16 ` Steven Price -1 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-13 16:16 UTC (permalink / raw) To: Laurent Pinchart Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das On 12/07/2021 22:55, Laurent Pinchart wrote: > Hi Steven, Hi Laurent, > On Mon, Jul 12, 2021 at 10:31:52PM +0100, Steven Price wrote: >> On 12/07/2021 17:50, Laurent Pinchart wrote: >>> On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: >>>> When bailing out due to the sanity check the iterator value needs to be >>>> freed because the early return prevents for_each_child_of_node() from >>>> doing the dereference itself. >>>> >>>> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") >>> >>> I don't think the Fixes tag is correct, the issue was already present >>> before 4ee48cc5586b. The fix looks right though. >> >> I'm not sure quite what you mean by "already present". As I understand >> it the timeline was: >> >> 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order >> The function was originally added. This made the mistake twice of >> calling of_node_put() on the wrong variable (remote_port rather than >> endpoint). > > Correct. > >> 2. 4ee48cc5586b drm: of: Fix double-free bug >> One of the of_node_put() calls was removed as it was a double-free. >> This left the first incorrect of_node_put() in place, and the second >> is now a straight leak. > > That's right, but this commit didn't introduce the leak, it was already > there in 6529007522de (in addition to the double-free). Ah, I see what you mean. My thought process was that the original comment had the bug "using the wrong variable", and (2) (partially) fixed that but in the process introduced a new bug (a memory leak). But I guess technically the memory leak was there from the beginning. The other reason I referenced (2) in the Fixes line is because this patch depends on patch (2), whereas it won't apply cleanly without. However I don't think it really matters either way: (2) has already been backported, and either way this needs fixing if either (1) or (2) are present. Would you like me to resend with a "Fixes: 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order", or are you happy to just fix this up when merging? Thanks, Steve >> 3. b557a5f8da57 drm/of: free the right object >> This (correctly) fixes the first of_node_put() to free endpoint. And >> the post from Daniel was what caused me to look. >> >> 4. This patch >> Reintroduces the of_node_put() removed in (2) but putting endpoint >> rather than remote_port. >> >> I've put (2) in the Fixes line as this patch is fixing the leak >> introduced by that patch, but that in itself was of course 'fixing' the >> double free of the original patch. >> >>>> Signed-off-by: Steven Price <steven.price@arm.com> >>>> --- >>>> drivers/gpu/drm/drm_of.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> Daniel's email[1] made me take a look at this function and it appears >>>> that for_each_child_of_node()'s interface had caused a bad bug fix due >>>> to the hidden reference counting in the iterator. >>>> >>>> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local >>>> >>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >>>> index 197c57477344..997b8827fed2 100644 >>>> --- a/drivers/gpu/drm/drm_of.c >>>> +++ b/drivers/gpu/drm/drm_of.c >>>> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( >>>> * configurations by passing the endpoints explicitly to >>>> * drm_of_lvds_get_dual_link_pixel_order(). >>>> */ >>>> - if (!current_pt || pixels_type != current_pt) >>>> + if (!current_pt || pixels_type != current_pt) { >>>> + of_node_put(endpoint); >>>> return -EINVAL; >>>> + } >>>> } >>>> >>>> return pixels_type; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure @ 2021-07-13 16:16 ` Steven Price 0 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-13 16:16 UTC (permalink / raw) To: Laurent Pinchart Cc: David Airlie, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das On 12/07/2021 22:55, Laurent Pinchart wrote: > Hi Steven, Hi Laurent, > On Mon, Jul 12, 2021 at 10:31:52PM +0100, Steven Price wrote: >> On 12/07/2021 17:50, Laurent Pinchart wrote: >>> On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: >>>> When bailing out due to the sanity check the iterator value needs to be >>>> freed because the early return prevents for_each_child_of_node() from >>>> doing the dereference itself. >>>> >>>> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") >>> >>> I don't think the Fixes tag is correct, the issue was already present >>> before 4ee48cc5586b. The fix looks right though. >> >> I'm not sure quite what you mean by "already present". As I understand >> it the timeline was: >> >> 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order >> The function was originally added. This made the mistake twice of >> calling of_node_put() on the wrong variable (remote_port rather than >> endpoint). > > Correct. > >> 2. 4ee48cc5586b drm: of: Fix double-free bug >> One of the of_node_put() calls was removed as it was a double-free. >> This left the first incorrect of_node_put() in place, and the second >> is now a straight leak. > > That's right, but this commit didn't introduce the leak, it was already > there in 6529007522de (in addition to the double-free). Ah, I see what you mean. My thought process was that the original comment had the bug "using the wrong variable", and (2) (partially) fixed that but in the process introduced a new bug (a memory leak). But I guess technically the memory leak was there from the beginning. The other reason I referenced (2) in the Fixes line is because this patch depends on patch (2), whereas it won't apply cleanly without. However I don't think it really matters either way: (2) has already been backported, and either way this needs fixing if either (1) or (2) are present. Would you like me to resend with a "Fixes: 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order", or are you happy to just fix this up when merging? Thanks, Steve >> 3. b557a5f8da57 drm/of: free the right object >> This (correctly) fixes the first of_node_put() to free endpoint. And >> the post from Daniel was what caused me to look. >> >> 4. This patch >> Reintroduces the of_node_put() removed in (2) but putting endpoint >> rather than remote_port. >> >> I've put (2) in the Fixes line as this patch is fixing the leak >> introduced by that patch, but that in itself was of course 'fixing' the >> double free of the original patch. >> >>>> Signed-off-by: Steven Price <steven.price@arm.com> >>>> --- >>>> drivers/gpu/drm/drm_of.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> Daniel's email[1] made me take a look at this function and it appears >>>> that for_each_child_of_node()'s interface had caused a bad bug fix due >>>> to the hidden reference counting in the iterator. >>>> >>>> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local >>>> >>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >>>> index 197c57477344..997b8827fed2 100644 >>>> --- a/drivers/gpu/drm/drm_of.c >>>> +++ b/drivers/gpu/drm/drm_of.c >>>> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( >>>> * configurations by passing the endpoints explicitly to >>>> * drm_of_lvds_get_dual_link_pixel_order(). >>>> */ >>>> - if (!current_pt || pixels_type != current_pt) >>>> + if (!current_pt || pixels_type != current_pt) { >>>> + of_node_put(endpoint); >>>> return -EINVAL; >>>> + } >>>> } >>>> >>>> return pixels_type; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure 2021-07-13 16:16 ` Steven Price @ 2021-07-14 8:35 ` Laurent Pinchart -1 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-14 8:35 UTC (permalink / raw) To: Steven Price Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das Hi Steven, On Tue, Jul 13, 2021 at 05:16:16PM +0100, Steven Price wrote: > On 12/07/2021 22:55, Laurent Pinchart wrote: > > On Mon, Jul 12, 2021 at 10:31:52PM +0100, Steven Price wrote: > >> On 12/07/2021 17:50, Laurent Pinchart wrote: > >>> On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: > >>>> When bailing out due to the sanity check the iterator value needs to be > >>>> freed because the early return prevents for_each_child_of_node() from > >>>> doing the dereference itself. > >>>> > >>>> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > >>> > >>> I don't think the Fixes tag is correct, the issue was already present > >>> before 4ee48cc5586b. The fix looks right though. > >> > >> I'm not sure quite what you mean by "already present". As I understand > >> it the timeline was: > >> > >> 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order > >> The function was originally added. This made the mistake twice of > >> calling of_node_put() on the wrong variable (remote_port rather than > >> endpoint). > > > > Correct. > > > >> 2. 4ee48cc5586b drm: of: Fix double-free bug > >> One of the of_node_put() calls was removed as it was a double-free. > >> This left the first incorrect of_node_put() in place, and the second > >> is now a straight leak. > > > > That's right, but this commit didn't introduce the leak, it was already > > there in 6529007522de (in addition to the double-free). > > Ah, I see what you mean. My thought process was that the original > comment had the bug "using the wrong variable", and (2) (partially) > fixed that but in the process introduced a new bug (a memory leak). But > I guess technically the memory leak was there from the beginning. > > The other reason I referenced (2) in the Fixes line is because this > patch depends on patch (2), whereas it won't apply cleanly without. > > However I don't think it really matters either way: (2) has already been > backported, and either way this needs fixing if either (1) or (2) are > present. > > Would you like me to resend with a "Fixes: 6529007522de drm: of: Add > drm_of_lvds_get_dual_link_pixel_order", or are you happy to just fix > this up when merging? I don't mind either way, from my point of view it can be fixed up by whoever will pick the patch up and merge it. > >> 3. b557a5f8da57 drm/of: free the right object > >> This (correctly) fixes the first of_node_put() to free endpoint. And > >> the post from Daniel was what caused me to look. > >> > >> 4. This patch > >> Reintroduces the of_node_put() removed in (2) but putting endpoint > >> rather than remote_port. > >> > >> I've put (2) in the Fixes line as this patch is fixing the leak > >> introduced by that patch, but that in itself was of course 'fixing' the > >> double free of the original patch. > >> > >>>> Signed-off-by: Steven Price <steven.price@arm.com> > >>>> --- > >>>> drivers/gpu/drm/drm_of.c | 4 +++- > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> Daniel's email[1] made me take a look at this function and it appears > >>>> that for_each_child_of_node()'s interface had caused a bad bug fix due > >>>> to the hidden reference counting in the iterator. > >>>> > >>>> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > >>>> index 197c57477344..997b8827fed2 100644 > >>>> --- a/drivers/gpu/drm/drm_of.c > >>>> +++ b/drivers/gpu/drm/drm_of.c > >>>> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > >>>> * configurations by passing the endpoints explicitly to > >>>> * drm_of_lvds_get_dual_link_pixel_order(). > >>>> */ > >>>> - if (!current_pt || pixels_type != current_pt) > >>>> + if (!current_pt || pixels_type != current_pt) { > >>>> + of_node_put(endpoint); > >>>> return -EINVAL; > >>>> + } > >>>> } > >>>> > >>>> return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/of: free the iterator object on failure @ 2021-07-14 8:35 ` Laurent Pinchart 0 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-14 8:35 UTC (permalink / raw) To: Steven Price Cc: David Airlie, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das Hi Steven, On Tue, Jul 13, 2021 at 05:16:16PM +0100, Steven Price wrote: > On 12/07/2021 22:55, Laurent Pinchart wrote: > > On Mon, Jul 12, 2021 at 10:31:52PM +0100, Steven Price wrote: > >> On 12/07/2021 17:50, Laurent Pinchart wrote: > >>> On Mon, Jul 12, 2021 at 04:57:58PM +0100, Steven Price wrote: > >>>> When bailing out due to the sanity check the iterator value needs to be > >>>> freed because the early return prevents for_each_child_of_node() from > >>>> doing the dereference itself. > >>>> > >>>> Fixes: 4ee48cc5586b ("drm: of: Fix double-free bug") > >>> > >>> I don't think the Fixes tag is correct, the issue was already present > >>> before 4ee48cc5586b. The fix looks right though. > >> > >> I'm not sure quite what you mean by "already present". As I understand > >> it the timeline was: > >> > >> 1. 6529007522de drm: of: Add drm_of_lvds_get_dual_link_pixel_order > >> The function was originally added. This made the mistake twice of > >> calling of_node_put() on the wrong variable (remote_port rather than > >> endpoint). > > > > Correct. > > > >> 2. 4ee48cc5586b drm: of: Fix double-free bug > >> One of the of_node_put() calls was removed as it was a double-free. > >> This left the first incorrect of_node_put() in place, and the second > >> is now a straight leak. > > > > That's right, but this commit didn't introduce the leak, it was already > > there in 6529007522de (in addition to the double-free). > > Ah, I see what you mean. My thought process was that the original > comment had the bug "using the wrong variable", and (2) (partially) > fixed that but in the process introduced a new bug (a memory leak). But > I guess technically the memory leak was there from the beginning. > > The other reason I referenced (2) in the Fixes line is because this > patch depends on patch (2), whereas it won't apply cleanly without. > > However I don't think it really matters either way: (2) has already been > backported, and either way this needs fixing if either (1) or (2) are > present. > > Would you like me to resend with a "Fixes: 6529007522de drm: of: Add > drm_of_lvds_get_dual_link_pixel_order", or are you happy to just fix > this up when merging? I don't mind either way, from my point of view it can be fixed up by whoever will pick the patch up and merge it. > >> 3. b557a5f8da57 drm/of: free the right object > >> This (correctly) fixes the first of_node_put() to free endpoint. And > >> the post from Daniel was what caused me to look. > >> > >> 4. This patch > >> Reintroduces the of_node_put() removed in (2) but putting endpoint > >> rather than remote_port. > >> > >> I've put (2) in the Fixes line as this patch is fixing the leak > >> introduced by that patch, but that in itself was of course 'fixing' the > >> double free of the original patch. > >> > >>>> Signed-off-by: Steven Price <steven.price@arm.com> > >>>> --- > >>>> drivers/gpu/drm/drm_of.c | 4 +++- > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> Daniel's email[1] made me take a look at this function and it appears > >>>> that for_each_child_of_node()'s interface had caused a bad bug fix due > >>>> to the hidden reference counting in the iterator. > >>>> > >>>> [1] https://lore.kernel.org/r/YOxQ5TbkNrqCGBDJ%40phenom.ffwll.local > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > >>>> index 197c57477344..997b8827fed2 100644 > >>>> --- a/drivers/gpu/drm/drm_of.c > >>>> +++ b/drivers/gpu/drm/drm_of.c > >>>> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > >>>> * configurations by passing the endpoints explicitly to > >>>> * drm_of_lvds_get_dual_link_pixel_order(). > >>>> */ > >>>> - if (!current_pt || pixels_type != current_pt) > >>>> + if (!current_pt || pixels_type != current_pt) { > >>>> + of_node_put(endpoint); > >>>> return -EINVAL; > >>>> + } > >>>> } > >>>> > >>>> return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] drm/of: free the iterator object on failure 2021-07-12 15:57 ` Steven Price @ 2021-07-14 14:33 ` Steven Price -1 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-14 14:33 UTC (permalink / raw) To: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Steven Price, dri-devel, linux-kernel, Biju Das, Laurent Pinchart, Pavel Machek When bailing out due to the sanity check the iterator value needs to be freed because the early return prevents for_each_child_of_node() from doing the dereference itself. Fixes: 6529007522de ("drm: of: Add drm_of_lvds_get_dual_link_pixel_order") Signed-off-by: Steven Price <steven.price@arm.com> --- drivers/gpu/drm/drm_of.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) v2: Fixes now refers to the original commit as suggested by Laurent, rather than 4ee48cc5586b ("drm: of: Fix double-free bug") which only fixed part of the problem. Note that 4ee48cc5586b is a dependency for this patch to cleanly apply. diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 197c57477344..997b8827fed2 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( * configurations by passing the endpoints explicitly to * drm_of_lvds_get_dual_link_pixel_order(). */ - if (!current_pt || pixels_type != current_pt) + if (!current_pt || pixels_type != current_pt) { + of_node_put(endpoint); return -EINVAL; + } } return pixels_type; -- 2.20.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2] drm/of: free the iterator object on failure @ 2021-07-14 14:33 ` Steven Price 0 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-14 14:33 UTC (permalink / raw) To: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Pavel Machek, linux-kernel, dri-devel, Steven Price, Laurent Pinchart, Biju Das When bailing out due to the sanity check the iterator value needs to be freed because the early return prevents for_each_child_of_node() from doing the dereference itself. Fixes: 6529007522de ("drm: of: Add drm_of_lvds_get_dual_link_pixel_order") Signed-off-by: Steven Price <steven.price@arm.com> --- drivers/gpu/drm/drm_of.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) v2: Fixes now refers to the original commit as suggested by Laurent, rather than 4ee48cc5586b ("drm: of: Fix double-free bug") which only fixed part of the problem. Note that 4ee48cc5586b is a dependency for this patch to cleanly apply. diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 197c57477344..997b8827fed2 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( * configurations by passing the endpoints explicitly to * drm_of_lvds_get_dual_link_pixel_order(). */ - if (!current_pt || pixels_type != current_pt) + if (!current_pt || pixels_type != current_pt) { + of_node_put(endpoint); return -EINVAL; + } } return pixels_type; -- 2.20.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/of: free the iterator object on failure 2021-07-14 14:33 ` Steven Price @ 2021-07-14 15:26 ` Laurent Pinchart -1 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-14 15:26 UTC (permalink / raw) To: Steven Price Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das, Pavel Machek Hi Steven, Thank you for the patch. On Wed, Jul 14, 2021 at 03:33:00PM +0100, Steven Price wrote: > When bailing out due to the sanity check the iterator value needs to be > freed because the early return prevents for_each_child_of_node() from > doing the dereference itself. > > Fixes: 6529007522de ("drm: of: Add drm_of_lvds_get_dual_link_pixel_order") > Signed-off-by: Steven Price <steven.price@arm.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/drm_of.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > v2: Fixes now refers to the original commit as suggested by Laurent, rather > than 4ee48cc5586b ("drm: of: Fix double-free bug") which only fixed part of > the problem. Note that 4ee48cc5586b is a dependency for this patch to > cleanly apply. > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 197c57477344..997b8827fed2 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > * configurations by passing the endpoints explicitly to > * drm_of_lvds_get_dual_link_pixel_order(). > */ > - if (!current_pt || pixels_type != current_pt) > + if (!current_pt || pixels_type != current_pt) { > + of_node_put(endpoint); > return -EINVAL; > + } > } > > return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/of: free the iterator object on failure @ 2021-07-14 15:26 ` Laurent Pinchart 0 siblings, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-14 15:26 UTC (permalink / raw) To: Steven Price Cc: David Airlie, Pavel Machek, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das Hi Steven, Thank you for the patch. On Wed, Jul 14, 2021 at 03:33:00PM +0100, Steven Price wrote: > When bailing out due to the sanity check the iterator value needs to be > freed because the early return prevents for_each_child_of_node() from > doing the dereference itself. > > Fixes: 6529007522de ("drm: of: Add drm_of_lvds_get_dual_link_pixel_order") > Signed-off-by: Steven Price <steven.price@arm.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/drm_of.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > v2: Fixes now refers to the original commit as suggested by Laurent, rather > than 4ee48cc5586b ("drm: of: Fix double-free bug") which only fixed part of > the problem. Note that 4ee48cc5586b is a dependency for this patch to > cleanly apply. > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 197c57477344..997b8827fed2 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( > * configurations by passing the endpoints explicitly to > * drm_of_lvds_get_dual_link_pixel_order(). > */ > - if (!current_pt || pixels_type != current_pt) > + if (!current_pt || pixels_type != current_pt) { > + of_node_put(endpoint); > return -EINVAL; > + } > } > > return pixels_type; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/of: free the iterator object on failure 2021-07-14 15:26 ` Laurent Pinchart @ 2021-07-15 10:32 ` Steven Price -1 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-15 10:32 UTC (permalink / raw) To: Laurent Pinchart Cc: Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, dri-devel, linux-kernel, Biju Das, Pavel Machek On 14/07/2021 16:26, Laurent Pinchart wrote: > Hi Steven, > > Thank you for the patch. > > On Wed, Jul 14, 2021 at 03:33:00PM +0100, Steven Price wrote: >> When bailing out due to the sanity check the iterator value needs to be >> freed because the early return prevents for_each_child_of_node() from >> doing the dereference itself. >> >> Fixes: 6529007522de ("drm: of: Add drm_of_lvds_get_dual_link_pixel_order") >> Signed-off-by: Steven Price <steven.price@arm.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! Applied to drm-misc-next. Steve >> --- >> drivers/gpu/drm/drm_of.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> v2: Fixes now refers to the original commit as suggested by Laurent, rather >> than 4ee48cc5586b ("drm: of: Fix double-free bug") which only fixed part of >> the problem. Note that 4ee48cc5586b is a dependency for this patch to >> cleanly apply. >> >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >> index 197c57477344..997b8827fed2 100644 >> --- a/drivers/gpu/drm/drm_of.c >> +++ b/drivers/gpu/drm/drm_of.c >> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( >> * configurations by passing the endpoints explicitly to >> * drm_of_lvds_get_dual_link_pixel_order(). >> */ >> - if (!current_pt || pixels_type != current_pt) >> + if (!current_pt || pixels_type != current_pt) { >> + of_node_put(endpoint); >> return -EINVAL; >> + } >> } >> >> return pixels_type; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/of: free the iterator object on failure @ 2021-07-15 10:32 ` Steven Price 0 siblings, 0 replies; 20+ messages in thread From: Steven Price @ 2021-07-15 10:32 UTC (permalink / raw) To: Laurent Pinchart Cc: David Airlie, Pavel Machek, linux-kernel, dri-devel, Thomas Zimmermann, Biju Das On 14/07/2021 16:26, Laurent Pinchart wrote: > Hi Steven, > > Thank you for the patch. > > On Wed, Jul 14, 2021 at 03:33:00PM +0100, Steven Price wrote: >> When bailing out due to the sanity check the iterator value needs to be >> freed because the early return prevents for_each_child_of_node() from >> doing the dereference itself. >> >> Fixes: 6529007522de ("drm: of: Add drm_of_lvds_get_dual_link_pixel_order") >> Signed-off-by: Steven Price <steven.price@arm.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! Applied to drm-misc-next. Steve >> --- >> drivers/gpu/drm/drm_of.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> v2: Fixes now refers to the original commit as suggested by Laurent, rather >> than 4ee48cc5586b ("drm: of: Fix double-free bug") which only fixed part of >> the problem. Note that 4ee48cc5586b is a dependency for this patch to >> cleanly apply. >> >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >> index 197c57477344..997b8827fed2 100644 >> --- a/drivers/gpu/drm/drm_of.c >> +++ b/drivers/gpu/drm/drm_of.c >> @@ -331,8 +331,10 @@ static int drm_of_lvds_get_remote_pixels_type( >> * configurations by passing the endpoints explicitly to >> * drm_of_lvds_get_dual_link_pixel_order(). >> */ >> - if (!current_pt || pixels_type != current_pt) >> + if (!current_pt || pixels_type != current_pt) { >> + of_node_put(endpoint); >> return -EINVAL; >> + } >> } >> >> return pixels_type; > ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-07-15 10:32 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-12 15:57 [PATCH] drm/of: free the iterator object on failure Steven Price 2021-07-12 15:57 ` Steven Price 2021-07-12 16:03 ` Biju Das 2021-07-12 16:03 ` Biju Das 2021-07-12 16:50 ` Laurent Pinchart 2021-07-12 16:50 ` Laurent Pinchart 2021-07-12 21:31 ` Steven Price 2021-07-12 21:31 ` Steven Price 2021-07-12 21:55 ` Laurent Pinchart 2021-07-12 21:55 ` Laurent Pinchart 2021-07-13 16:16 ` Steven Price 2021-07-13 16:16 ` Steven Price 2021-07-14 8:35 ` Laurent Pinchart 2021-07-14 8:35 ` Laurent Pinchart 2021-07-14 14:33 ` [PATCH v2] " Steven Price 2021-07-14 14:33 ` Steven Price 2021-07-14 15:26 ` Laurent Pinchart 2021-07-14 15:26 ` Laurent Pinchart 2021-07-15 10:32 ` Steven Price 2021-07-15 10:32 ` Steven Price
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.