2022-11-15 05:23:46

by Jakub Kicinski

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

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;

2022-11-15 16:46:53

by Andrew Lunn

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

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

2022-11-16 23:07:38

by Saeed Mahameed

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

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 ?