Messages in this thread Patch in this message | | | Date | Mon, 14 Nov 2022 21:07:05 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH v1] net/ethtool/ioctl: ensure that we have phy ops before using them |
| |
On Mon, 14 Nov 2022 11:15:32 +0300 Daniil Tatianin wrote: > + if (!(phydev && phy_ops && phy_ops->get_stats) && > + !ops->get_ethtool_phy_stats)
This condition is still complicated.
> + return -EOPNOTSUPP;
The only way this crash can happen is if driver incorrectly returns non-zero stats count but doesn't have a callback to read the stats. So WARN_ON() would be in order here.
> if (!phydev && (!ops->get_ethtool_phy_stats || !ops->get_sset_count)) > return -EOPNOTSUPP; > > @@ -2063,13 +2066,12 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr) > if (!data) > return -ENOMEM; > > - if (dev->phydev && !ops->get_ethtool_phy_stats && > - phy_ops && phy_ops->get_stats) { > - ret = phy_ops->get_stats(dev->phydev, &stats, data); > + if (ops->get_ethtool_phy_stats) { > + ops->get_ethtool_phy_stats(dev, &stats, data); > + } else { > + ret = phy_ops->get_stats(phydev, &stats, data); > if (ret < 0) > goto out; > - } else { > - ops->get_ethtool_phy_stats(dev, &stats, data); > }
We can also clean up the pointless indentation of this code while at it.
How about something along these lines (completely untested, please review, test and make your own):
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 99272a67525c..ee04c388f4c9 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -2105,23 +2105,28 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr) stats.n_stats = n_stats; - if (n_stats) { - data = vzalloc(array_size(n_stats, sizeof(u64))); - if (!data) - return -ENOMEM; + if (!n_stats) { + data = NULL; + goto copy_back; + } - if (phydev && !ops->get_ethtool_phy_stats && - phy_ops && phy_ops->get_stats) { - ret = phy_ops->get_stats(phydev, &stats, data); - if (ret < 0) - goto out; - } else { - ops->get_ethtool_phy_stats(dev, &stats, data); - } + data = vzalloc(array_size(n_stats, sizeof(u64))); + if (!data) + return -ENOMEM; + + if (ops->get_ethtool_phy_stats) { + ops->get_ethtool_phy_stats(dev, &stats, data); + } else if (phydev && phy_ops && phy_ops->get_stats) { + ret = phy_ops->get_stats(phydev, &stats, data); + if (ret < 0) + goto out; } else { - data = NULL; + WARN_ON_ONCE(1); + n_stats = 0; + stats.n_stats = 0; } +copy_back: ret = -EFAULT; if (copy_to_user(useraddr, &stats, sizeof(stats))) goto out;
| |