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=-2.4 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=unavailable 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 A7DB2C04AAD for ; Mon, 6 May 2019 17:48:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 88AE72082F for ; Mon, 6 May 2019 17:48:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726703AbfEFRss (ORCPT ); Mon, 6 May 2019 13:48:48 -0400 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:51119 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725883AbfEFRss (ORCPT ); Mon, 6 May 2019 13:48:48 -0400 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id 3E6AD80179; Mon, 6 May 2019 19:48:35 +0200 (CEST) Date: Mon, 6 May 2019 19:48:46 +0200 From: Pavel Machek To: wen.yang99@zte.com.cn Cc: pavel@denx.de, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, anirudh@xilinx.com, John.Linn@xilinx.com, davem@davemloft.net, michal.simek@xilinx.com, netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sashal@kernel.org Subject: Re: [PATCH 4.19 46/72] net: xilinx: fix possible object referenceleak Message-ID: <20190506174846.GA13326@amd> References: <20190503100816.GD5834@amd> <201905051417486865228@zte.com.cn> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="VS++wcV0S1rZb1Fb" Content-Disposition: inline In-Reply-To: <201905051417486865228@zte.com.cn> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --VS++wcV0S1rZb1Fb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi! > > > [ Upstream commit fa3a419d2f674b431d38748cb58fb7da17ee8949 ] > > > > > > The call to of_parse_phandle returns a node pointer with refcount > > > incremented thus it must be explicitly decremented after the last > > > usage. > > > > > > Detected by coccinelle with the following warnings: > > > ./drivers/net/ethernet/xilinx/xilinx_axienet_main.c:1624:1-7: ERROR: = missing of_node_put; acquired a node pointer with refcount incremented on l= ine 1569, but without a corresponding object release within this function. > >=20 > > Bug is real, but fix is horrible. This already uses gotos for error > > handling, so use them.... > >=20 > > This fixes it up. > >=20 > > Plus... I do not think these "of_node_put" fixes belong in > > stable. They are theoretical bugs; so we hold reference to device tree > > structure. a) it is small, b) it stays in memory, anyway. This does > > not fix any real problem. > >=20 >=20 > Thank you very much for your comments. > We developed the following coccinelle SmPL to look for places where > there is an of_node_put on some path but not on others. I agree that the fix is good. Thanks for doing coccinelle work. > We use it to detect drivers/net/ethernet/xilinx/xilinx_axienet_main.c and= found the following issue: >=20 > static int axienet_probe(struct platform_device *pdev) > { > ... > struct device_node *np; > ... > if (ret) { > dev_err(&pdev->dev, "unable to get DMA resource\n"); > goto free_netdev; ---> leaked here > } > ... > if (IS_ERR(lp->dma_regs)) { > dev_err(&pdev->dev, "could not map DMA regs\n"); > ret =3D PTR_ERR(lp->dma_regs); > goto free_netdev; ---> leaked here > } > ... > of_node_put(np); ---> released here > ... > free_netdev: > free_netdev(ndev); >=20 > return ret; > } >=20 > If we insmod/rmmod xilinx_emaclite.ko multiple times,=20 > axienet_probe() may be called multiple times, then a resource leak > may occur. Yeah, well. I agree the bug is real. But how much memory will it leak during each insmod? Kilobyte? (Is it actually anything at all? I'd expect just reference counter to be increaed.) How often do you usually insmod? > At the same time, we also checked the code for handling resource leaks in= the current kernel > and found that the regular of_node_put mode is commonly used in > addition to the goto target mode. Ok, so this uglyness happens elsewhere. But I'd really prefer to use goto if it is already used in the function. Thanks, Pavel --=20 DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany --VS++wcV0S1rZb1Fb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlzQc34ACgkQMOfwapXb+vKjLQCfR30gJwbflpVIZMeXq9XtoP1X bpMAn0gYdpIGkf2vx98ZqTyzLuMecomn =Ekqr -----END PGP SIGNATURE----- --VS++wcV0S1rZb1Fb-- 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=-2.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,T_DKIMWL_WL_HIGH, USER_AGENT_MUTT 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 1F82DC04A6B for ; Mon, 6 May 2019 17:49:09 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EDA992087F for ; Mon, 6 May 2019 17:49:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="CRRjZbto" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EDA992087F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=oCON9Nnl8KVqPAc58aPBhWykGeQquBxiwp5YUUHIoss=; b=CRRjZbtox2j+CmLS9yLi8xTTD gyow1U6ER2xYOocxeqtel6EqyUPEdECH5QDoa9aWXDUCkAG/GocgGytiUIB+uxllO/D+gAyulqhlX UmGx2apPzka35ZK9zisUFk9fLSwJK1PuJJULoq3qlWIvvThcbMHTExBgglbVVOibM9KnHLurwzULE I606TBZktcGr/VYSweeyktBSTlgebKQssXNqneLXKbFAizccq3Tj9xlPFrrD23ZDflxcXZv3L3zoL 3nlvDZA0WqZyRahGFGKarCq/nxhGvzsj1ws5bGcdTgkng2fVbf+JlZCqKsfEdxafAQZ9dkzwy8vjU Rb/Rkazng==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hNhjb-0006lK-HX; Mon, 06 May 2019 17:49:03 +0000 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1hNhjW-0006kV-Rv for linux-arm-kernel@lists.infradead.org; Mon, 06 May 2019 17:49:01 +0000 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id 3E6AD80179; Mon, 6 May 2019 19:48:35 +0200 (CEST) Date: Mon, 6 May 2019 19:48:46 +0200 From: Pavel Machek To: wen.yang99@zte.com.cn Subject: Re: [PATCH 4.19 46/72] net: xilinx: fix possible object referenceleak Message-ID: <20190506174846.GA13326@amd> References: <20190503100816.GD5834@amd> <201905051417486865228@zte.com.cn> MIME-Version: 1.0 In-Reply-To: <201905051417486865228@zte.com.cn> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190506_104859_201663_ABACAC7B X-CRM114-Status: GOOD ( 26.21 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: sashal@kernel.org, michal.simek@xilinx.com, pavel@denx.de, linux-kernel@vger.kernel.org, stable@vger.kernel.org, davem@davemloft.net, netdev@vger.kernel.org, anirudh@xilinx.com, gregkh@linuxfoundation.org, John.Linn@xilinx.com, linux-arm-kernel@lists.infradead.org Content-Type: multipart/mixed; boundary="===============3860526148423131022==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============3860526148423131022== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="VS++wcV0S1rZb1Fb" Content-Disposition: inline --VS++wcV0S1rZb1Fb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi! > > > [ Upstream commit fa3a419d2f674b431d38748cb58fb7da17ee8949 ] > > > > > > The call to of_parse_phandle returns a node pointer with refcount > > > incremented thus it must be explicitly decremented after the last > > > usage. > > > > > > Detected by coccinelle with the following warnings: > > > ./drivers/net/ethernet/xilinx/xilinx_axienet_main.c:1624:1-7: ERROR: = missing of_node_put; acquired a node pointer with refcount incremented on l= ine 1569, but without a corresponding object release within this function. > >=20 > > Bug is real, but fix is horrible. This already uses gotos for error > > handling, so use them.... > >=20 > > This fixes it up. > >=20 > > Plus... I do not think these "of_node_put" fixes belong in > > stable. They are theoretical bugs; so we hold reference to device tree > > structure. a) it is small, b) it stays in memory, anyway. This does > > not fix any real problem. > >=20 >=20 > Thank you very much for your comments. > We developed the following coccinelle SmPL to look for places where > there is an of_node_put on some path but not on others. I agree that the fix is good. Thanks for doing coccinelle work. > We use it to detect drivers/net/ethernet/xilinx/xilinx_axienet_main.c and= found the following issue: >=20 > static int axienet_probe(struct platform_device *pdev) > { > ... > struct device_node *np; > ... > if (ret) { > dev_err(&pdev->dev, "unable to get DMA resource\n"); > goto free_netdev; ---> leaked here > } > ... > if (IS_ERR(lp->dma_regs)) { > dev_err(&pdev->dev, "could not map DMA regs\n"); > ret =3D PTR_ERR(lp->dma_regs); > goto free_netdev; ---> leaked here > } > ... > of_node_put(np); ---> released here > ... > free_netdev: > free_netdev(ndev); >=20 > return ret; > } >=20 > If we insmod/rmmod xilinx_emaclite.ko multiple times,=20 > axienet_probe() may be called multiple times, then a resource leak > may occur. Yeah, well. I agree the bug is real. But how much memory will it leak during each insmod? Kilobyte? (Is it actually anything at all? I'd expect just reference counter to be increaed.) How often do you usually insmod? > At the same time, we also checked the code for handling resource leaks in= the current kernel > and found that the regular of_node_put mode is commonly used in > addition to the goto target mode. Ok, so this uglyness happens elsewhere. But I'd really prefer to use goto if it is already used in the function. Thanks, Pavel --=20 DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany --VS++wcV0S1rZb1Fb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlzQc34ACgkQMOfwapXb+vKjLQCfR30gJwbflpVIZMeXq9XtoP1X bpMAn0gYdpIGkf2vx98ZqTyzLuMecomn =Ekqr -----END PGP SIGNATURE----- --VS++wcV0S1rZb1Fb-- --===============3860526148423131022== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============3860526148423131022==--