| Message ID | c02d4d86-6e65-4270-bc46-70acb6eb2d4a@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ethtool: check for unsupported modes in EEE advertisement | expand |
> + > + 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
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?
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? >
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? >> >
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
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
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
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)
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(+)