[PATCH net 0/4] net: fix device reference leaks

[PATCH net 0/4] net: fix device reference leaks

[Johan Hovold]

This series fixes a number of device reference leaks (and one of_node
leak) due to failure to drop the references taken by bus_find_device()
and friends.

Note that the final two patches have been compile tested only.

Thanks,
Johan


Johan Hovold (4):
  phy: fix device reference leaks
  net: ethernet: ti: cpsw: fix device and of_node leaks
  net: ethernet: ti: davinci_emac: fix device reference leak
  net: hns: fix device reference leaks

 drivers/net/ethernet/hisilicon/hns/hnae.c |  8 +++++++-
 drivers/net/ethernet/ti/cpsw-phy-sel.c    |  3 +++
 drivers/net/ethernet/ti/davinci_emac.c    | 10 ++++++----
 drivers/net/phy/phy_device.c              |  2 ++
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.7.3

[PATCH net 1/4] phy: fix device reference leaks

[Johan Hovold]

Make sure to drop the reference taken by bus_find_device_by_name()
before returning from phy_connect() and phy_attach().

Note that both function still take a reference to the phy device
through phy_attach_direct().

Fixes: e13934563db0 ("[PATCH] PHY Layer fixup")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e977ba931878..1a4bf8acad78 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -723,6 +723,7 @@ struct phy_device *phy_connect(struct net_device *dev, const char *bus_id,
 	phydev = to_phy_device(d);
 
 	rc = phy_connect_direct(dev, phydev, handler, interface);
+	put_device(d);
 	if (rc)
 		return ERR_PTR(rc);
 
@@ -953,6 +954,7 @@ struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
 	phydev = to_phy_device(d);
 
 	rc = phy_attach_direct(dev, phydev, phydev->dev_flags, interface);
+	put_device(d);
 	if (rc)
 		return ERR_PTR(rc);
 
-- 
2.7.3

[PATCH net 2/4] net: ethernet: ti: cpsw: fix device and of_node leaks

[Johan Hovold]

Make sure to drop the references taken by of_get_child_by_name() and
bus_find_device() before returning from cpsw_phy_sel().

Note that there is no guarantee that the devres-managed struct
cpsw_phy_sel_priv will continue to be valid until this function returns
regardless of this change.

Fixes: 5892cd135e16 ("drivers: net: cpsw-phy-sel: Add new driver...")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw-phy-sel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
index 054a8dd23dae..589beb843f56 100644
--- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
+++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
@@ -176,8 +176,11 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
 	}
 
 	dev = bus_find_device(&platform_bus_type, NULL, node, match);
+	of_node_put(node);
 	priv = dev_get_drvdata(dev);
 
+	put_device(dev);
+
 	priv->cpsw_phy_sel(priv, phy_mode, slave);
 }
 EXPORT_SYMBOL_GPL(cpsw_phy_sel);
-- 
2.7.3

[PATCH net 3/4] net: ethernet: ti: davinci_emac: fix device reference leak

[Johan Hovold]

Make sure to drop the references taken by bus_find_device() before
returning from emac_dev_open().

Note that phy_connect still takes a reference to the phy device.

Fixes: 5d69e0076a72 ("net: davinci_emac: switch to new mdio")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/ti/davinci_emac.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 2fd94a5bc1f3..84fbe5714f8b 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1410,6 +1410,7 @@ static int emac_dev_open(struct net_device *ndev)
 	int i = 0;
 	struct emac_priv *priv = netdev_priv(ndev);
 	struct phy_device *phydev = NULL;
