LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3] net: phy: Check phy_driver ready before accessing
@ 2018-06-07 15:53 Brandon Maier
  2018-06-07 15:53 ` [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus Brandon Maier
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Brandon Maier @ 2018-06-07 15:53 UTC (permalink / raw)
  To: netdev
  Cc: andrew, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel, Brandon Maier

Since a phy_device is added to the global mdio_bus list during
phy_device_register(), but a phy_device's phy_driver doesn't get
attached until phy_probe(). It's possible of_phy_find_device() in
xgmiitorgmii will return a valid phy with a NULL phy_driver. Leading to
a NULL pointer access during the memcpy().

Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 2e5150b0b8d5..04c8bec1c4c1 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -81,6 +81,11 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 		return -EPROBE_DEFER;
 	}
 
+	if (!priv->phy_dev->drv) {
+		dev_info(dev, "Attached phy not ready\n");
+		return -EPROBE_DEFER;
+	}
+
 	priv->addr = mdiodev->addr;
 	priv->phy_drv = priv->phy_dev->drv;
 	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
-- 
2.14.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus
  2018-06-07 15:53 [PATCH 1/3] net: phy: Check phy_driver ready before accessing Brandon Maier
@ 2018-06-07 15:53 ` Brandon Maier
  2018-06-07 16:49   ` Andrew Lunn
  2018-06-07 15:53 ` [PATCH 3/3] net: phy: xgmiitorgmii: Check read_status results Brandon Maier
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Brandon Maier @ 2018-06-07 15:53 UTC (permalink / raw)
  To: netdev
  Cc: andrew, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel, Brandon Maier

The xgmiitorgmii is using the mii_bus of the device it's attached too,
instead of the bus it was given during probe.

Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 04c8bec1c4c1..d6f8b64cddbe 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -33,17 +33,19 @@ struct gmii2rgmii {
 	struct phy_device *phy_dev;
 	struct phy_driver *phy_drv;
 	struct phy_driver conv_phy_drv;
-	int addr;
+	struct mdio_device *mdio;
 };
 
 static int xgmiitorgmii_read_status(struct phy_device *phydev)
 {
 	struct gmii2rgmii *priv = phydev->priv;
+	struct mii_bus *bus = priv->mdio->bus;
+	int addr = priv->mdio->addr;
 	u16 val = 0;
 
 	priv->phy_drv->read_status(phydev);
 
-	val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG);
+	val = mdiobus_read(bus, addr, XILINX_GMII2RGMII_REG);
 	val &= ~XILINX_GMII2RGMII_SPEED_MASK;
 
 	if (phydev->speed == SPEED_1000)
@@ -53,7 +55,7 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 	else
 		val |= BMCR_SPEED10;
 
-	mdiobus_write(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG, val);
+	mdiobus_write(bus, addr, XILINX_GMII2RGMII_REG, val);
 
 	return 0;
 }
@@ -86,7 +88,7 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 		return -EPROBE_DEFER;
 	}
 
-	priv->addr = mdiodev->addr;
+	priv->mdio = mdiodev;
 	priv->phy_drv = priv->phy_dev->drv;
 	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
 	       sizeof(struct phy_driver));
