| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMArcTWRb6uxQcgCg4=v1BkTjTGuOy-x_N8uyFt4m4TXzfQV3w@mail.gmail.com>
Date: Sun, 23 Feb 2025 21:26:20 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, dxu@...uu.xyz,
michael.chan@...adcom.com
Subject: Re: [PATCH net v2 1/2] net: ethtool: fix ioctl confusing drivers
about desired HDS user config
On Fri, Feb 21, 2025 at 11:51 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> The legacy ioctl path does not have support for extended attributes.
> So we issue a GET to fetch the current settings from the driver,
> in an attempt to keep them unchanged. HDS is a bit "special" as
> the GET only returns on/off while the SET takes a "ternary" argument
> (on/off/default). If the driver was in the "default" setting -
> executing the ioctl path binds it to on or off, even tho the user
> did not intend to change HDS config.
>
> Factor the relevant logic out of the netlink code and reuse it.
>
> Fixes: 87c8f8496a05 ("bnxt_en: add support for tcp-data-split ethtool command")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
Tested-by: Taehee Yoo <ap420073@...il.com>
> v2:
> - fix the core rather than the driver
> v1: https://lore.kernel.org/20250220005318.560733-1-kuba@kernel.org
>
> CC: michael.chan@...adcom.com
> CC: ap420073@...il.com
> ---
> net/ethtool/common.h | 6 ++++++
> net/ethtool/common.c | 16 ++++++++++++++++
> net/ethtool/ioctl.c | 4 ++--
> net/ethtool/rings.c | 9 ++++-----
> 4 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/net/ethtool/common.h b/net/ethtool/common.h
> index 58e9e7db06f9..a1088c2441d0 100644
> --- a/net/ethtool/common.h
> +++ b/net/ethtool/common.h
> @@ -51,6 +51,12 @@ int ethtool_check_max_channel(struct net_device *dev,
> struct ethtool_channels channels,
> struct genl_info *info);
> int ethtool_check_rss_ctx_busy(struct net_device *dev, u32 rss_context);
> +
> +void ethtool_ringparam_get_cfg(struct net_device *dev,
> + struct ethtool_ringparam *param,
> + struct kernel_ethtool_ringparam *kparam,
> + struct netlink_ext_ack *extack);
> +
> int __ethtool_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info);
> int ethtool_get_ts_info_by_phc(struct net_device *dev,
> struct kernel_ethtool_ts_info *info,
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index d88e9080643b..b97374b508f6 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -6,6 +6,7 @@
> #include <linux/rtnetlink.h>
> #include <linux/ptp_clock_kernel.h>
> #include <linux/phy_link_topology.h>
> +#include <net/netdev_queues.h>
>
> #include "netlink.h"
> #include "common.h"
> @@ -771,6 +772,21 @@ int ethtool_check_ops(const struct ethtool_ops *ops)
> return 0;
> }
>
> +void ethtool_ringparam_get_cfg(struct net_device *dev,
> + struct ethtool_ringparam *param,
> + struct kernel_ethtool_ringparam *kparam,
> + struct netlink_ext_ack *extack)
> +{
> + memset(param, 0, sizeof(*param));
> + memset(kparam, 0, sizeof(*kparam));
> +
> + param->cmd = ETHTOOL_GRINGPARAM;
> + dev->ethtool_ops->get_ringparam(dev, param, kparam, extack);
> +
> + /* Driver gives us current state, we want to return current config */
> + kparam->tcp_data_split = dev->cfg->hds_config;
> +}
> +
> static void ethtool_init_tsinfo(struct kernel_ethtool_ts_info *info)
> {
> memset(info, 0, sizeof(*info));
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 7609ce2b2c5e..1c3ba2247776 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2059,8 +2059,8 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
>
> static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
> {
> - struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM };
> struct kernel_ethtool_ringparam kernel_ringparam;
> + struct ethtool_ringparam ringparam, max;
> int ret;
>
> if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam)
> @@ -2069,7 +2069,7 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
> if (copy_from_user(&ringparam, useraddr, sizeof(ringparam)))
> return -EFAULT;
>
> - dev->ethtool_ops->get_ringparam(dev, &max, &kernel_ringparam, NULL);
> + ethtool_ringparam_get_cfg(dev, &max, &kernel_ringparam, NULL);
>
> /* ensure new ring parameters are within the maximums */
> if (ringparam.rx_pending > max.rx_max_pending ||
> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
> index 7839bfd1ac6a..aeedd5ec6b8c 100644
> --- a/net/ethtool/rings.c
> +++ b/net/ethtool/rings.c
> @@ -215,17 +215,16 @@ ethnl_set_rings_validate(struct ethnl_req_info *req_info,
> static int
> ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
> {
> - struct kernel_ethtool_ringparam kernel_ringparam = {};
> - struct ethtool_ringparam ringparam = {};
> + struct kernel_ethtool_ringparam kernel_ringparam;
> struct net_device *dev = req_info->dev;
> + struct ethtool_ringparam ringparam;
> struct nlattr **tb = info->attrs;
> const struct nlattr *err_attr;
> bool mod = false;
> int ret;
>
> - dev->ethtool_ops->get_ringparam(dev, &ringparam,
> - &kernel_ringparam, info->extack);
> - kernel_ringparam.tcp_data_split = dev->cfg->hds_config;
> + ethtool_ringparam_get_cfg(dev, &ringparam, &kernel_ringparam,
> + info->extack);
>
> ethnl_update_u32(&ringparam.rx_pending, tb[ETHTOOL_A_RINGS_RX], &mod);
> ethnl_update_u32(&ringparam.rx_mini_pending,
> --
> 2.48.1
>
Powered by blists - more mailing lists