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 <[email protected]>
Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
Signed-off-by: Yoshihiro Shimoda <[email protected]>
---
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
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 <[email protected]>
> Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> Signed-off-by: Yoshihiro Shimoda <[email protected]>
> ---
> 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
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 <[email protected]>
> > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > Signed-off-by: Yoshihiro Shimoda <[email protected]>
> > ---
> > 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
Hi Shimoda-san,
On Wed, Feb 27, 2019 at 8:00 AM Yoshihiro Shimoda
<[email protected]> 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 <[email protected]>
> Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> Signed-off-by: Yoshihiro Shimoda <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Shimoda-san,
On Wed, Feb 27, 2019 at 9:53 AM Yoshihiro Shimoda
<[email protected]> 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 <[email protected]>
> > > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > > Signed-off-by: Yoshihiro Shimoda <[email protected]>
> > > ---
> > > 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 -- [email protected]
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
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 <[email protected]>
> > > Fixes: 1233f59f745 ("phy: Renesas R-Car Gen2 PHY driver")
> > > Signed-off-by: Yoshihiro Shimoda <[email protected]>
> > > ---
> > > 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
>