-- 
2.14.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] net: phy: xgmiitorgmii: Check read_status results
  2018-06-07 15:53 [PATCH 1/3] net: phy: Check phy_driver ready before accessing Brandon Maier
  2018-06-07 15:53 ` [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus Brandon Maier
@ 2018-06-07 15:53 ` Brandon Maier
  2018-06-07 16:43   ` Andrew Lunn
  2018-06-07 16:52 ` [PATCH 1/3] net: phy: Check phy_driver ready before accessing Andrew Lunn
  2018-06-07 16:54 ` Andrew Lunn
  3 siblings, 1 reply; 10+ messages in thread
From: Brandon Maier @ 2018-06-07 15:53 UTC (permalink / raw)
  To: netdev
  Cc: andrew, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel, Brandon Maier

We're ignoring the result of the attached phy device's read_status().
Return it so we can detect errors.

Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index d6f8b64cddbe..74a8782313cf 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -42,8 +42,11 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 	struct mii_bus *bus = priv->mdio->bus;
 	int addr = priv->mdio->addr;
 	u16 val = 0;
+	int err;
 
-	priv->phy_drv->read_status(phydev);
+	err = priv->phy_drv->read_status(phydev);
+	if (err < 0)
+		return err;
 
 	val = mdiobus_read(bus, addr, XILINX_GMII2RGMII_REG);
 	val &= ~XILINX_GMII2RGMII_SPEED_MASK;
-- 
2.14.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] net: phy: xgmiitorgmii: Check read_status results
  2018-06-07 15:53 ` [PATCH 3/3] net: phy: xgmiitorgmii: Check read_status results Brandon Maier
