| Message ID | 20220407133036.213217-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Dynamic OF and use after free related fixes | expand |
Quoting Nuno Sá (2022-04-07 06:30:33) > We need to call 'of_node_put()' when we are done with the node or on > error paths. Otherwise this can leak memory in DYNAMIC_OF setups. > > Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates assigned from device tree") > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > (cherry picked from commit 69eb47a26e7f728a5c052687380993cd9a0dd643) This line should be removed. > --- > drivers/clk/clk-conf.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c > index 2ef819606c41..d6065cdf1540 100644 > --- a/drivers/clk/clk-conf.c > +++ b/drivers/clk/clk-conf.c > @@ -33,9 +33,12 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) > else > return rc; > } > - if (clkspec.np == node && !clk_supplier) > + if (clkspec.np == node && !clk_supplier) { > + of_node_put(clkspec.np); > return 0; > + } > pclk = of_clk_get_from_provider(&clkspec); > + of_node_put(clkspec.np); > if (IS_ERR(pclk)) { > if (PTR_ERR(pclk) != -EPROBE_DEFER) > pr_warn("clk: couldn't get parent clock %d for %pOF\n", I suspect it would be easier to follow and fix if the code was reorganized to have most of the contents inside this function as a sub-routine that is called for each index. Then all the node putting and clk putting can be in one place in that other function and this is a simple loop around that stops on the first error. > @@ -49,7 +52,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) > goto err; > if (clkspec.np == node && !clk_supplier) { > rc = 0; > - goto err; > + goto err_of_put; > } > clk = of_clk_get_from_provider(&clkspec); > if (IS_ERR(clk)) {
> -----Original Message----- > From: Stephen Boyd <sboyd@kernel.org> > Sent: Thursday, April 21, 2022 9:58 PM > To: Sa, Nuno <Nuno.Sa@analog.com>; linux-clk@vger.kernel.org > Cc: Michael Turquette <mturquette@baylibre.com> > Subject: Re: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes > > [External] > > Quoting Nuno Sá (2022-04-07 06:30:33) > > We need to call 'of_node_put()' when we are done with the node or > on > > error paths. Otherwise this can leak memory in DYNAMIC_OF setups. > > > > Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates > assigned from device tree") > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > (cherry picked from commit > 69eb47a26e7f728a5c052687380993cd9a0dd643) > > This line should be removed. > Yes, I know :). I Was just waiting for the review. Somehow I messed up when preparing the patches to submit... - Nuno Sá
> -----Original Message----- > From: Stephen Boyd <sboyd@kernel.org> > Sent: Thursday, April 21, 2022 9:58 PM > To: Sa, Nuno <Nuno.Sa@analog.com>; linux-clk@vger.kernel.org > Cc: Michael Turquette <mturquette@baylibre.com> > Subject: Re: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes > > [External] > > Quoting Nuno Sá (2022-04-07 06:30:33) > > We need to call 'of_node_put()' when we are done with the node or > on > > error paths. Otherwise this can leak memory in DYNAMIC_OF setups. > > > > Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates > assigned from device tree") > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > (cherry picked from commit > 69eb47a26e7f728a5c052687380993cd9a0dd643) > > This line should be removed. > > > --- > > drivers/clk/clk-conf.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c > > index 2ef819606c41..d6065cdf1540 100644 > > --- a/drivers/clk/clk-conf.c > > +++ b/drivers/clk/clk-conf.c > > @@ -33,9 +33,12 @@ static int __set_clk_parents(struct > device_node *node, bool clk_supplier) > > else > > return rc; > > } > > - if (clkspec.np == node && !clk_supplier) > > + if (clkspec.np == node && !clk_supplier) { > > + of_node_put(clkspec.np); > > return 0; > > + } > > pclk = of_clk_get_from_provider(&clkspec); > > + of_node_put(clkspec.np); > > if (IS_ERR(pclk)) { > > if (PTR_ERR(pclk) != -EPROBE_DEFER) > > pr_warn("clk: couldn't get parent clock %d for > %pOF\n", > > I suspect it would be easier to follow and fix if the code was > reorganized to have most of the contents inside this function as a > sub-routine that is called for each index. Then all the node putting and > clk putting can be in one place in that other function and this is a > simple loop around that stops on the first error. > Ups, forgot to answer this one. Yeah, I did not wanted to change much but I agree that the code could be a bit refactored and I can do that in v2. - Nuno Sá
diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c index 2ef819606c41..d6065cdf1540 100644 --- a/drivers/clk/clk-conf.c +++ b/drivers/clk/clk-conf.c @@ -33,9 +33,12 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) else return rc; } - if (clkspec.np == node && !clk_supplier) + if (clkspec.np == node && !clk_supplier) { + of_node_put(clkspec.np); return 0; + } pclk = of_clk_get_from_provider(&clkspec); + of_node_put(clkspec.np); if (IS_ERR(pclk)) { if (PTR_ERR(pclk) != -EPROBE_DEFER) pr_warn("clk: couldn't get parent clock %d for %pOF\n", @@ -49,7 +52,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) goto err; if (clkspec.np == node && !clk_supplier) { rc = 0; - goto err; + goto err_of_put; } clk = of_clk_get_from_provider(&clkspec); if (IS_ERR(clk)) { @@ -57,7 +60,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) pr_warn("clk: couldn't get assigned clock %d for %pOF\n", index, node); rc = PTR_ERR(clk); - goto err; + goto err_of_put; } rc = clk_set_parent(clk, pclk); @@ -66,8 +69,11 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) __clk_get_name(clk), __clk_get_name(pclk), rc); clk_put(clk); clk_put(pclk); + of_node_put(clkspec.np); } return 0; +err_of_put: + of_node_put(clkspec.np); err: clk_put(pclk); return rc; @@ -93,14 +99,17 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier) else return rc; } - if (clkspec.np == node && !clk_supplier) + if (clkspec.np == node && !clk_supplier) { + of_node_put(clkspec.np); return 0; + } clk = of_clk_get_from_provider(&clkspec); if (IS_ERR(clk)) { if (PTR_ERR(clk) != -EPROBE_DEFER) pr_warn("clk: couldn't get clock %d for %pOF\n", index, node); + of_node_put(clkspec.np); return PTR_ERR(clk); } @@ -110,6 +119,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier) __clk_get_name(clk), rate, rc, clk_get_rate(clk)); clk_put(clk); + of_node_put(clkspec.np); } index++; }
We need to call 'of_node_put()' when we are done with the node or on error paths. Otherwise this can leak memory in DYNAMIC_OF setups. Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates assigned from device tree") Signed-off-by: Nuno Sá <nuno.sa@analog.com> (cherry picked from commit 69eb47a26e7f728a5c052687380993cd9a0dd643) --- drivers/clk/clk-conf.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)