[PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Heiner Kallweit]

Let the core check whether userspace returned unsupported modes in the
EEE advertisement bitmap. This allows to remove these checks from
drivers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 net/ethtool/eee.c   | 12 ++++++++++++
 net/ethtool/ioctl.c |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index db6faa18f..9596cf888 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <linux/linkmode.h>
+
 #include "netlink.h"
 #include "common.h"
 #include "bitset.h"
@@ -145,6 +147,7 @@ static int
 ethnl_set_eee(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct net_device *dev = req_info->dev;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
 	struct nlattr **tb = info->attrs;
 	struct ethtool_keee eee = {};
 	bool mod = false;
@@ -166,6 +169,15 @@ ethnl_set_eee(struct ethnl_req_info *req_info, struct genl_info *info)
 	}
 	if (ret < 0)
 		return ret;
+
+	if (ethtool_eee_use_linkmodes(&eee)) {
+		if (linkmode_andnot(tmp, eee.advertised, eee.supported))
+			return -EINVAL;
+	} else {
+		if (eee.advertised_u32 & ~eee.supported_u32)
+			return -EINVAL;
+	}
+
 	ethnl_update_bool(&eee.eee_enabled, tb[ETHTOOL_A_EEE_ENABLED], &mod);
 	ethnl_update_bool(&eee.tx_lpi_enabled, tb[ETHTOOL_A_EEE_TX_LPI_ENABLED],
 			  &mod);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 1763e8b69..622a2d4fc 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1590,6 +1590,9 @@ static int ethtool_set_eee(struct net_device *dev, char __user *useraddr)
 	if (copy_from_user(&eee, useraddr, sizeof(eee)))
 		return -EFAULT;
 
+	if (eee.advertised & ~eee.supported)
+		return -EINVAL;
+
 	eee_to_keee(&keee, &eee);
 	ret = dev->ethtool_ops->set_eee(dev, &keee);
 	if (!ret)
-- 
2.43.1

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Andrew Lunn]

> +
> +	if (ethtool_eee_use_linkmodes(&eee)) {
> +		if (linkmode_andnot(tmp, eee.advertised, eee.supported))
> +			return -EINVAL;
> +	} else {
> +		if (eee.advertised_u32 & ~eee.supported_u32)
> +			return -EINVAL;
> +	}

Do we have the necessary parameters to be able to use an extack and
give user space a useful error message, more than EINVAL?

     Andrew

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Russell King (Oracle)]

On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
> Let the core check whether userspace returned unsupported modes in the
> EEE advertisement bitmap. This allows to remove these checks from
> drivers.

Why is this a good thing to implement?

Concerns:
1) This is a change of behaviour for those drivers that do not
implement this behaviour.

2) This behaviour is different from ksettings_set() which silently
trims the advertisement down to the modes that are supported

3) This check is broken. Userspace is at liberty to pass in ~0 for
the supported mask and the advertising mask which subverts this
check.

So... I think overall, it's a NAK to this from me - I don't think
it's something that anyone should implement. Restricting the
advertisement to the modes that are supported (where the supported
mask is pulled from the network driver and not userspace) would
be acceptable, but is that actually necessary?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Heiner Kallweit]

On 15.02.2024 16:53, Russell King (Oracle) wrote:
> On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
>> Let the core check whether userspace returned unsupported modes in the
>> EEE advertisement bitmap. This allows to remove these checks from
>> drivers.
> 
> Why is this a good thing to implement?
> 
Because it allows to remove all the duplicated checks from drivers.

> Concerns:
> 1) This is a change of behaviour for those drivers that do not
> implement this behaviour.
> 
Not of regular behavior. And at least for all drivers using phylib
it's no change.

> 2) This behaviour is different from ksettings_set() which silently
> trims the advertisement down to the modes that are supported
> 
It's the same check that we have in genphy_c45_ethtool_set_eee().
So it's in line with what we do in phylib.
But I would also be fine with silently trimming the advertisement.

> 3) This check is broken. Userspace is at liberty to pass in ~0 for
> the supported mask and the advertising mask which subverts this
> check.
> 
ethtool retrieves the supported mask with get_eee() from kernel.
And this (unmodified) value is passed with set_eee().
So at least with ethtool this scenario can't occur.