@ 2018-06-07 16:43   ` Andrew Lunn
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2018-06-07 16:43 UTC (permalink / raw)
  To: Brandon Maier
  Cc: netdev, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel

On Thu, Jun 07, 2018 at 10:53:48AM -0500, Brandon Maier wrote:
> We're ignoring the result of the attached phy device's read_status().
> Return it so we can detect errors.
> 
> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus
  2018-06-07 15:53 ` [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus Brandon Maier
@ 2018-06-07 16:49   ` Andrew Lunn
  2018-06-07 17:37     ` Brandon Maier
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2018-06-07 16:49 UTC (permalink / raw)
  To: Brandon Maier
  Cc: netdev, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel

On Thu, Jun 07, 2018 at 10:53:47AM -0500, Brandon Maier wrote:
> The xgmiitorgmii is using the mii_bus of the device it's attached too,
> instead of the bus it was given during probe.

Hi Brandon

I think the assumption was they are both on the same bus.
However, they don't need to be.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: phy: Check phy_driver ready before accessing
  2018-06-07 15:53 [PATCH 1/3] net: phy: Check phy_driver ready before accessing Brandon Maier
  2018-06-07 15:53 ` [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus Brandon Maier
  2018-06-07 15:53 ` [PATCH 3/3] net: phy: xgmiitorgmii: Check read_status results Brandon Maier
@ 2018-06-07 16:52 ` Andrew Lunn
  2018-06-07 17:34   ` Brandon Maier
  2018-06-07 16:54 ` Andrew Lunn
  3 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2018-06-07 16:52 UTC (permalink / raw)
  To: Brandon Maier
  Cc: netdev, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel

On Thu, Jun 07, 2018 at 10:53:46AM -0500, Brandon Maier wrote:
> Since a phy_device is added to the global mdio_bus list during
> phy_device_register(), but a phy_device's phy_driver doesn't get
> attached until phy_probe(). It's possible of_phy_find_device() in
> xgmiitorgmii will return a valid phy with a NULL phy_driver. Leading to
> a NULL pointer access during the memcpy().

I'm sure there are more issues like this in the code.  e.g. there is
no attempt made to hold a reference to the child phy. So it could be
unbound. priv->phy_drv->read_status(phydev) is then going to do bad
things.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: phy: Check phy_driver ready before accessing
  2018-06-07 15:53 [PATCH 1/3] net: phy: Check phy_driver ready before accessing Brandon Maier
                   ` (2 preceding siblings ...)
  2018-06-07 16:52 ` [PATCH 1/3] net: phy: Check phy_driver ready before accessing Andrew Lunn
@ 2018-06-07 16:54 ` Andrew Lunn
  3 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2018-06-07 16:54 UTC (permalink / raw)
  To: Brandon Maier
  Cc: netdev, f.fainelli, davem, michal.simek, clayton.shotwell,
	kristopher.cory, linux-kernel

On Thu, Jun 07, 2018 at 10:53:46AM -0500, Brandon Maier wrote:
> Since a phy_device is added to the global mdio_bus list during
> phy_device_register(), but a phy_device's phy_driver doesn't get
> attached until phy_probe(). It's possible of_phy_find_device() in
> xgmiitorgmii will return a valid phy with a NULL phy_driver. Leading to
> a NULL pointer access during the memcpy().

Hi Brandon

FYI: net-next is closed at the moment. Please resubmit these in two
weeks time.

      Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: phy: Check phy_driver ready before accessing
  2018-06-07 16:52 ` [PATCH 1/3] net: phy: Check phy_driver ready before accessing Andrew Lunn
@ 2018-06-07 17:34   ` Brandon Maier
  2018-06-07 18:12     ` Andrew Lunn
  0 siblings, 1 reply; 10+ messages in thread
From: Brandon Maier @ 2018-06-07 17:34 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, f.fainelli, davem, michal.simek, Clayton Shotwell,
	Kristopher Cory, linux-kernel

On Thu, Jun 7, 2018 at 11:52 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> FYI: net-next is closed at the moment. Please resubmit these in two
> weeks time.

Ah, I didn't see networking/netdev-FAQ.txt. I'll resubmit these then.

> I'm sure there are more issues like this in the code.  e.g. there is
> no attempt made to hold a reference to the child phy. So it could be
> unbound. priv->phy_drv->read_status(phydev) is then going to do bad
> things.
>

Agreed. Another thing that looks suspicious to me is the driver
overrides the private data of the device it's attaching too, in the
`priv->phy_dev->priv = priv` bit. Seems like that could cause all
sorts of driver corruption problems.

But fixing that is going to require more drastic changes to how this
driver works. So it'd be worth applying this patch in the mean time.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus
  2018-06-07 16:49   ` Andrew Lunn
@ 2018-06-07 17:37     ` Brandon Maier
  0 siblings, 0 replies; 10+ messages in thread
From: Brandon Maier @ 2018-06-07 17:37 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, f.fainelli, davem, michal.simek, Clayton Shotwell,
	Kristopher Cory, linux-kernel

On Thu, Jun 7, 2018 at 11:49 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> I think the assumption was they are both on the same bus.
> However, they don't need to be.

That was my assumption as well. We ran into a scenario where they were
on separate buses.

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Thanks

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: phy: Check phy_driver ready before accessing
  2018-06-07 17:34   ` Brandon Maier
@ 2018-06-07 18:12     ` Andrew Lunn
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2018-06-07 18:12 UTC (permalink / raw)
  To: Brandon Maier
  Cc: netdev, f.fainelli, davem, michal.simek, Clayton Shotwell,
	Kristopher Cory, linux-kernel

> Agreed. Another thing that looks suspicious to me is the driver
> overrides the private data of the device it's attaching too, in the
> `priv->phy_dev->priv = priv` bit. Seems like that could cause all
> sorts of driver corruption problems.

Ah, yes. That is very broken. Many PHYs will just explode sometime
later, since they use phdev->priv.

> But fixing that is going to require more drastic changes to how this
> driver works. So it'd be worth applying this patch in the mean time.

Patches welcome.

	Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-06-07 18:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07 15:53 [PATCH 1/3] net: phy: Check phy_driver ready before accessing Brandon Maier
2018-06-07 15:53 ` [PATCH 2/3] net: phy: xgmiitorgmii: Use correct mdio bus Brandon Maier
2018-06-07 16:49   ` Andrew Lunn
2018-06-07 17:37     ` Brandon Maier
2018-06-07 15:53 ` [PATCH 3/3] net: phy: xgmiitorgmii: Check read_status results Brandon Maier
2018-06-07 16:43   ` Andrew Lunn
2018-06-07 16:52 ` [PATCH 1/3] net: phy: Check phy_driver ready before accessing Andrew Lunn
2018-06-07 17:34   ` Brandon Maier
2018-06-07 18:12     ` Andrew Lunn
2018-06-07 16:54 ` Andrew Lunn

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).