| Message ID | 1551250826-10302-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | phy: renesas: rcar-gen2: Fix memory leak at error paths | expand |
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
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
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
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
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 >
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);
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(+)