Message ID | 1547207806-30572-1-git-send-email-camelia.groza@nxp.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: phy: guard against accessing a NULL features bitmap | expand |
On Fri, Jan 11, 2019 at 01:56:46PM +0200, Camelia Groza wrote: > Since phy driver features became a link_mode bitmap, phy drivers that > don't have a list of features configured will cause the kernel to crash > when probed. Hi Camelia A NULL features is a driver bug. So i would prefer to solve this differently. Please make phy_driver_register() do a WARN_ON(!new_driver->features) and return -EINVAL. Do you know of a specific driver which as a NULL value? We should fix that as well. Thank Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Friday, January 11, 2019 15:38 > To: Camelia Alexandra Groza <camelia.groza@nxp.com> > Cc: f.fainelli@gmail.com; hkallweit1@gmail.com; davem@davemloft.net; > oss@buserror.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net] net: phy: guard against accessing a NULL features > bitmap > > On Fri, Jan 11, 2019 at 01:56:46PM +0200, Camelia Groza wrote: > > Since phy driver features became a link_mode bitmap, phy drivers that > > don't have a list of features configured will cause the kernel to crash > > when probed. > > Hi Camelia > > A NULL features is a driver bug. So i would prefer to solve this > differently. > > Please make phy_driver_register() do a WARN_ON(!new_driver->features) > and return -EINVAL. I wasn't aware that features are mandatory. I'll make the change. > Do you know of a specific driver which as a NULL value? We should fix > that as well. Yes, there are five drivers that don't have features configured: BCM8706, BCM8727, CS4340, TN2020 and KSZ8873MLL. I'm planning to send patches on net-next for the Cortina and Teranetics ones at least, but fixing the generic crash was my priority. Thanks, Camelia
> > Hi Camelia > > > > A NULL features is a driver bug. So i would prefer to solve this > > differently. > > > > Please make phy_driver_register() do a WARN_ON(!new_driver->features) > > and return -EINVAL. > > I wasn't aware that features are mandatory. I'll make the change. It was not origionally, but really it should be now. > Yes, there are five drivers that don't have features configured: BCM8706, BCM8727, CS4340, TN2020 and KSZ8873MLL. > I'm planning to send patches on net-next for the Cortina and > Teranetics ones at least, but fixing the generic crash was my > priority. O.K, will take the others. Thanks Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 5199000..ddf04ab 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2154,7 +2154,8 @@ static int phy_probe(struct device *dev) * a controller will attach, and may modify one * or both of these values */ - linkmode_copy(phydev->supported, phydrv->features); + if (phydrv->features) + linkmode_copy(phydev->supported, phydrv->features); of_set_phy_supported(phydev); linkmode_copy(phydev->advertising, phydev->supported); @@ -2174,8 +2175,9 @@ static int phy_probe(struct device *dev) * (e.g. hardware erratum) where the driver wants to set only one * of these bits. */ - if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) || - test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) { + if (phydrv->features && + (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) || + test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features))) { linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
Since phy driver features became a link_mode bitmap, phy drivers that don't have a list of features configured will cause the kernel to crash when probed. Fixes: 719655a14971 ("net: phy: Replace phy driver features u32 with link_mode bitmap") Reported-by: Scott Wood <oss@buserror.net> Signed-off-by: Camelia Groza <camelia.groza@nxp.com> --- I'll submit a modified version of this patch to 4.20 stable once this one is accepted. --- drivers/net/phy/phy_device.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)