[PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Florian Fainelli]

Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
dealt with MDIO bus module reference count, but sort of introduced a
regression in that, if an Ethernet driver registers its own MDIO bus
driver, as is common, we will end up with the Ethernet driver's
module->refnct set to 1, thus preventing this driver from any removal.

Fix this by comparing the network device's device driver owner against
the MDIO bus driver owner, and only if they are different, increment the
MDIO bus module refcount.

Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Russell,

I verified this against the ethoc driver primarily (on a TS7300 board)
and bcmgenet.

Thanks!

 drivers/net/phy/phy_device.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1a4bf8acad78..c4ceb082e970 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface)
 {
+	struct module *ndev_owner = dev->dev.parent->driver->owner;
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct device *d = &phydev->mdio.dev;
 	int err;
 
-	if (!try_module_get(bus->owner)) {
+	/* For Ethernet device drivers that register their own MDIO bus, we
+	 * will have bus->owner match ndev_mod, so we do not want to increment
+	 * our own module->refcnt here, otherwise we would not be able to
+	 * unload later on.
+	 */
+	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
 		dev_err(&dev->dev, "failed to get the bus module\n");
 		return -EIO;
 	}
@@ -921,7 +927,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error:
 	put_device(d);
-	module_put(bus->owner);
+	if (ndev_owner != bus->owner)
+		module_put(bus->owner);
 	return err;
 }
 EXPORT_SYMBOL(phy_attach_direct);
@@ -971,6 +978,8 @@ EXPORT_SYMBOL(phy_attach);
  */
 void phy_detach(struct phy_device *phydev)
 {
+	struct net_device *dev = phydev->attached_dev;
+	struct module *ndev_owner = dev->dev.parent->driver->owner;
 	struct mii_bus *bus;
 	int i;
 
@@ -998,7 +1007,8 @@ void phy_detach(struct phy_device *phydev)
 	bus = phydev->mdio.bus;
 
 	put_device(&phydev->mdio.dev);
-	module_put(bus->owner);
+	if (ndev_owner != bus->owner)
+		module_put(bus->owner);
 }
 EXPORT_SYMBOL(phy_detach);
 
-- 
2.9.3

Re: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue,  6 Dec 2016 20:54:43 -0800

> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
> dealt with MDIO bus module reference count, but sort of introduced a
> regression in that, if an Ethernet driver registers its own MDIO bus
> driver, as is common, we will end up with the Ethernet driver's
> module->refnct set to 1, thus preventing this driver from any removal.
> 
> Fix this by comparing the network device's device driver owner against
> the MDIO bus driver owner, and only if they are different, increment the
> MDIO bus module refcount.
> 
> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Applied.

Re: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Johan Hovold]