> So... I think overall, it's a NAK to this from me - I don't think
> it's something that anyone should implement. Restricting the
> advertisement to the modes that are supported (where the supported
> mask is pulled from the network driver and not userspace) would
> be acceptable, but is that actually necessary?
> 

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Heiner Kallweit]

On 15.02.2024 21:27, Heiner Kallweit wrote:
> On 15.02.2024 16:53, Russell King (Oracle) wrote:
>> On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
>>> Let the core check whether userspace returned unsupported modes in the
>>> EEE advertisement bitmap. This allows to remove these checks from
>>> drivers.
>>
>> Why is this a good thing to implement?
>>
> Because it allows to remove all the duplicated checks from drivers.
> 
>> Concerns:
>> 1) This is a change of behaviour for those drivers that do not
>> implement this behaviour.
>>
> Not of regular behavior. And at least for all drivers using phylib
> it's no change.
> 
>> 2) This behaviour is different from ksettings_set() which silently
>> trims the advertisement down to the modes that are supported
>>
> It's the same check that we have in genphy_c45_ethtool_set_eee().
> So it's in line with what we do in phylib.
> But I would also be fine with silently trimming the advertisement.
> 
>> 3) This check is broken. Userspace is at liberty to pass in ~0 for
>> the supported mask and the advertising mask which subverts this
>> check.
>>
> ethtool retrieves the supported mask with get_eee() from kernel.
> And this (unmodified) value is passed with set_eee().
> So at least with ethtool this scenario can't occur.
> 
In addition: In the netlink case the supported mask isn't even
transferred to userspace for the set_eee operation.

>> So... I think overall, it's a NAK to this from me - I don't think
>> it's something that anyone should implement. Restricting the
>> advertisement to the modes that are supported (where the supported
>> mask is pulled from the network driver and not userspace) would
>> be acceptable, but is that actually necessary?
>>
> 

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Heiner Kallweit]

On 15.02.2024 15:44, Andrew Lunn wrote:
>> +
>> +	if (ethtool_eee_use_linkmodes(&eee)) {
>> +		if (linkmode_andnot(tmp, eee.advertised, eee.supported))
>> +			return -EINVAL;
>> +	} else {
>> +		if (eee.advertised_u32 & ~eee.supported_u32)
>> +			return -EINVAL;
>> +	}
> 
> Do we have the necessary parameters to be able to use an extack and
> give user space a useful error message, more than EINVAL?
> 
We could at least print the same error message that we have in
genphy_c45_ethtool_set_eee():
GENL_SET_ERR_MSG("At least some EEE link modes are not supported.")

>      Andrew
Heiner

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Andrew Lunn]

On Thu, Feb 15, 2024 at 03:53:40PM +0000, Russell King (Oracle) wrote:
> On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
> > Let the core check whether userspace returned unsupported modes in the
> > EEE advertisement bitmap. This allows to remove these checks from
> > drivers.
> 
> Why is this a good thing to implement?
> 
> Concerns:
> 1) This is a change of behaviour for those drivers that do not
> implement this behaviour.

Hi Heiner

You say phylib does this by default? Are there any none phylib/phylink
drivers which don't implement this behaviour?

	Andrew

Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Heiner Kallweit]

On 16.02.2024 00:33, Andrew Lunn wrote:
> On Thu, Feb 15, 2024 at 03:53:40PM +0000, Russell King (Oracle) wrote:
>> On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
>>> Let the core check whether userspace returned unsupported modes in the
>>> EEE advertisement bitmap. This allows to remove these checks from
>>> drivers.
>>
>> Why is this a good thing to implement?
>>
>> Concerns:
>> 1) This is a change of behaviour for those drivers that do not
>> implement this behaviour.
> 
> Hi Heiner
> 
> You say phylib does this by default? Are there any none phylib/phylink
> drivers which don't implement this behaviour?
> 
Very few drivers ignore unknown/unsupported modes:
bnx2, r8152, ax88179, igc
Basically the ones using ethtool_adv_to_mmd_eee_adv_t() on the userspace-provided
EEE advertisement. However for e.g. Intel drivers the behavior isn't consistent,
some of their drivers check for unsupported modes and bail out if found.

> 	Andrew
> 
Heiner