diff mbox series

[net-next,2/6] net: provide pending ring configuration in net_device

Message ID 20250117194815.1514410-3-kuba@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: ethtool: fixes for HDS threshold | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 39 this patch: 39
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers warning 1 maintainers not CCed: maxime.chevallier@bootlin.com
netdev/build_clang success Errors and warnings before: 6619 this patch: 6619
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4096 this patch: 4096
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 93 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 95 this patch: 95
netdev/source_inline success Was 0 now: 0

Commit Message

Jakub Kicinski Jan. 17, 2025, 7:48 p.m. UTC 
Record the pending configuration in net_device struct.
ethtool core duplicates the current config and the specific
handlers (for now just ringparam) can modify it.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netdevice.h |  6 ++++++
 net/core/dev.c            |  2 ++
 net/ethtool/netlink.c     | 22 +++++++++++++++++++---
 net/ethtool/rings.c       |  8 +++-----
 4 files changed, 30 insertions(+), 8 deletions(-)

Comments

Michael Chan Jan. 17, 2025, 10:07 p.m. UTC | #1 
On Fri, Jan 17, 2025 at 11:48 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Record the pending configuration in net_device struct.
> ethtool core duplicates the current config and the specific
> handlers (for now just ringparam) can modify it.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

> @@ -688,21 +690,35 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
>                         goto out_dev;
>         }
>
> +       dev = req_info.dev;
> +
> +       dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
> +                                  GFP_KERNEL_ACCOUNT);
> +       if (!dev->cfg_pending) {
> +               ret = -ENOMEM;
> +               goto out_tie_cfg;
> +       }
> +
>         rtnl_lock();
> -       ret = ethnl_ops_begin(req_info.dev);
> +       ret = ethnl_ops_begin(dev);
>         if (ret < 0)
>                 goto out_rtnl;
>
>         ret = ops->set(&req_info, info);
>         if (ret <= 0)
>                 goto out_ops;
> -       ethtool_notify(req_info.dev, ops->set_ntf_cmd, NULL);
> +       ethtool_notify(dev, ops->set_ntf_cmd, NULL);
>
>         ret = 0;
>  out_ops:
> -       ethnl_ops_complete(req_info.dev);
> +       ethnl_ops_complete(dev);
>  out_rtnl:
>         rtnl_unlock();
> +       if (ret >= 0) /* success! */
> +               swap(dev->cfg, dev->cfg_pending);
> +       kfree(dev->cfg_pending);
> +out_tie_cfg:
> +       dev->cfg_pending = dev->cfg;

If I understand this correctly, cfg_pending can momentarily point to
the old cfg and freed memory before pointing to the new cfg outside of
rtnl_lock.  Shouldn't it be inside the rtnl_lock?

In the bnxt patch, we now look at cfg_pending so it must always point
to the correct cfg.
Jakub Kicinski Jan. 17, 2025, 10:18 p.m. UTC | #2 
On Fri, 17 Jan 2025 14:07:03 -0800 Michael Chan wrote:
> > +out_tie_cfg:
> > +       dev->cfg_pending = dev->cfg;  
> 
> If I understand this correctly, cfg_pending can momentarily point to
> the old cfg and freed memory before pointing to the new cfg outside of
> rtnl_lock.  Shouldn't it be inside the rtnl_lock?
> 
> In the bnxt patch, we now look at cfg_pending so it must always point
> to the correct cfg.

Good catch, I moved it out to avoid an allocation under rtnl_lock..
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 173a8b3a9eb2..8da4c61f97b9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2413,6 +2413,12 @@  struct net_device {
 
 	/** @cfg: net_device queue-related configuration */
 	struct netdev_config	*cfg;
+	/**
+	 * @cfg_pending: same as @cfg but when device is being actively
+	 *	reconfigured includes any changes to the configuration
+	 *	requested by the user, but which may or may not be rejected.
+	 */
+	struct netdev_config	*cfg_pending;
 	struct ethtool_netdev_state *ethtool;
 
 	/* protected by rtnl_lock */
diff --git a/net/core/dev.c b/net/core/dev.c
index 5bcf44e3bc6c..1daedb0f8aad 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11543,6 +11543,7 @@  struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->cfg = kzalloc(sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
 	if (!dev->cfg)
 		goto free_all;
+	dev->cfg_pending = dev->cfg;
 
 	napi_config_sz = array_size(maxqs, sizeof(*dev->napi_config));
 	dev->napi_config = kvzalloc(napi_config_sz, GFP_KERNEL_ACCOUNT);
@@ -11600,6 +11601,7 @@  void free_netdev(struct net_device *dev)
 
 	mutex_destroy(&dev->lock);
 
+	WARN_ON(dev->cfg != dev->cfg_pending);
 	kfree(dev->cfg);
 	kfree(dev->ethtool);
 	netif_free_tx_queues(dev);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 849c98e637c6..0d47e69dcda0 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <net/netdev_queues.h>
 #include <net/sock.h>
 #include <linux/ethtool_netlink.h>
 #include <linux/phy_link_topology.h>
@@ -667,6 +668,7 @@  static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	const struct ethnl_request_ops *ops;
 	struct ethnl_req_info req_info = {};
 	const u8 cmd = info->genlhdr->cmd;
+	struct net_device *dev;
 	int ret;
 
 	ops = ethnl_default_requests[cmd];
@@ -688,21 +690,35 @@  static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 			goto out_dev;
 	}
 
+	dev = req_info.dev;
+
+	dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
+				   GFP_KERNEL_ACCOUNT);
+	if (!dev->cfg_pending) {
+		ret = -ENOMEM;
+		goto out_tie_cfg;
+	}
+
 	rtnl_lock();
-	ret = ethnl_ops_begin(req_info.dev);
+	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		goto out_rtnl;
 
 	ret = ops->set(&req_info, info);
 	if (ret <= 0)
 		goto out_ops;
-	ethtool_notify(req_info.dev, ops->set_ntf_cmd, NULL);
+	ethtool_notify(dev, ops->set_ntf_cmd, NULL);
 
 	ret = 0;
 out_ops:
-	ethnl_ops_complete(req_info.dev);
+	ethnl_ops_complete(dev);
 out_rtnl:
 	rtnl_unlock();
+	if (ret >= 0) /* success! */
+		swap(dev->cfg, dev->cfg_pending);
+	kfree(dev->cfg_pending);
+out_tie_cfg:
+	dev->cfg_pending = dev->cfg;
 out_dev:
 	ethnl_parse_header_dev_put(&req_info);
 	return ret;
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 7a3c2a2dff12..5e8ba81fbb3e 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -294,13 +294,11 @@  ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 		return -EINVAL;
 	}
 
+	dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
+	dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
+
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, info->extack);
-	if (!ret) {
-		dev->cfg->hds_config = kernel_ringparam.tcp_data_split;
-		dev->cfg->hds_thresh = kernel_ringparam.hds_thresh;
-	}
-
 	return ret < 0 ? ret : 1;
 }