From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Steven Price <steven.price@arm.com> Cc: 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>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Biju Das <biju.das.jz@bp.renesas.com> Subject: Re: [PATCH] drm/of: free the iterator object on failure Date: Tue, 13 Jul 2021 00:55:33 +0300 [thread overview] Message-ID: <YOy6VQNz8Htg6Usb@pendragon.ideasonboard.com> (raw) In-Reply-To: <b420a4e6-8038-6c1e-7c97-75ef3bea3c21@arm.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Steven Price <steven.price@arm.com> Cc: David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Thomas Zimmermann <tzimmermann@suse.de>, Biju Das <biju.das.jz@bp.renesas.com> Subject: Re: [PATCH] drm/of: free the iterator object on failure Date: Tue, 13 Jul 2021 00:55:33 +0300 [thread overview] Message-ID: <YOy6VQNz8Htg6Usb@pendragon.ideasonboard.com> (raw) In-Reply-To: <b420a4e6-8038-6c1e-7c97-75ef3bea3c21@arm.com> 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
next prev parent reply other threads:[~2021-07-12 21:56 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YOy6VQNz8Htg6Usb@pendragon.ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=airlied@linux.ie \ --cc=biju.das.jz@bp.renesas.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=steven.price@arm.com \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.