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

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Nov 22, 2022 at 10:21:43AM +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 checks to avoid unnecessary nesting and make them more
> readable. Add an extra WARN_ON_ONCE(1) to emit a warning when a driver
> declares that it has phy stats without a way to retrieve them.

So i have two different suggestions here, take your pick and even
merge them together.

I wonder if we can simply this some more. If there are 0 stats we
already issue a WARN_ON_ONCE():

https://elixir.bootlin.com/linux/v6.1-rc6/source/net/ethtool/ioctl.c#L2096

We will then copy back to user space the ethtool_stats and zero
statistics and return 0. If that useful? Would it make more sense to
just return -EOPNOTSUPP after the WARN_ON_ONCE()?

That would be patch 1/X.

Patch 2/X would then remove the if (n_stats) code, but otherwise make
no changes. That keeps the patch simple to review.

Patch 3/X would then add the additional verification of
ops->get_ethtool_phy_stats(). But do it at the top of the function,
along with all the other verification, and return -EOPNOTSUPP.

Alternatively, given the complexity of the checking and the function
as a whole, i'm wondering if it make sense to actually pull this
function apart. Add a ethtool_get_phy_stats_phydev() and
ethtool_get_phy_stats_ethtool(), and have ethtool_get_phy_stats() do
the copy_from_user(), call one of the two helpers, and if they don't
return an error do the two copy_to_user().

       Andrew