+	struct device *phy = NULL;
 
 	ret = pm_runtime_get_sync(&priv->pdev->dev);
 	if (ret < 0) {
@@ -1488,19 +1489,20 @@ static int emac_dev_open(struct net_device *ndev)
 
 	/* use the first phy on the bus if pdata did not give us a phy id */
 	if (!phydev && !priv->phy_id) {
-		struct device *phy;
-
 		phy = bus_find_device(&mdio_bus_type, NULL, NULL,
 				      match_first_device);
-		if (phy)
+		if (phy) {
 			priv->phy_id = dev_name(phy);
+			if (!priv->phy_id || !*priv->phy_id)
+				put_device(phy);
+		}
 	}
 
 	if (!phydev && priv->phy_id && *priv->phy_id) {
 		phydev = phy_connect(ndev, priv->phy_id,
 				     &emac_adjust_link,
 				     PHY_INTERFACE_MODE_MII);
-
+		put_device(phy);	/* reference taken by bus_find_device */
 		if (IS_ERR(phydev)) {
 			dev_err(emac_dev, "could not connect to phy %s\n",
 				priv->phy_id);
-- 
2.7.3

[PATCH net 4/4] net: hns: fix device reference leaks

[Johan Hovold]

Make sure to drop the reference taken by class_find_device() in
hnae_get_handle() on errors and when later releasing the handle.

Fixes: 6fe6611ff275 ("net: add Hisilicon Network Subsystem...")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/hisilicon/hns/hnae.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c
index c54c6fac0d1d..b6ed818f78ff 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.c
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.c
@@ -332,8 +332,10 @@ struct hnae_handle *hnae_get_handle(struct device *owner_dev,
 		return ERR_PTR(-ENODEV);
 
 	handle = dev->ops->get_handle(dev, port_id);
-	if (IS_ERR(handle))
+	if (IS_ERR(handle)) {
+		put_device(&dev->cls_dev);
 		return handle;
+	}
 
 	handle->dev = dev;
 	handle->owner_dev = owner_dev;
@@ -356,6 +358,8 @@ struct hnae_handle *hnae_get_handle(struct device *owner_dev,
 	for (j = i - 1; j >= 0; j--)
 		hnae_fini_queue(handle->qs[j]);
 
+	put_device(&dev->cls_dev);
+
 	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL(hnae_get_handle);
@@ -377,6 +381,8 @@ void hnae_put_handle(struct hnae_handle *h)
 		dev->ops->put_handle(h);
 
 	module_put(dev->owner);
+
+	put_device(&dev->cls_dev);
 }
 EXPORT_SYMBOL(hnae_put_handle);
 
-- 
2.7.3

Re: [PATCH net 2/4] net: ethernet: ti: cpsw: fix device and of_node leaks

[David Miller]

From: Johan Hovold <johan@kernel.org>
Date: Tue,  1 Nov 2016 12:03:35 +0100

> diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> index 054a8dd23dae..589beb843f56 100644
> --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
> +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> @@ -176,8 +176,11 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
>  	}
>  
>  	dev = bus_find_device(&platform_bus_type, NULL, node, match);
> +	of_node_put(node);
>  	priv = dev_get_drvdata(dev);
>  
> +	put_device(dev);
> +
>  	priv->cpsw_phy_sel(priv, phy_mode, slave);
>  }
>  EXPORT_SYMBOL_GPL(cpsw_phy_sel);

The only reference you have to 'dev' is the one obtained from the
bus_find_device() call, therefore you must at least hold onto
'dev' until after the priv->cpsw_phy_sel(priv, phy_mode, slave); call.

Re: [PATCH net 2/4] net: ethernet: ti: cpsw: fix device and of_node leaks

[Johan Hovold]

On Tue, Nov 01, 2016 at 12:27:11PM -0400, David Miller wrote:
> From: Johan Hovold <johan@kernel.org>
> Date: Tue,  1 Nov 2016 12:03:35 +0100
> 
> > diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > index 054a8dd23dae..589beb843f56 100644
> > --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > @@ -176,8 +176,11 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
> >  	}
> >  
> >  	dev = bus_find_device(&platform_bus_type, NULL, node, match);
> > +	of_node_put(node);
> >  	priv = dev_get_drvdata(dev);
> >  
> > +	put_device(dev);
> > +
> >  	priv->cpsw_phy_sel(priv, phy_mode, slave);
> >  }
> >  EXPORT_SYMBOL_GPL(cpsw_phy_sel);
> 
> The only reference you have to 'dev' is the one obtained from the
> bus_find_device() call, therefore you must at least hold onto
> 'dev' until after the priv->cpsw_phy_sel(priv, phy_mode, slave); call.

