From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753057AbdDJXkp (ORCPT ); Mon, 10 Apr 2017 19:40:45 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35717 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751564AbdDJXko (ORCPT ); Mon, 10 Apr 2017 19:40:44 -0400 Subject: Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested To: Grygorii Strashko , "David S. Miller" , netdev@vger.kernel.org, Andrew Lunn References: <20170410233324.1178-1-grygorii.strashko@ti.com> Cc: Sekhar Nori , linux-kernel@vger.kernel.org From: Florian Fainelli Message-ID: Date: Mon, 10 Apr 2017 16:40:37 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170410233324.1178-1-grygorii.strashko@ti.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/10/2017 04:33 PM, Grygorii Strashko wrote: > Now the command: > ethtool --phy-statistics eth0 > will cause system crash with meassage "Unable to handle kernel NULL pointer > dereference at virtual address 00000010" from: > > (kszphy_get_stats) from [] (ethtool_get_phy_stats+0xd8/0x210) > (ethtool_get_phy_stats) from [] (dev_ethtool+0x5b8/0x228c) > (dev_ethtool) from [] (dev_ioctl+0x3fc/0x964) > (dev_ioctl) from [] (sock_ioctl+0x170/0x2c0) > (sock_ioctl) from [] (do_vfs_ioctl+0xa8/0x95c) > (do_vfs_ioctl) from [] (SyS_ioctl+0x3c/0x64) > (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x44) > > The reason: phy_driver structure for KSZ9031 phy has no .probe() callback > defined. As result, struct phy_device *phydev->priv pointer will not be > initializes (null). This is a strange way to fix the problem, presumably this PHY supports fetching statistics, if that is the case it sounds like we would want to sort of provide two probe function: - one which is just allocating the PHY device's private structure so we have enough room for statistics - another one which is doing all the reference clock fetching and so on By adding a NULL pointer check here, you'd be better off just removing all the function pointers pertaining to ethtool statistics. > > Fix it by adding additional checks for !phydev->priv in > kszphy_get_stats(), kszphy_get_strings() and kszphy_get_sset_count() > > Fixes: 2b2427d06426 ("phy: micrel: Add ethtool statistics counters") > Signed-off-by: Grygorii Strashko > --- > drivers/net/phy/micrel.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 6742070..8dbc1be 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -574,7 +574,6 @@ static int ksz9031_config_init(struct phy_device *phydev) > MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4, > tx_data_skews, 4); > } > - > return ksz9031_center_flp_timing(phydev); > } > > @@ -654,6 +653,9 @@ ksz9021_wr_mmd_phyreg(struct phy_device *phydev, int ptrad, int devnum, > > static int kszphy_get_sset_count(struct phy_device *phydev) > { > + if (!phydev->priv) > + return -EOPNOTSUPP; > + > return ARRAY_SIZE(kszphy_hw_stats); > } > > @@ -661,6 +663,9 @@ static void kszphy_get_strings(struct phy_device *phydev, u8 *data) > { > int i; > > + if (!phydev->priv) > + return; > + > for (i = 0; i < ARRAY_SIZE(kszphy_hw_stats); i++) { > memcpy(data + i * ETH_GSTRING_LEN, > kszphy_hw_stats[i].string, ETH_GSTRING_LEN); > @@ -694,6 +699,9 @@ static void kszphy_get_stats(struct phy_device *phydev, > { > int i; > > + if (!phydev->priv) > + return; > + > for (i = 0; i < ARRAY_SIZE(kszphy_hw_stats); i++) > data[i] = kszphy_get_stat(phydev, i); > } > -- Florian