Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1620779 > unrolled thread
| Started by | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| First post | 2017-04-11 01:40 +0200 |
| Last post | 2017-04-11 19:20 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested Grygorii Strashko <grygorii.strashko@ti.com> - 2017-04-11 01:40 +0200
Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested Florian Fainelli <f.fainelli@gmail.com> - 2017-04-11 01:50 +0200
Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested Grygorii Strashko <grygorii.strashko@ti.com> - 2017-04-11 18:20 +0200
Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested Andrew Lunn <andrew@lunn.ch> - 2017-04-11 18:40 +0200
Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested Grygorii Strashko <grygorii.strashko@ti.com> - 2017-04-11 19:20 +0200
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2017-04-11 01:40 +0200 |
| Subject | [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested |
| Message-ID | <tuR4C-7xq-13@gated-at.bofh.it> |
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 [<c069f1d8>] (ethtool_get_phy_stats+0xd8/0x210)
(ethtool_get_phy_stats) from [<c06a0738>] (dev_ethtool+0x5b8/0x228c)
(dev_ethtool) from [<c06b5484>] (dev_ioctl+0x3fc/0x964)
(dev_ioctl) from [<c0679f7c>] (sock_ioctl+0x170/0x2c0)
(sock_ioctl) from [<c02419d4>] (do_vfs_ioctl+0xa8/0x95c)
(do_vfs_ioctl) from [<c02422c4>] (SyS_ioctl+0x3c/0x64)
(SyS_ioctl) from [<c0107d60>] (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).
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 <grygorii.strashko@ti.com>
---
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);
}
--
2.10.1
[toc] | [next] | [standalone]
| From | Florian Fainelli <f.fainelli@gmail.com> |
|---|---|
| Date | 2017-04-11 01:50 +0200 |
| Subject | Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested |
| Message-ID | <tuRei-7C4-5@gated-at.bofh.it> |
| In reply to | #1620779 |
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 [<c069f1d8>] (ethtool_get_phy_stats+0xd8/0x210)
> (ethtool_get_phy_stats) from [<c06a0738>] (dev_ethtool+0x5b8/0x228c)
> (dev_ethtool) from [<c06b5484>] (dev_ioctl+0x3fc/0x964)
> (dev_ioctl) from [<c0679f7c>] (sock_ioctl+0x170/0x2c0)
> (sock_ioctl) from [<c02419d4>] (do_vfs_ioctl+0xa8/0x95c)
> (do_vfs_ioctl) from [<c02422c4>] (SyS_ioctl+0x3c/0x64)
> (SyS_ioctl) from [<c0107d60>] (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 <grygorii.strashko@ti.com>
> ---
> 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
[toc] | [prev] | [next] | [standalone]
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2017-04-11 18:20 +0200 |
| Subject | Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested |
| Message-ID | <tv6Gm-QX-29@gated-at.bofh.it> |
| In reply to | #1620782 |
On 04/10/2017 06:40 PM, Florian Fainelli wrote:
> 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 [<c069f1d8>] (ethtool_get_phy_stats+0xd8/0x210)
>> (ethtool_get_phy_stats) from [<c06a0738>] (dev_ethtool+0x5b8/0x228c)
>> (dev_ethtool) from [<c06b5484>] (dev_ioctl+0x3fc/0x964)
>> (dev_ioctl) from [<c0679f7c>] (sock_ioctl+0x170/0x2c0)
>> (sock_ioctl) from [<c02419d4>] (do_vfs_ioctl+0xa8/0x95c)
>> (do_vfs_ioctl) from [<c02422c4>] (SyS_ioctl+0x3c/0x64)
>> (SyS_ioctl) from [<c0107d60>] (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.
I've considered all this option, honestly, but I do not know if this
phys (issue should affect KSZ886X, KSZ8873MLL, KSZ9031, KSZ9021, KSZ8061, KS8737)
support fetching statistics or not and was hoping to get feedback from community.
Simples way for me was to block statistic callbacks for them,
but I, of course, can just remove those callback for above phys if this is acceptable.
>
>>
>> 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 <grygorii.strashko@ti.com>
>> ---
>> 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);
>> }
>>
>
>
--
regards,
-grygorii
[toc] | [prev] | [next] | [standalone]
| From | Andrew Lunn <andrew@lunn.ch> |
|---|---|
| Date | 2017-04-11 18:40 +0200 |
| Subject | Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested |
| Message-ID | <tv6ZJ-Y3-27@gated-at.bofh.it> |
| In reply to | #1621450 |
> I've considered all this option, honestly, but I do not know if this
> phys (issue should affect KSZ886X, KSZ8873MLL, KSZ9031, KSZ9021, KSZ8061, KS8737)
> support fetching statistics or not and was hoping to get feedback from community.
Hi Grygorii
Microchip are one of the good guys. All there datesheets are open. So
you should be able to check this.
Andrew
[toc] | [prev] | [next] | [standalone]
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2017-04-11 19:20 +0200 |
| Subject | Re: [PATCH] net: phy: micrel: KSZ9031: fix crash when statistic requested |
| Message-ID | <tv7Cp-1vq-5@gated-at.bofh.it> |
| In reply to | #1621463 |
On 04/11/2017 11:36 AM, Andrew Lunn wrote: >> I've considered all this option, honestly, but I do not know if this >> phys (issue should affect KSZ886X, KSZ8873MLL, KSZ9031, KSZ9021, KSZ8061, KS8737) >> support fetching statistics or not and was hoping to get feedback from community. > > Hi Grygorii > > Microchip are one of the good guys. All there datesheets are open. So > you should be able to check this. Below is what i've found 0xa 0x15 action KS8737 - + remove stats? - reg 10(0xa) reserved KSZ8061MNX/MNG - + remove stats? - reg 10(0xa) reserved KSZ9021GN + + add .probe() KSZ9031 + + add .probe() KSZ8873MLL/FLL/RLL - - remove stats KSZ886X/KSZ8862 - - remove stats The kszphy_probe() can be reused as .probe() callback as it seems not phy specific. -- regards, -grygorii
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web