[lvc-project] [PATCH v1 3/3] net/ethtool/ioctl: correct & simplify ethtool_get_phy_stats if checks

[Daniil Tatianin]

On 11/26/22 2:25 AM, Andrew Lunn wrote:
>> 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);
I guess this does indeed look cleaner although I'm not sure about the 
duplicated n_stats validation code. Maybe this could be moved to a 
separate helper as well or do you think it's okay to duplicate it in 
this case?

Thank you