From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6B2C7C07E99 for ; Mon, 12 Jul 2021 21:32:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3E02B61242 for ; Mon, 12 Jul 2021 21:32:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234233AbhGLVew (ORCPT ); Mon, 12 Jul 2021 17:34:52 -0400 Received: from foss.arm.com ([217.140.110.172]:33138 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231878AbhGLVew (ORCPT ); Mon, 12 Jul 2021 17:34:52 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 31BD26D; Mon, 12 Jul 2021 14:32:03 -0700 (PDT) Received: from [192.168.1.179] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BFFCA3F694; Mon, 12 Jul 2021 14:32:01 -0700 (PDT) Subject: Re: [PATCH] drm/of: free the iterator object on failure To: Laurent Pinchart Cc: Daniel Vetter , David Airlie , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Biju Das References: <20210712155758.48286-1-steven.price@arm.com> From: Steven Price Message-ID: Date: Mon, 12 Jul 2021 22:31:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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; >