On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
> dealt with MDIO bus module reference count, but sort of introduced a
> regression in that, if an Ethernet driver registers its own MDIO bus
> driver, as is common, we will end up with the Ethernet driver's
> module->refnct set to 1, thus preventing this driver from any removal.
> 
> Fix this by comparing the network device's device driver owner against
> the MDIO bus driver owner, and only if they are different, increment the
> MDIO bus module refcount.
> 
> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Russell,
> 
> I verified this against the ethoc driver primarily (on a TS7300 board)
> and bcmgenet.
> 
> Thanks!
> 
>  drivers/net/phy/phy_device.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 1a4bf8acad78..c4ceb082e970 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
>  int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		      u32 flags, phy_interface_t interface)
>  {
> +	struct module *ndev_owner = dev->dev.parent->driver->owner;

Is this really safe? A driver does not need to set a parent device, and
in that case you get a NULL-deref here (I tried using cpsw).

>  	struct mii_bus *bus = phydev->mdio.bus;
>  	struct device *d = &phydev->mdio.dev;
>  	int err;
>  
> -	if (!try_module_get(bus->owner)) {
> +	/* For Ethernet device drivers that register their own MDIO bus, we
> +	 * will have bus->owner match ndev_mod, so we do not want to increment

You also wanted s/ndev_mod/ndev_owner/ here.

> +	 * our own module->refcnt here, otherwise we would not be able to
> +	 * unload later on.
> +	 */
> +	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
>  		dev_err(&dev->dev, "failed to get the bus module\n");
>  		return -EIO;

Johan

Re: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Florian Fainelli]

On 12/08/2016 08:27 AM, Johan Hovold wrote:
> On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
>> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
>> dealt with MDIO bus module reference count, but sort of introduced a
>> regression in that, if an Ethernet driver registers its own MDIO bus
>> driver, as is common, we will end up with the Ethernet driver's
>> module->refnct set to 1, thus preventing this driver from any removal.
>>
>> Fix this by comparing the network device's device driver owner against
>> the MDIO bus driver owner, and only if they are different, increment the
>> MDIO bus module refcount.
>>
>> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Russell,
>>
>> I verified this against the ethoc driver primarily (on a TS7300 board)
>> and bcmgenet.
>>
>> Thanks!
>>
>>  drivers/net/phy/phy_device.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 1a4bf8acad78..c4ceb082e970 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
>>  int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>>  		      u32 flags, phy_interface_t interface)
>>  {
>> +	struct module *ndev_owner = dev->dev.parent->driver->owner;
> 
> Is this really safe? A driver does not need to set a parent device, and
> in that case you get a NULL-deref here (I tried using cpsw).

Humm, cpsw does call SET_NETDEV_DEV() which should take care of that, is
the call made too late? Do you have an example oops?

I don't mind safeguarding this with a check against dev->dev.parent, but
I would like to fix the drivers where relevant too, since
SET_NETDEV_DEV() should really be called, otherwise a number of things
just don't work

> 
>>  	struct mii_bus *bus = phydev->mdio.bus;
>>  	struct device *d = &phydev->mdio.dev;
>>  	int err;
>>  
>> -	if (!try_module_get(bus->owner)) {
>> +	/* For Ethernet device drivers that register their own MDIO bus, we
>> +	 * will have bus->owner match ndev_mod, so we do not want to increment
> 
> You also wanted s/ndev_mod/ndev_owner/ here.

Meh, it's merged now, but thanks, I will fix this once we find out the
proper solution for cpsw.
-- 
Florian

Re: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Johan Hovold]

On Thu, Dec 08, 2016 at 08:47:54AM -0800, Florian Fainelli wrote:
> On 12/08/2016 08:27 AM, Johan Hovold wrote:
> > On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
> >> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
> >> dealt with MDIO bus module reference count, but sort of introduced a
> >> regression in that, if an Ethernet driver registers its own MDIO bus
> >> driver, as is common, we will end up with the Ethernet driver's
> >> module->refnct set to 1, thus preventing this driver from any removal.
> >>
> >> Fix this by comparing the network device's device driver owner against
> >> the MDIO bus driver owner, and only if they are different, increment the
> >> MDIO bus module refcount.
> >>
> >> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >> Russell,
> >>
> >> I verified this against the ethoc driver primarily (on a TS7300 board)
> >> and bcmgenet.
> >>
> >> Thanks!
> >>
> >>  drivers/net/phy/phy_device.c | 16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> >> index 1a4bf8acad78..c4ceb082e970 100644
> >> --- a/drivers/net/phy/phy_device.c
> >> +++ b/drivers/net/phy/phy_device.c
> >> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
> >>  int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> >>  		      u32 flags, phy_interface_t interface)
> >>  {
> >> +	struct module *ndev_owner = dev->dev.parent->driver->owner;
> > 
> > Is this really safe? A driver does not need to set a parent device, and
> > in that case you get a NULL-deref here (I tried using cpsw).
> 
> Humm, cpsw does call SET_NETDEV_DEV() which should take care of that, is
> the call made too late? Do you have an example oops?

Sorry if I was being unclear, cpsw does set a parent device, but there
are network driver that do not. Perhaps such drivers will never hit this
code path, but I can't say for sure and everything appear to work for
cpsw if you comment out that SET_NETDEV_DEV (well, at least before this
patch).

> I don't mind safeguarding this with a check against dev->dev.parent, but
> I would like to fix the drivers where relevant too, since
> SET_NETDEV_DEV() should really be called, otherwise a number of things
> just don't work

I grepped for for register_netdev and think I saw a number of drivers
which do not call SET_NETDEV_DEV.

Again, perhaps they will never hit this path, but thought I should ask.

Johan

Re: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Florian Fainelli]

On 12/08/2016 09:01 AM, Johan Hovold wrote:
> On Thu, Dec 08, 2016 at 08:47:54AM -0800, Florian Fainelli wrote:
>> On 12/08/2016 08:27 AM, Johan Hovold wrote:
>>> On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
>>>> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
>>>> dealt with MDIO bus module reference count, but sort of introduced a
>>>> regression in that, if an Ethernet driver registers its own MDIO bus
>>>> driver, as is common, we will end up with the Ethernet driver's
>>>> module->refnct set to 1, thus preventing this driver from any removal.
>>>>
>>>> Fix this by comparing the network device's device driver owner against
>>>> the MDIO bus driver owner, and only if they are different, increment the
>>>> MDIO bus module refcount.
>>>>
>>>> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>> Russell,
>>>>
>>>> I verified this against the ethoc driver primarily (on a TS7300 board)
>>>> and bcmgenet.
>>>>
>>>> Thanks!
>>>>
>>>>  drivers/net/phy/phy_device.c | 16 +++++++++++++---
>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>>> index 1a4bf8acad78..c4ceb082e970 100644
>>>> --- a/drivers/net/phy/phy_device.c
>>>> +++ b/drivers/net/phy/phy_device.c
>>>> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
>>>>  int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>>>>  		      u32 flags, phy_interface_t interface)
>>>>  {
>>>> +	struct module *ndev_owner = dev->dev.parent->driver->owner;
>>>
>>> Is this really safe? A driver does not need to set a parent device, and
>>> in that case you get a NULL-deref here (I tried using cpsw).
>>
>> Humm, cpsw does call SET_NETDEV_DEV() which should take care of that, is
>> the call made too late? Do you have an example oops?
> 
> Sorry if I was being unclear, cpsw does set a parent device, but there
> are network driver that do not. Perhaps such drivers will never hit this
> code path, but I can't say for sure and everything appear to work for
> cpsw if you comment out that SET_NETDEV_DEV (well, at least before this
> patch).

You were clear, I did not understand that you exercised this with cpsw
to see whether this was safe in all conditions.

> 
>> I don't mind safeguarding this with a check against dev->dev.parent, but
>> I would like to fix the drivers where relevant too, since
>> SET_NETDEV_DEV() should really be called, otherwise a number of things
>> just don't work
> 
> I grepped for for register_netdev and think I saw a number of drivers
> which do not call SET_NETDEV_DEV.
> 
> Again, perhaps they will never hit this path, but thought I should ask.

You are absolutely right, this is a potential problem, so far I found
two legitimate drivers that do not call SET_NETDEV_DEV (lantiq_etop.c
and cpmac.c, both fixed), and Freescale's FMAN driver, which I have a
hard time understanding what it does with mac_dev->net_dev...

Thanks!
-- 
Florian

RE: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Madalin-Cristian Bucur]

> From: netdev-owner@vger.kernel.org On Behalf Of Florian Fainelli
> Sent: Thursday, December 08, 2016 7:54 PM
> To: Johan Hovold <johan@kernel.org>
> On 12/08/2016 09:01 AM, Johan Hovold wrote:
> > On Thu, Dec 08, 2016 at 08:47:54AM -0800, Florian Fainelli wrote:
> >> On 12/08/2016 08:27 AM, Johan Hovold wrote:
> >>> On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
> >>>> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way
> we
> >>>> dealt with MDIO bus module reference count, but sort of introduced a
> >>>> regression in that, if an Ethernet driver registers its own MDIO bus
> >>>> driver, as is common, we will end up with the Ethernet driver's
> >>>> module->refnct set to 1, thus preventing this driver from any
> removal.
> >>>>
> >>>> Fix this by comparing the network device's device driver owner
> against
> >>>> the MDIO bus driver owner, and only if they are different, increment
> the
> >>>> MDIO bus module refcount.
> >>>>
> >>>> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >>>> ---
> >>>> Russell,
> >>>>
> >>>> I verified this against the ethoc driver primarily (on a TS7300
> board)
> >>>> and bcmgenet.
> >>>>
> >>>> Thanks!
> >>>>
> >>>>  drivers/net/phy/phy_device.c | 16 +++++++++++++---
> >>>>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/phy/phy_device.c
> b/drivers/net/phy/phy_device.c
> >>>> index 1a4bf8acad78..c4ceb082e970 100644
> >>>> --- a/drivers/net/phy/phy_device.c
> >>>> +++ b/drivers/net/phy/phy_device.c
> >>>> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
> >>>>  int phy_attach_direct(struct net_device *dev, struct phy_device
> *phydev,
> >>>>  		      u32 flags, phy_interface_t interface)
> >>>>  {
> >>>> +	struct module *ndev_owner = dev->dev.parent->driver->owner;
> >>>
> >>> Is this really safe? A driver does not need to set a parent device,
> and
> >>> in that case you get a NULL-deref here (I tried using cpsw).
> >>
> >> Humm, cpsw does call SET_NETDEV_DEV() which should take care of that,
> is
> >> the call made too late? Do you have an example oops?
> >
> > Sorry if I was being unclear, cpsw does set a parent device, but there
> > are network driver that do not. Perhaps such drivers will never hit this
> > code path, but I can't say for sure and everything appear to work for
> > cpsw if you comment out that SET_NETDEV_DEV (well, at least before this
> > patch).
> 
> You were clear, I did not understand that you exercised this with cpsw
> to see whether this was safe in all conditions.
> 
> >
> >> I don't mind safeguarding this with a check against dev->dev.parent,
> but
> >> I would like to fix the drivers where relevant too, since
> >> SET_NETDEV_DEV() should really be called, otherwise a number of things
> >> just don't work
> >
> > I grepped for for register_netdev and think I saw a number of drivers
> > which do not call SET_NETDEV_DEV.
> >
> > Again, perhaps they will never hit this path, but thought I should ask.
> 
> You are absolutely right, this is a potential problem, so far I found
> two legitimate drivers that do not call SET_NETDEV_DEV (lantiq_etop.c
> and cpmac.c, both fixed), and Freescale's FMAN driver, which I have a
> hard time understanding what it does with mac_dev->net_dev...
> 
> Thanks!
> --
> Florian

Hi Florian,

The Freescale DPAA Ethernet driver is in drivers/net/ethernet/freescale/dpaa:

drivers/net/ethernet/freescale/dpaa/dpaa_eth.c:2501:	SET_NETDEV_DEV(net_dev, dev);

and it is making use of the MAC and ports driver in the FMan driver (and of the
QBMan drivers in drivers/soc/fsl/qbman but that's off topic). You need to look
at the net-next tree for this, the drivers were gradually added.

Madalin