* [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
@ 2019-02-27 7:00 Yoshihiro Shimoda
2019-02-27 8:25 ` Julia Lawall
2019-02-27 10:47 ` Geert Uytterhoeven
0 siblings, 2 replies; 6+ messages in thread
From: Yoshihiro Shimoda @ 2019-02-27 7:00 UTC (permalink / raw)
To: kishon; +Cc: julia.lawall, linux-kernel, linux-renesas-soc, Yoshihiro Shimoda
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 <julia.lawall@lip6.fr>
Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
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);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
2019-02-27 7:00 [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths Yoshihiro Shimoda
@ 2019-02-27 8:25 ` Julia Lawall
2019-02-27 8:53 ` Yoshihiro Shimoda
2019-02-27 10:47 ` Geert Uytterhoeven
1 sibling, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2019-02-27 8:25 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: kishon, linux-kernel, linux-renesas-soc
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 <julia.lawall@lip6.fr>
> Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> 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?
julia
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
2019-02-27 8:25 ` Julia Lawall
@ 2019-02-27 8:53 ` Yoshihiro Shimoda
2019-02-27 10:53 ` Geert Uytterhoeven
2019-02-27 20:55 ` Julia Lawall
0 siblings, 2 replies; 6+ messages in thread
From: Yoshihiro Shimoda @ 2019-02-27 8:53 UTC (permalink / raw)
To: Julia Lawall; +Cc: kishon, linux-kernel, linux-renesas-soc
Hello,
> 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 <julia.lawall@lip6.fr>
> > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> > 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;
}
---
Best regards,
Yoshihiro Shimoda
> julia
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
2019-02-27 7:00 [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths Yoshihiro Shimoda
2019-02-27 8:25 ` Julia Lawall
@ 2019-02-27 10:47 ` Geert Uytterhoeven
1 sibling, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2019-02-27 10:47 UTC (permalink / raw)
To: Yoshihiro Shimoda
Cc: Kishon Vijay Abraham I, Julia Lawall, Linux Kernel Mailing List,
Linux-Renesas
Hi Shimoda-san,
On Wed, Feb 27, 2019 at 8:00 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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 <julia.lawall@lip6.fr>
> Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
2019-02-27 8:53 ` Yoshihiro Shimoda
@ 2019-02-27 10:53 ` Geert Uytterhoeven
2019-02-27 20:55 ` Julia Lawall
1 sibling, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2019-02-27 10:53 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: Julia Lawall, kishon, linux-kernel, linux-renesas-soc
Hi Shimoda-san,
On Wed, Feb 27, 2019 at 9:53 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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 <julia.lawall@lip6.fr>
> > > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths
2019-02-27 8:53 ` Yoshihiro Shimoda
2019-02-27 10:53 ` Geert Uytterhoeven
@ 2019-02-27 20:55 ` Julia Lawall
1 sibling, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2019-02-27 20:55 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: kishon, linux-kernel, linux-renesas-soc
On Wed, 27 Feb 2019, Yoshihiro Shimoda wrote:
> Hello,
>
> > 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 <julia.lawall@lip6.fr>
> > > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > > 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?
I see that the value is only used for comparison. Thanks for pointing
that out. If you are sure that the pointers are the same when the
function is called as when the pointer to the possibly now freed device
node was stored in the channels structure, then everything should be ok.
julia
>
> ---
> 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;
> }
> ---
>
> Best regards,
> Yoshihiro Shimoda
>
> > julia
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-27 20:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 7:00 [PATCH] phy: renesas: rcar-gen2: Fix memory leak at error paths Yoshihiro Shimoda
2019-02-27 8:25 ` Julia Lawall
2019-02-27 8:53 ` Yoshihiro Shimoda
2019-02-27 10:53 ` Geert Uytterhoeven
2019-02-27 20:55 ` Julia Lawall
2019-02-27 10:47 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).