As I mentioned in the commit message "...there is no guarantee that the
devres-managed struct cpsw_phy_sel_priv will continue to be valid until
this function returns regardless of this change".

Specifically, holding a reference to dev does not prevent the
cpsw_phy_sel driver from being unbound and priv from being freed.

Thanks,
Johan

Re: [PATCH net 2/4] net: ethernet: ti: cpsw: fix device and of_node leaks

[David Miller]

From: Johan Hovold <johan@kernel.org>
Date: Tue, 1 Nov 2016 17:42:25 +0100

> On Tue, Nov 01, 2016 at 12:27:11PM -0400, David Miller wrote:
>> From: Johan Hovold <johan@kernel.org>
>> Date: Tue,  1 Nov 2016 12:03:35 +0100
>> 
>> > diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
>> > index 054a8dd23dae..589beb843f56 100644
>> > --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
>> > +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
>> > @@ -176,8 +176,11 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
>> >  	}
>> >  
>> >  	dev = bus_find_device(&platform_bus_type, NULL, node, match);
>> > +	of_node_put(node);
>> >  	priv = dev_get_drvdata(dev);
>> >  
>> > +	put_device(dev);
>> > +
>> >  	priv->cpsw_phy_sel(priv, phy_mode, slave);
>> >  }
>> >  EXPORT_SYMBOL_GPL(cpsw_phy_sel);
>> 
>> The only reference you have to 'dev' is the one obtained from the
>> bus_find_device() call, therefore you must at least hold onto
>> 'dev' until after the priv->cpsw_phy_sel(priv, phy_mode, slave); call.
> 
> As I mentioned in the commit message "...there is no guarantee that the
> devres-managed struct cpsw_phy_sel_priv will continue to be valid until
> this function returns regardless of this change".
> 
> Specifically, holding a reference to dev does not prevent the
> cpsw_phy_sel driver from being unbound and priv from being freed.

But you should at least hold onto the object while you call a function
pointer embedded in a data structure referred by it.

Re: [PATCH net 2/4] net: ethernet: ti: cpsw: fix device and of_node leaks

[Johan Hovold]

On Tue, Nov 01, 2016 at 12:48:48PM -0400, David Miller wrote:
> From: Johan Hovold <johan@kernel.org>
> Date: Tue, 1 Nov 2016 17:42:25 +0100
> 
> > On Tue, Nov 01, 2016 at 12:27:11PM -0400, David Miller wrote:
> >> From: Johan Hovold <johan@kernel.org>
> >> Date: Tue,  1 Nov 2016 12:03:35 +0100
> >> 
> >> > diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> >> > index 054a8dd23dae..589beb843f56 100644
> >> > --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
> >> > +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> >> > @@ -176,8 +176,11 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
> >> >  	}
> >> >  
> >> >  	dev = bus_find_device(&platform_bus_type, NULL, node, match);
> >> > +	of_node_put(node);
> >> >  	priv = dev_get_drvdata(dev);
> >> >  
> >> > +	put_device(dev);
> >> > +
> >> >  	priv->cpsw_phy_sel(priv, phy_mode, slave);
> >> >  }
> >> >  EXPORT_SYMBOL_GPL(cpsw_phy_sel);
> >> 
> >> The only reference you have to 'dev' is the one obtained from the
> >> bus_find_device() call, therefore you must at least hold onto
> >> 'dev' until after the priv->cpsw_phy_sel(priv, phy_mode, slave); call.
> > 
> > As I mentioned in the commit message "...there is no guarantee that the
> > devres-managed struct cpsw_phy_sel_priv will continue to be valid until
> > this function returns regardless of this change".
> > 
> > Specifically, holding a reference to dev does not prevent the
> > cpsw_phy_sel driver from being unbound and priv from being freed.
> 
> But you should at least hold onto the object while you call a function
> pointer embedded in a data structure referred by it.

But there is no such reference to be held (currently). While priv is
valid (i.e. dev has a driver bound), driver core will hold a reference
to dev. If someone unbinds the driver, priv will be gone long before dev
and all bets are off anyway.

So I released the reference to dev before dereferencing priv on purpose
to avoid having someone make false assumptions about the lifetime of priv.

Johan