Re: [PATCH v1 1/3] net/ethtool/ioctl: return -EOPNOTSUPP if we have no phy stats

Re: [PATCH v1 1/3] net/ethtool/ioctl: return -EOPNOTSUPP if we have no phy stats

[Andrew Lunn]

On Fri, Nov 25, 2022 at 07:49:11PM +0300, Daniil Tatianin wrote:
> It's not very useful to copy back an empty ethtool_stats struct and
> return 0 if we didn't actually have any stats. This also allows for
> further simplification of this function in the future commits.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  net/ethtool/ioctl.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 57e7238a4136..071e9bf16007 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2089,11 +2089,15 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
>  		n_stats = phy_ops->get_sset_count(phydev);
>  	else
>  		n_stats = ops->get_sset_count(dev, ETH_SS_PHY_STATS);
> +
>  	if (n_stats < 0)

Please don't make unneeded white space changes. It just distracts from
the real change being made here.

>  		return n_stats;
>  	if (n_stats > S32_MAX / sizeof(u64))
>  		return -ENOMEM;
> -	WARN_ON_ONCE(!n_stats);
> +	if (!n_stats) {
> +		WARN_ON_ONCE(1);
> +		return -EOPNOTSUPP;
> +	}

WARN_ON_ONCE() returns the result of the comparison being made. so you can do:

	if (WARN_ON_ONCE(!n_stats))
		return -EOPNOTSUPP;

	Andrew		

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

[Andrew Lunn]

On Fri, Nov 25, 2022 at 07:49:13PM +0300, Daniil Tatianin wrote:
> ops->get_ethtool_phy_stats was getting called in an else branch
> of ethtool_get_phy_stats() unconditionally without making sure
> it was actually present.
> 
> Refactor the if checks so that it's more obvious what's going on and
> avoid accidental NULL derefs.
> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE
> static analysis tool.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  net/ethtool/ioctl.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index f83118c68e20..2b01e0042e6e 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2076,25 +2076,27 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
>  {
>  	const struct ethtool_phy_ops *phy_ops = ethtool_phy_ops;
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	bool has_phy_stats_ops = ops->get_ethtool_phy_stats != NULL;
>  	struct phy_device *phydev = dev->phydev;
>  	struct ethtool_stats stats;
>  	u64 *data;
>  	int ret, n_stats;
>  
> -	if (!phydev && (!ops->get_ethtool_phy_stats || !ops->get_sset_count))
> -		return -EOPNOTSUPP;
> +	if (!phydev || !phy_ops) {
> +		if (!ops->get_sset_count)
> +			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);
> +	} else {
> +		n_stats = phy_ops->get_sset_count(phydev);
> +		has_phy_stats_ops |= phy_ops->get_stats != NULL;

I'm not sure this is actually any clearer. You are mixing together
ethtool ops and phy ops.

This is part of why i suggested splitting phydev and !phydev into
helpers. The tests become a lot simpler. Please try it and see what
the resulting code looks like.

    Andrew

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

[Andrew Lunn]

> 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);

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

[Andrew Lunn]

> 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?

Yes, the validation and the memory allocation could be pulled out into
a helper.

The main thing here is that the code is pretty much identical for the
two functions. If one is correct, the other is also correct. There is
no access in an else branch which can be overlooked.

  Andrew