Re: [PATCH v1] net/ethtool/ioctl: ensure that we have phy ops before using them

Re: [PATCH v1] net/ethtool/ioctl: ensure that we have phy ops before using them

[Jakub Kicinski]

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;

Re: [PATCH v1] net/ethtool/ioctl: ensure that we have phy ops before using them

[Andrew Lunn]

On Mon, Nov 14, 2022 at 09:07:05PM -0800, Jakub Kicinski wrote:
> 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.

Hi Daniil

I'm missing the patch itself, and b4 does not return it. Please
consider reposting. Since this appear to be to do with PHY statistics,
you should Cc: the PHY maintainers.

       Andrew

Re: [PATCH v1] net/ethtool/ioctl: ensure that we have phy ops before using them

[Saeed Mahameed]

On 15 Nov 16:40, Andrew Lunn wrote:
>On Mon, Nov 14, 2022 at 09:07:05PM -0800, Jakub Kicinski wrote:
>> 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.
>
>Hi Daniil
>
>I'm missing the patch itself, and b4 does not return it. Please

same! I only see Jakub's reply, maybe the patch didn't make it through to
netdev ML ? 

Re: [PATCH v1] net/ethtool/ioctl: ensure that we have phy ops before using them

[Andrew Lunn]

> Strange, pretty sure it CCed the netdev ML. Unless there's something else
> you must do for it to get through?

HTML might get it rejected. But so long as you used git send-email, it
normally works.

Anyway, please resend.

	Andrew