Received: by 2002:ac0:8845:0:0:0:0:0 with SMTP id g63csp1637798img; Wed, 27 Feb 2019 02:55:04 -0800 (PST) X-Google-Smtp-Source: AHgI3IYT4TMu/9Lq7QOXISGgg0bJUt1o/2HmBlM1+nRN/OmY/JrOSaQYgvSggSOCZliSt+C9jpAO X-Received: by 2002:a17:902:f83:: with SMTP id 3mr1541386plz.125.1551264904853; Wed, 27 Feb 2019 02:55:04 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1551264904; cv=none; d=google.com; s=arc-20160816; b=Myq+zFH9ajiohTHKEzlzM+0POqdWWp84bj0b8a+5MVhfQggpiUPtf7L1KFxdtlHBn3 4PZJtWpUWUPJXEeB5azGHl2I4G9xrD7HTp+J+XZ+icFtcCDMCGuoxnkGD6hP4vSXOW09 oaHR/eiN52YiqKe1hgu5NstgM5IFgzwJgEzKHUotpC1i9htlM7qtDhD3Yc3IM4wYKni4 HMCCTLlJ58TmoN7JJyZBq6dOY1vKtJ9ODovXI987Le7rQE1MAtN427I4VbnOOB99MdCC W8L7sbcVUoTZmoH48Nr2LpFSIlK9HWahivfvD1xSw6l+d/J1s+/bQVNIgGotCz8Z/Dtl fW4w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version; bh=NuTIOVOuY7I7P7hxVe+SwtAhqXFMqR8h72mwBUMo7PU=; b=0rj1pzjCoGJTj5aNZZC+f/AQ6BnIpG5qxLfSEsDTNW0391pBSAQGQwqjvaKTpmPYIh c6VNc9qyfBQ2LqHFM008iGAIuArGKyIbV2tO6ocgNIgSDm6jiwFq4cK6AicqFqsK7h7H TvU1tkx5AAboISpg33lhHrppZf5zC2eTKPsb/M1uRWgqXPfA9DKEJDnVLp348CQEbZwK H7IXBCGMzKlcrwraP5WZEKRnx907oATQe2VWk+B0WWMRAdYvkCZNbAv4WOtLnFkQbx+C VNQeZH0AASTh411UYMH2e+f+tMUsxPEZnNLuPcvZYxR6Y3l4LXc4t19G7knvY8cNM/8D yOwg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l84si5200761pfi.35.2019.02.27.02.54.48; Wed, 27 Feb 2019 02:55:04 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728082AbfB0Kxl (ORCPT + 99 others); Wed, 27 Feb 2019 05:53:41 -0500 Received: from mail-ua1-f67.google.com ([209.85.222.67]:35103 "EHLO mail-ua1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726062AbfB0Kxl (ORCPT ); Wed, 27 Feb 2019 05:53:41 -0500 Received: by mail-ua1-f67.google.com with SMTP id f88so14904489uaf.2; Wed, 27 Feb 2019 02:53:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=NuTIOVOuY7I7P7hxVe+SwtAhqXFMqR8h72mwBUMo7PU=; b=uQ673d1ldAFqhwZgagYQlnVTQawR0ZQHAGbg4W2irHdIbn5w+hEnCUGZb+mID7hk85 dSE8wunBhXmgAXhjLyyFvr8CgzCP0yrNHsr35O6E5VkLemxUxy152ubrOSD0Ybk4gRHW iVrrpwRrZfuj76Tcl8mmR0EeC4dKG6UKEPUMO9lk2WOgM7BK4wE2nlGWA11hdjT/CI1x jnW7KL5ECfN5POvI7ZBmL8D/h4oOOdj8pjAIDeCCch/af5vMgiBOFQO5yFoVZaYyNTbq OutW2ZbNUiPQLjNOG28C+M/C9ETn4wR82yqKexFIOLfDR7Uaaoy1qL9HdQqjMxZM+SSW DUwQ== X-Gm-Message-State: AHQUAuaAvl7+a/ulAAv3e1FxN0+HjuojcTWgVVxM38ravIVILnWRGKry E0JaJ8RFsPGbM706dfqg1rCkPkDiTp7xAOYfhtU= X-Received: by 2002:ab0:6950:: with SMTP id c16mr1728626uas.0.1551264820013; Wed, 27 Feb 2019 02:53:40 -0800 (PST) MIME-Version: 1.0 References: <1551250826-10302-1-git-send-email-yoshihiro.shimoda.uh@renesas.com> In-Reply-To: From: Geert Uytterhoeven Date: Wed, 27 Feb 2019 11:53:28 +0100 Message-ID: Subject: Re: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths To: Yoshihiro Shimoda Cc: Julia Lawall , "kishon@ti.com" , "linux-kernel@vger.kernel.org" , "linux-renesas-soc@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Shimoda-san, On Wed, Feb 27, 2019 at 9:53 AM Yoshihiro Shimoda wrote: > > From: Julia Lawall, Sent: Wednesday, February 27, 2019 5:25 PM > > On Wed, 27 Feb 2019, Yoshihiro Shimoda wrote: > > > This patch fixes memory leak at error paths of the probe function. > > > In for_each_child_of_node, if the loop returns, the driver should > > > call of_put_node() before returns. > > > > > > Reported-by: Julia Lawall > > > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver") > > > Signed-off-by: Yoshihiro Shimoda > > > --- > > > drivers/phy/renesas/phy-rcar-gen2.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/phy/renesas/phy-rcar-gen2.c b/drivers/phy/renesas/phy-rcar-gen2.c > > > index 72eeb06..570b4e4 100644 > > > --- a/drivers/phy/renesas/phy-rcar-gen2.c > > > +++ b/drivers/phy/renesas/phy-rcar-gen2.c > > > @@ -285,6 +285,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev) > > > error = of_property_read_u32(np, "reg", &channel_num); > > > if (error || channel_num > 2) { > > > dev_err(dev, "Invalid \"reg\" property\n"); > > > + of_node_put(np); > > > return error; > > > } > > > channel->select_mask = select_mask[channel_num]; > > > @@ -300,6 +301,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev) > > > &rcar_gen2_phy_ops); > > > if (IS_ERR(phy->phy)) { > > > dev_err(dev, "Failed to create PHY\n"); > > > + of_node_put(np); > > > return PTR_ERR(phy->phy); > > > } > > > phy_set_drvdata(phy->phy, phy); > > > > Hello, > > > > I was concerned about the assignment channel->of_node = np;. Because > > channels is allocated with a devm function, it will get freed on an error > > return, so this pointer doesn't matter. But don't you need an of_node_get > > on this assignment? Does the fact that you haven't seen a problem with > > this in testing mean that the field is actually never accessed? > > The channel->of_node will be used in the rcar_gen2_phy_xlate() as drv->channels[i].of_node. > The assignment is not used for any device tree APIs, just comparing the pointer. > So, I don't think this driver needs an of_node_get() on this assignment. > Is my understanding incorrect? > > --- > static struct phy *rcar_gen2_phy_xlate(struct device *dev, > struct of_phandle_args *args) > { > struct rcar_gen2_phy_driver *drv; > struct device_node *np = args->np; > int i; > > drv = dev_get_drvdata(dev); > if (!drv) > return ERR_PTR(-EINVAL); > > for (i = 0; i < drv->num_channels; i++) { > if (np == drv->channels[i].of_node) // <--- here only > break; > } > > if (i >= drv->num_channels || args->args[0] >= 2) > return ERR_PTR(-ENODEV); > > return drv->channels[i].phys[args->args[0]].phy; > } I think that is OK. You could mark rcar_gen2_channel.o_node const to indicate this, but I don't think that matters much. To make it really safe for future extension, you could call of_node_get(). However, then you have to make sure of_node_put() is always called later, in driver remove and in all probe error paths, complicating the code. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds