netdev - Re: [PATCH v1 3/3] net/ethtool/ioctl: correct & simplify ethtool_get_phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <Y4FO65aWvYu8Y6U7@lunn.ch>
Date:   Sat, 26 Nov 2022 00:25:31 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Daniil Tatianin <d-tatianin@...dex-team.ru>
Cc:     netdev@...r.kernel.org, Michal Kubecek <mkubecek@...e.cz>,
        yc-core@...dex-team.ru, lvc-project@...uxtesting.org
Subject: Re: [PATCH v1 3/3] net/ethtool/ioctl: correct & simplify
 ethtool_get_phy_stats if checks

> I did experiment with that for a bit at the start, couldn't come up with a
> clear solution right away so went with this instead.

How about this? The patch is a lot less readable than the end code.
Compile tested only.

	Andrew

--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2077,58 +2077,88 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
        return ret;
 }
 
-static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
+static int ethtool_get_phy_stats_phydev(struct phy_device *phydev,
+                                       struct ethtool_stats *stats,
+                                       u64 **data)
 {
        const struct ethtool_phy_ops *phy_ops = ethtool_phy_ops;
+       int n_stats;
+
+       if (!phy_ops || !phy_ops->get_sset_count || !phy_ops->get_stats)
+               return -EOPNOTSUPP;
+
+       n_stats = phy_ops->get_sset_count(phydev);
+       if (n_stats < 0)
+               return n_stats;
+       if (n_stats > S32_MAX / sizeof(u64))
+               return -ENOMEM;
+       if (WARN_ON_ONCE(!n_stats))
+               return -EOPNOTSUPP;
+
+       stats->n_stats = n_stats;
+
+       *data = vzalloc(array_size(n_stats, sizeof(u64)));
+       if (!*data)
+               return -ENOMEM;
+
+       return phy_ops->get_stats(phydev, stats, *data);
+}
+
+static int ethtool_get_phy_stats_ethtool(struct net_device *dev,
+                                        struct ethtool_stats *stats,
+                                        u64 **data)
+{
        const struct ethtool_ops *ops = dev->ethtool_ops;
-       struct phy_device *phydev = dev->phydev;
-       struct ethtool_stats stats;
-       u64 *data;
-       int ret, n_stats;
+       int n_stats;
 
-       if (!phydev && (!ops->get_ethtool_phy_stats || !ops->get_sset_count))
+       if (!ops || !ops->get_sset_count || ops->get_ethtool_phy_stats)
                return -EOPNOTSUPP;
 
-       if (phydev && !ops->get_ethtool_phy_stats &&
-           phy_ops && phy_ops->get_sset_count)
-               n_stats = phy_ops->get_sset_count(phydev);
-       else
-               n_stats = ops->get_sset_count(dev, ETH_SS_PHY_STATS);
+       n_stats = ops->get_sset_count(dev, ETH_SS_PHY_STATS);
        if (n_stats < 0)
                return n_stats;
        if (n_stats > S32_MAX / sizeof(u64))
                return -ENOMEM;
-       WARN_ON_ONCE(!n_stats);
+       if (WARN_ON_ONCE(!n_stats))
+               return -EOPNOTSUPP;
+
+       stats->n_stats = n_stats;
+
+       *data = vzalloc(array_size(n_stats, sizeof(u64)));
+       if (!*data)
+               return -ENOMEM;
+
+       ops->get_ethtool_phy_stats(dev, stats, *data);
+
+       return 0;
+}
+
+static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
+{
+       struct phy_device *phydev = dev->phydev;
+       struct ethtool_stats stats;
+       u64 *data;
+       int ret;
 
        if (copy_from_user(&stats, useraddr, sizeof(stats)))
                return -EFAULT;
 
-       stats.n_stats = n_stats;
-
-       if (n_stats) {
-               data = vzalloc(array_size(n_stats, sizeof(u64)));
-               if (!data)
-                       return -ENOMEM;
+       if (phydev)
+               ret = ethtool_get_phy_stats_phydev(phydev, &stats, &data);
+       else
+               ret = ethtool_get_phy_stats_ethtool(dev, &stats, &data);
+       if (ret)
+               return ret;
 
-               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);
-               }
-       } else {
-               data = NULL;
+       if (copy_to_user(useraddr, &stats, sizeof(stats))) {
+               ret = -EFAULT;
+               goto out;
        }
 
-       ret = -EFAULT;
-       if (copy_to_user(useraddr, &stats, sizeof(stats)))
-               goto out;
        useraddr += sizeof(stats);
-       if (n_stats && copy_to_user(useraddr, data, array_size(n_stats, sizeof(u64))))
-               goto out;
-       ret = 0;
+       if (copy_to_user(useraddr, data,
+                        array_size(stats.n_stats, sizeof(u64))))
+               ret = -EFAULT;
 
  out:
        vfree(data);