[PATCH net-next v7 0/3] net: ethtool: Introduce ethnl dump helpers

[PATCH net-next v7 0/3] net: ethtool: Introduce ethnl dump helpers

[Maxime Chevallier]

Hi everyone,

This is V7 for per-phy DUMP helpers, improving support for ->dumpit()
operations for PHY targetting commands.

This V7 simply contains a conversion from dev_hold() to netdev_hold(),
I also included a patch converting the original ->dumpit(). As this
refcounting is still a bit obscure to me, let me know if I'm getting
this wrong...

Thanks,

Maxime

Changes in V7:
 - Move from dev_hold to netdev_hold()
 - Add an extra patch fr dev_hold() conversion.

Changes in V6:
 - Squash pse and plca patches into the first one, to avoid unused
   functions (Jakub)

Changes in V5:
 - Move to a less generic approach, focusing only on the PHY case.

Changes in V4:
 - Don't grab rcu_read_lock when we already have a refcounter netdev on
   the filtered dump path (Paolo)
 - Move the dump_all stuff in a dedicated helper (Paolo)
 - Added patch 1 to set the dev in ctx->req_info

Changes in V3:
 - Fixed some typos and xmas tree issues
 - Added a missing check for EOPNOTSUPP in patch 1
 - Added missing kdoc
 - Added missing comments in phy_reply_size

Changes in V2:
 - Rebased on the netdev_lock work by Stanislav and the fixes from Eric
 - Fixed a bissectability issue
 - Fixed kdoc for the new ethnl ops and fields

V1: https://lore.kernel.org/netdev/20250305141938.319282-1-maxime.chevallier@bootlin.com/
V2: https://lore.kernel.org/netdev/20250308155440.267782-1-maxime.chevallier@bootlin.com/
V3: https://lore.kernel.org/netdev/20250313182647.250007-1-maxime.chevallier@bootlin.com/
V4: https://lore.kernel.org/netdev/20250324104012.367366-1-maxime.chevallier@bootlin.com/
V5: https://lore.kernel.org/netdev/20250410123350.174105-1-maxime.chevallier@bootlin.com/
V6: https://lore.kernel.org/netdev/20250415085155.132963-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (3):
  net: ethtool: Introduce per-PHY DUMP operations
  net: ethtool: phy: Convert the PHY_GET command to generic phy dump
  net: ethtool: netlink: Use netdev_hold for dumpit() operations

 net/ethtool/netlink.c | 195 ++++++++++++++++++++++--
 net/ethtool/netlink.h |   4 -
 net/ethtool/phy.c     | 342 ++++++++++++------------------------------
 3 files changed, 276 insertions(+), 265 deletions(-)

-- 
2.49.0

[PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Maxime Chevallier]

ethnl commands that target a phy_device need a DUMP implementation that
will fill the reply for every PHY behind a netdev. We therefore need to
iterate over the dev->topo to list them.

When multiple PHYs are behind the same netdev, it's also useful to
perform DUMP with a filter on a given netdev, to get the capability of
every PHY.

Implement dedicated genl ->start(), ->dumpit() and ->done() operations
for PHY-targetting command, allowing filtered dumps and using a dump
context that keep track of the PHY iteration for multi-message dump.

PSE-PD and PLCA are converted to this new set of ops along the way.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V7: Move to netdev_hold()

 net/ethtool/netlink.c | 181 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 172 insertions(+), 9 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 977beeaaa2f9..42b8aa5569a7 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -357,6 +357,16 @@ struct ethnl_dump_ctx {
 	unsigned long			pos_ifindex;
 };
 
+/**
+ * struct ethnl_perphy_dump_ctx - context for dumpit() PHY-aware callbacks
+ * @ethnl_ctx: generic ethnl context
+ * @pos_phyindex: iterator position for multi-msg DUMP
+ */
+struct ethnl_perphy_dump_ctx {
+	struct ethnl_dump_ctx	ethnl_ctx;
+	unsigned long		pos_phyindex;
+};
+
 static const struct ethnl_request_ops *
 ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_STRSET_GET]	= &ethnl_strset_request_ops,
@@ -407,6 +417,12 @@ static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
 	return (struct ethnl_dump_ctx *)cb->ctx;
 }
 
+static struct ethnl_perphy_dump_ctx *
+ethnl_perphy_dump_context(struct netlink_callback *cb)
+{
+	return (struct ethnl_perphy_dump_ctx *)cb->ctx;
+}
+
 /**
  * ethnl_default_parse() - Parse request message
  * @req_info:    pointer to structure to put data into
@@ -662,6 +678,153 @@ static int ethnl_default_start(struct netlink_callback *cb)
 	return ret;
 }
 
+/* perphy ->start() handler for GET requests */
+static int ethnl_perphy_start(struct netlink_callback *cb)
+{
+	struct ethnl_perphy_dump_ctx *phy_ctx = ethnl_perphy_dump_context(cb);
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct ethnl_dump_ctx *ctx = &phy_ctx->ethnl_ctx;
+	struct ethnl_reply_data *reply_data;
+	const struct ethnl_request_ops *ops;
+	struct ethnl_req_info *req_info;
+	struct genlmsghdr *ghdr;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
+
+	ghdr = nlmsg_data(cb->nlh);
+	ops = ethnl_default_requests[ghdr->cmd];
+	if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", ghdr->cmd))
+		return -EOPNOTSUPP;
+	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
+	if (!req_info)
+		return -ENOMEM;
+	reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL);
+	if (!reply_data) {
+		ret = -ENOMEM;
+		goto free_req_info;
+	}
+
+	/* Don't ignore the dev even for DUMP requests */
+	ret = ethnl_default_parse(req_info, &info->info, ops, false);
+	if (ret < 0)
+		goto free_reply_data;
+
+	ctx->ops = ops;
+	ctx->req_info = req_info;
+	ctx->reply_data = reply_data;
+	ctx->pos_ifindex = 0;
+
+	return 0;
+
+free_reply_data:
+	kfree(reply_data);
+free_req_info:
+	kfree(req_info);
+
+	return ret;
+}
+
+static int ethnl_perphy_dump_one_dev(struct sk_buff *skb,
+				     struct net_device *dev,
+				     struct ethnl_perphy_dump_ctx *ctx,
+				     const struct genl_info *info)
+{
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+	struct phy_device_node *pdn;
+	int ret = 0;
+
+	if (!dev->link_topo)
+		return 0;
+
+	xa_for_each_start(&dev->link_topo->phys, ctx->pos_phyindex, pdn,
+			  ctx->pos_phyindex) {
+		ethnl_ctx->req_info->phy_index = ctx->pos_phyindex;
+
+		/* We can re-use the original dump_one as ->prepare_data in
+		 * commands use ethnl_req_get_phydev(), which gets the PHY from
+		 * the req_info->phy_index
+		 */
+		ret = ethnl_default_dump_one(skb, dev, ethnl_ctx, info);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int ethnl_perphy_dump_all_dev(struct sk_buff *skb,
+				     struct ethnl_perphy_dump_ctx *ctx,
+				     const struct genl_info *info)
+{
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+	struct net *net = sock_net(skb->sk);
+	netdevice_tracker dev_tracker;
+	struct net_device *dev;
+	int ret = 0;
+
+	rcu_read_lock();
+	for_each_netdev_dump(net, dev, ethnl_ctx->pos_ifindex) {
+		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
+		rcu_read_unlock();
+
+		/* per-PHY commands use ethnl_req_get_phydev(), which needs the
+		 * net_device in the req_info
+		 */
+		ethnl_ctx->req_info->dev = dev;
+		ret = ethnl_perphy_dump_one_dev(skb, dev, ctx, info);
+
+		rcu_read_lock();
+		netdev_put(dev, &dev_tracker);
+
+		if (ret < 0 && ret != -EOPNOTSUPP) {
+			if (likely(skb->len))
+				ret = skb->len;
+			break;
+		}
+		ret = 0;
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
+/* perphy ->dumpit() handler for GET requests. */
+static int ethnl_perphy_dumpit(struct sk_buff *skb,
+			       struct netlink_callback *cb)
+{
+	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+	int ret = 0;
+
+	if (ethnl_ctx->req_info->dev) {
+		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
+						ctx, genl_info_dump(cb));
+
+		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
+			ret = skb->len;
+
+		netdev_put(ethnl_ctx->req_info->dev,
+			   &ethnl_ctx->req_info->dev_tracker);
+	} else {
+		ret = ethnl_perphy_dump_all_dev(skb, ctx, genl_info_dump(cb));
+	}
+
+	return ret;
+}
+
+/* perphy ->done() handler for GET requests */
+static int ethnl_perphy_done(struct netlink_callback *cb)
+{
+	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
+	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
+
+	kfree(ethnl_ctx->reply_data);
+	kfree(ethnl_ctx->req_info);
+
+	return 0;
+}
+
 /* default ->done() handler for GET requests */
 static int ethnl_default_done(struct netlink_callback *cb)
 {
@@ -1200,9 +1363,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
 	{
 		.cmd	= ETHTOOL_MSG_PSE_GET,
 		.doit	= ethnl_default_doit,
-		.start	= ethnl_default_start,
-		.dumpit	= ethnl_default_dumpit,
-		.done	= ethnl_default_done,
+		.start	= ethnl_perphy_start,
+		.dumpit	= ethnl_perphy_dumpit,
+		.done	= ethnl_perphy_done,
 		.policy = ethnl_pse_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_pse_get_policy) - 1,
 	},
@@ -1224,9 +1387,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
 	{
 		.cmd	= ETHTOOL_MSG_PLCA_GET_CFG,
 		.doit	= ethnl_default_doit,
-		.start	= ethnl_default_start,
-		.dumpit	= ethnl_default_dumpit,
-		.done	= ethnl_default_done,
+		.start	= ethnl_perphy_start,
+		.dumpit	= ethnl_perphy_dumpit,
+		.done	= ethnl_perphy_done,
 		.policy = ethnl_plca_get_cfg_policy,
 		.maxattr = ARRAY_SIZE(ethnl_plca_get_cfg_policy) - 1,
 	},
@@ -1240,9 +1403,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
 	{
 		.cmd	= ETHTOOL_MSG_PLCA_GET_STATUS,
 		.doit	= ethnl_default_doit,
-		.start	= ethnl_default_start,
-		.dumpit	= ethnl_default_dumpit,
-		.done	= ethnl_default_done,
+		.start	= ethnl_perphy_start,
+		.dumpit	= ethnl_perphy_dumpit,
+		.done	= ethnl_perphy_done,
 		.policy = ethnl_plca_get_status_policy,
 		.maxattr = ARRAY_SIZE(ethnl_plca_get_status_policy) - 1,
 	},
-- 
2.49.0

[PATCH net-next v7 2/3] net: ethtool: phy: Convert the PHY_GET command to generic phy dump

[Maxime Chevallier]

Now that we have an infrastructure in ethnl for perphy DUMPs, we can get
rid of the custom ->doit and ->dumpit to deal with PHY listing commands.

As most of the code was custom, this basically means re-writing how we
deal with PHY listing.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/netlink.c |   9 +-
 net/ethtool/netlink.h |   4 -
 net/ethtool/phy.c     | 342 ++++++++++++------------------------------
 3 files changed, 101 insertions(+), 254 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 42b8aa5569a7..8ccc6957295d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -410,6 +410,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_TSCONFIG_GET]	= &ethnl_tsconfig_request_ops,
 	[ETHTOOL_MSG_TSCONFIG_SET]	= &ethnl_tsconfig_request_ops,
+	[ETHTOOL_MSG_PHY_GET]		= &ethnl_phy_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1434,10 +1435,10 @@ static const struct genl_ops ethtool_genl_ops[] = {
 	},
 	{
 		.cmd	= ETHTOOL_MSG_PHY_GET,
-		.doit	= ethnl_phy_doit,
-		.start	= ethnl_phy_start,
-		.dumpit	= ethnl_phy_dumpit,
-		.done	= ethnl_phy_done,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_perphy_start,
+		.dumpit	= ethnl_perphy_dumpit,
+		.done	= ethnl_perphy_done,
 		.policy = ethnl_phy_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_phy_get_policy) - 1,
 	},
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index ec6ab5443a6f..91b953924af3 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -499,10 +499,6 @@ int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info);
 int ethnl_rss_dump_start(struct netlink_callback *cb);
 int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
-int ethnl_phy_start(struct netlink_callback *cb);
-int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info);
-int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
-int ethnl_phy_done(struct netlink_callback *cb);
 int ethnl_tsinfo_start(struct netlink_callback *cb);
 int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_tsinfo_done(struct netlink_callback *cb);
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index 1f590e8d75ed..68372bef4b2f 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -12,304 +12,154 @@
 #include <net/netdev_lock.h>
 
 struct phy_req_info {
-	struct ethnl_req_info		base;
-	struct phy_device_node		*pdn;
+	struct ethnl_req_info base;
 };
 
-#define PHY_REQINFO(__req_base) \
-	container_of(__req_base, struct phy_req_info, base)
+struct phy_reply_data {
+	struct ethnl_reply_data	base;
+	u32 phyindex;
+	char *drvname;
+	char *name;
+	unsigned int upstream_type;
+	char *upstream_sfp_name;
+	unsigned int upstream_index;
+	char *downstream_sfp_name;
+};
+
+#define PHY_REPDATA(__reply_base) \
+	container_of(__reply_base, struct phy_reply_data, base)
 
 const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1] = {
 	[ETHTOOL_A_PHY_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
 };
 
-/* Caller holds rtnl */
-static ssize_t
-ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
-		     struct netlink_ext_ack *extack)
+static int phy_reply_size(const struct ethnl_req_info *req_info,
+			  const struct ethnl_reply_data *reply_data)
 {
-	struct phy_req_info *req_info = PHY_REQINFO(req_base);
-	struct phy_device_node *pdn = req_info->pdn;
-	struct phy_device *phydev = pdn->phy;
+	struct phy_reply_data *rep_data = PHY_REPDATA(reply_data);
 	size_t size = 0;
 
-	ASSERT_RTNL();
-
 	/* ETHTOOL_A_PHY_INDEX */
 	size += nla_total_size(sizeof(u32));
 
 	/* ETHTOOL_A_DRVNAME */
-	if (phydev->drv)
-		size += nla_total_size(strlen(phydev->drv->name) + 1);
+	if (rep_data->drvname)
+		size += nla_total_size(strlen(rep_data->drvname) + 1);
 
 	/* ETHTOOL_A_NAME */
-	size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)) + 1);
+	size += nla_total_size(strlen(rep_data->name) + 1);
 
 	/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
 	size += nla_total_size(sizeof(u32));
 
-	if (phy_on_sfp(phydev)) {
-		const char *upstream_sfp_name = sfp_get_name(pdn->parent_sfp_bus);
-
-		/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
-		if (upstream_sfp_name)
-			size += nla_total_size(strlen(upstream_sfp_name) + 1);
+	/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
+	if (rep_data->upstream_sfp_name)
+		size += nla_total_size(strlen(rep_data->upstream_sfp_name) + 1);
 
-		/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
+	/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
+	if (rep_data->upstream_index)
 		size += nla_total_size(sizeof(u32));
-	}
 
 	/* ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME */
-	if (phydev->sfp_bus) {
-		const char *sfp_name = sfp_get_name(phydev->sfp_bus);
-
-		if (sfp_name)
-			size += nla_total_size(strlen(sfp_name) + 1);
-	}
+	if (rep_data->downstream_sfp_name)
+		size += nla_total_size(strlen(rep_data->downstream_sfp_name) + 1);
 
 	return size;
 }
 
-static int
-ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
+static int phy_prepare_data(const struct ethnl_req_info *req_info,
+			    struct ethnl_reply_data *reply_data,
+			    const struct genl_info *info)
 {
-	struct phy_req_info *req_info = PHY_REQINFO(req_base);
-	struct phy_device_node *pdn = req_info->pdn;
-	struct phy_device *phydev = pdn->phy;
-	enum phy_upstream ptype;
+	struct phy_link_topology *topo = reply_data->dev->link_topo;
+	struct phy_reply_data *rep_data = PHY_REPDATA(reply_data);
+	struct nlattr **tb = info->attrs;
+	struct phy_device_node *pdn;
+	struct phy_device *phydev;
 
-	ptype = pdn->upstream_type;
+	/* RTNL is held by the caller */
+	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PHY_HEADER,
+				      info->extack);
+	if (IS_ERR_OR_NULL(phydev))
+		return -EOPNOTSUPP;
 
-	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
-	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
-	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype))
-		return -EMSGSIZE;
+	pdn = xa_load(&topo->phys, phydev->phyindex);
+	if (!pdn)
+		return -EOPNOTSUPP;
 
-	if (phydev->drv &&
-	    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))
-		return -EMSGSIZE;
+	rep_data->phyindex = phydev->phyindex;
+	rep_data->name = kstrdup(dev_name(&phydev->mdio.dev), GFP_KERNEL);
+	rep_data->drvname = kstrdup(phydev->drv->name, GFP_KERNEL);
+	rep_data->upstream_type = pdn->upstream_type;
 
-	if (ptype == PHY_UPSTREAM_PHY) {
+	if (pdn->upstream_type == PHY_UPSTREAM_PHY) {
 		struct phy_device *upstream = pdn->upstream.phydev;
-		const char *sfp_upstream_name;
-
-		/* Parent index */
-		if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
-			return -EMSGSIZE;
-
-		if (pdn->parent_sfp_bus) {
-			sfp_upstream_name = sfp_get_name(pdn->parent_sfp_bus);
-			if (sfp_upstream_name &&
-			    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
-					   sfp_upstream_name))
-				return -EMSGSIZE;
-		}
-	}
-
-	if (phydev->sfp_bus) {
-		const char *sfp_name = sfp_get_name(phydev->sfp_bus);
-
-		if (sfp_name &&
-		    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
-				   sfp_name))
-			return -EMSGSIZE;
+		rep_data->upstream_index = upstream->phyindex;
 	}
 
-	return 0;
-}
-
-static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
-				   struct nlattr **tb,
-				   struct netlink_ext_ack *extack)
-{
-	struct phy_link_topology *topo = req_base->dev->link_topo;
-	struct phy_req_info *req_info = PHY_REQINFO(req_base);
-	struct phy_device *phydev;
+	if (pdn->parent_sfp_bus)
+		rep_data->upstream_sfp_name = kstrdup(sfp_get_name(pdn->parent_sfp_bus),
+						      GFP_KERNEL);
 
-	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PHY_HEADER,
-				      extack);
-	if (!phydev)
-		return 0;
-
-	if (IS_ERR(phydev))
-		return PTR_ERR(phydev);
-
-	if (!topo)
-		return 0;
-
-	req_info->pdn = xa_load(&topo->phys, phydev->phyindex);
+	if (phydev->sfp_bus)
+		rep_data->downstream_sfp_name = kstrdup(sfp_get_name(phydev->sfp_bus),
+							GFP_KERNEL);
 
 	return 0;
 }
 
-int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
+static int phy_fill_reply(struct sk_buff *skb,
+			  const struct ethnl_req_info *req_info,
+			  const struct ethnl_reply_data *reply_data)
 {
-	struct phy_req_info req_info = {};
-	struct nlattr **tb = info->attrs;
-	struct sk_buff *rskb;
-	void *reply_payload;
-	int reply_len;
-	int ret;
-
-	ret = ethnl_parse_header_dev_get(&req_info.base,
-					 tb[ETHTOOL_A_PHY_HEADER],
-					 genl_info_net(info), info->extack,
-					 true);
-	if (ret < 0)
-		return ret;
-
-	rtnl_lock();
-	netdev_lock_ops(req_info.base.dev);
-
-	ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
-	if (ret < 0)
-		goto err_unlock;
-
-	/* No PHY, return early */
-	if (!req_info.pdn)
-		goto err_unlock;
-
-	ret = ethnl_phy_reply_size(&req_info.base, info->extack);
-	if (ret < 0)
-		goto err_unlock;
-	reply_len = ret + ethnl_reply_header_size();
-
-	rskb = ethnl_reply_init(reply_len, req_info.base.dev,
-				ETHTOOL_MSG_PHY_GET_REPLY,
-				ETHTOOL_A_PHY_HEADER,
-				info, &reply_payload);
-	if (!rskb) {
-		ret = -ENOMEM;
-		goto err_unlock;
-	}
-
-	ret = ethnl_phy_fill_reply(&req_info.base, rskb);
-	if (ret)
-		goto err_free_msg;
+	struct phy_reply_data *rep_data = PHY_REPDATA(reply_data);
 
-	netdev_unlock_ops(req_info.base.dev);
-	rtnl_unlock();
-	ethnl_parse_header_dev_put(&req_info.base);
-	genlmsg_end(rskb, reply_payload);
-
-	return genlmsg_reply(rskb, info);
-
-err_free_msg:
-	nlmsg_free(rskb);
-err_unlock:
-	netdev_unlock_ops(req_info.base.dev);
-	rtnl_unlock();
-	ethnl_parse_header_dev_put(&req_info.base);
-	return ret;
-}
-
-struct ethnl_phy_dump_ctx {
-	struct phy_req_info	*phy_req_info;
-	unsigned long ifindex;
-	unsigned long phy_index;
-};
-
-int ethnl_phy_start(struct netlink_callback *cb)
-{
-	const struct genl_info *info = genl_info_dump(cb);
-	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
-	int ret;
-
-	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
-
-	ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
-	if (!ctx->phy_req_info)
-		return -ENOMEM;
-
-	ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
-					 info->attrs[ETHTOOL_A_PHY_HEADER],
-					 sock_net(cb->skb->sk), cb->extack,
-					 false);
-	ctx->ifindex = 0;
-	ctx->phy_index = 0;
-
-	if (ret)
-		kfree(ctx->phy_req_info);
+	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, rep_data->phyindex) ||
+	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, rep_data->name) ||
+	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, rep_data->upstream_type))
+		return -EMSGSIZE;
 
-	return ret;
-}
+	if (rep_data->drvname &&
+	    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, rep_data->drvname))
+		return -EMSGSIZE;
 
-int ethnl_phy_done(struct netlink_callback *cb)
-{
-	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	if (rep_data->upstream_index &&
+	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX,
+			rep_data->upstream_index))
+		return -EMSGSIZE;
 
-	if (ctx->phy_req_info->base.dev)
-		ethnl_parse_header_dev_put(&ctx->phy_req_info->base);
+	if (rep_data->upstream_sfp_name &&
+	    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
+			   rep_data->upstream_sfp_name))
+		return -EMSGSIZE;
 
-	kfree(ctx->phy_req_info);
+	if (rep_data->downstream_sfp_name &&
+	    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
+			   rep_data->downstream_sfp_name))
+		return -EMSGSIZE;
 
 	return 0;
 }
 
-static int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
-				  struct netlink_callback *cb)
+static void phy_cleanup_data(struct ethnl_reply_data *reply_data)
 {
-	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
-	struct phy_req_info *pri = ctx->phy_req_info;
-	struct phy_device_node *pdn;
-	int ret = 0;
-	void *ehdr;
-
-	if (!dev->link_topo)
-		return 0;
-
-	xa_for_each_start(&dev->link_topo->phys, ctx->phy_index, pdn, ctx->phy_index) {
-		ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_PHY_GET_REPLY);
-		if (!ehdr) {
-			ret = -EMSGSIZE;
-			break;
-		}
-
-		ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_PHY_HEADER);
-		if (ret < 0) {
-			genlmsg_cancel(skb, ehdr);
-			break;
-		}
-
-		pri->pdn = pdn;
-		ret = ethnl_phy_fill_reply(&pri->base, skb);
-		if (ret < 0) {
-			genlmsg_cancel(skb, ehdr);
-			break;
-		}
-
-		genlmsg_end(skb, ehdr);
-	}
+	struct phy_reply_data *rep_data = PHY_REPDATA(reply_data);
 
-	return ret;
+	kfree(rep_data->drvname);
+	kfree(rep_data->name);
+	kfree(rep_data->upstream_sfp_name);
+	kfree(rep_data->downstream_sfp_name);
 }
 
-int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
-{
-	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
-	struct net *net = sock_net(skb->sk);
-	struct net_device *dev;
-	int ret = 0;
-
-	rtnl_lock();
-
-	if (ctx->phy_req_info->base.dev) {
-		dev = ctx->phy_req_info->base.dev;
-		netdev_lock_ops(dev);
-		ret = ethnl_phy_dump_one_dev(skb, dev, cb);
-		netdev_unlock_ops(dev);
-	} else {
-		for_each_netdev_dump(net, dev, ctx->ifindex) {
-			netdev_lock_ops(dev);
-			ret = ethnl_phy_dump_one_dev(skb, dev, cb);
-			netdev_unlock_ops(dev);
-			if (ret)
-				break;
-
-			ctx->phy_index = 0;
-		}
-	}
-	rtnl_unlock();
-
-	return ret;
-}
+const struct ethnl_request_ops ethnl_phy_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PHY_GET,
+	.reply_cmd		= ETHTOOL_MSG_PHY_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_PHY_HEADER,
+	.req_info_size		= sizeof(struct phy_req_info),
+	.reply_data_size	= sizeof(struct phy_reply_data),
+
+	.prepare_data		= phy_prepare_data,
+	.reply_size		= phy_reply_size,
+	.fill_reply		= phy_fill_reply,
+	.cleanup_data		= phy_cleanup_data,
+};
-- 
2.49.0

[PATCH net-next v7 3/3] net: ethtool: netlink: Use netdev_hold for dumpit() operations

[Maxime Chevallier]

Move away from dev_hold and use netdev_hold with a local reftracker when
performing a DUMP on each netdev.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/netlink.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 8ccc6957295d..9f08e9d9ed21 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -601,18 +601,19 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
 {
 	struct ethnl_dump_ctx *ctx = ethnl_dump_context(cb);
 	struct net *net = sock_net(skb->sk);
+	netdevice_tracker dev_tracker;
 	struct net_device *dev;
 	int ret = 0;
 
 	rcu_read_lock();
 	for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
-		dev_hold(dev);
+		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
 		rcu_read_unlock();
 
 		ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
 
 		rcu_read_lock();
-		dev_put(dev);
+		netdev_put(dev, &dev_tracker);
 
 		if (ret < 0 && ret != -EOPNOTSUPP) {
 			if (likely(skb->len))
-- 
2.49.0

Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Jakub Kicinski]

On Tue, 22 Apr 2025 18:17:14 +0200 Maxime Chevallier wrote:
> +/* perphy ->start() handler for GET requests */

Just because I think there are real bugs, I will allow myself an
uber-nit of asking to spell the perphy as per-PHY or such in the
comment? :)

> +static int ethnl_perphy_start(struct netlink_callback *cb)
> +{
> +	struct ethnl_perphy_dump_ctx *phy_ctx = ethnl_perphy_dump_context(cb);
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +	struct ethnl_dump_ctx *ctx = &phy_ctx->ethnl_ctx;
> +	struct ethnl_reply_data *reply_data;
> +	const struct ethnl_request_ops *ops;
> +	struct ethnl_req_info *req_info;
> +	struct genlmsghdr *ghdr;
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
> +
> +	ghdr = nlmsg_data(cb->nlh);
> +	ops = ethnl_default_requests[ghdr->cmd];
> +	if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", ghdr->cmd))
> +		return -EOPNOTSUPP;
> +	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
> +	if (!req_info)
> +		return -ENOMEM;
> +	reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL);
> +	if (!reply_data) {
> +		ret = -ENOMEM;
> +		goto free_req_info;
> +	}
> +
> +	/* Don't ignore the dev even for DUMP requests */

another nit, this comment wasn't super clear without looking at the dump
for non-per-phy case. Maybe:

	/* Unlike per-dev dump, don't ignore dev. The dump handler
	 * will notice it and dump PHYs from given dev.
	 */
?

> +	ret = ethnl_default_parse(req_info, &info->info, ops, false);
> +	if (ret < 0)
> +		goto free_reply_data;
> +
> +	ctx->ops = ops;
> +	ctx->req_info = req_info;
> +	ctx->reply_data = reply_data;
> +	ctx->pos_ifindex = 0;
> +
> +	return 0;
> +
> +free_reply_data:
> +	kfree(reply_data);
> +free_req_info:
> +	kfree(req_info);
> +
> +	return ret;
> +}
> +
> +static int ethnl_perphy_dump_one_dev(struct sk_buff *skb,
> +				     struct net_device *dev,
> +				     struct ethnl_perphy_dump_ctx *ctx,
> +				     const struct genl_info *info)
> +{
> +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> +	struct phy_device_node *pdn;
> +	int ret = 0;
> +
> +	if (!dev->link_topo)
> +		return 0;

Now for the bugs..

> +	xa_for_each_start(&dev->link_topo->phys, ctx->pos_phyindex, pdn,
> +			  ctx->pos_phyindex) {
> +		ethnl_ctx->req_info->phy_index = ctx->pos_phyindex;
> +
> +		/* We can re-use the original dump_one as ->prepare_data in
> +		 * commands use ethnl_req_get_phydev(), which gets the PHY from
> +		 * the req_info->phy_index
> +		 */
> +		ret = ethnl_default_dump_one(skb, dev, ethnl_ctx, info);
> +		if (ret)
> +			break;

		return ret;

> +	}

	ctx->pos_phyindex = 0;

	return 0;

IOW I don't see you resetting the pos_phyindex, so I think you'd only
dump correctly the first device? The next device will try to dump its
PHYs starting from the last index of the previous dev's PHY? [1]

> +	return ret;
> +}
> +
> +static int ethnl_perphy_dump_all_dev(struct sk_buff *skb,
> +				     struct ethnl_perphy_dump_ctx *ctx,
> +				     const struct genl_info *info)
> +{
> +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> +	struct net *net = sock_net(skb->sk);
> +	netdevice_tracker dev_tracker;
> +	struct net_device *dev;
> +	int ret = 0;
> +
> +	rcu_read_lock();
> +	for_each_netdev_dump(net, dev, ethnl_ctx->pos_ifindex) {
> +		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
> +		rcu_read_unlock();
> +
> +		/* per-PHY commands use ethnl_req_get_phydev(), which needs the
> +		 * net_device in the req_info
> +		 */
> +		ethnl_ctx->req_info->dev = dev;
> +		ret = ethnl_perphy_dump_one_dev(skb, dev, ctx, info);
> +
> +		rcu_read_lock();
> +		netdev_put(dev, &dev_tracker);

missing

		ethnl_ctx->req_info->dev = NULL;

right? Otherwise if we need to send multiple skbs the "continuation"
one will think we're doing a filtered dump.

Looking at commits 7c93a88785dae6 and c0111878d45e may be helpful,
but I doubt you can test it on a real system, filling even 4kB
may be hard for small messages :(

> +		if (ret < 0 && ret != -EOPNOTSUPP) {
> +			if (likely(skb->len))
> +				ret = skb->len;
> +			break;
> +		}
> +		ret = 0;

[1] or you can clear the pos_index here

> +	}
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +
> +/* perphy ->dumpit() handler for GET requests. */
> +static int ethnl_perphy_dumpit(struct sk_buff *skb,
> +			       struct netlink_callback *cb)
> +{
> +	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
> +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> +	int ret = 0;
> +
> +	if (ethnl_ctx->req_info->dev) {
> +		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
> +						ctx, genl_info_dump(cb));
> +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> +			ret = skb->len;
> +
> +		netdev_put(ethnl_ctx->req_info->dev,
> +			   &ethnl_ctx->req_info->dev_tracker);

You have to release this in .done
dumpit gets called multiple times until we run out of objects to dump.
OTOH user may close the socket without finishing the dump operation.
So all .dumpit implementations must be "balanced". The only state we
should touch in them is the dump context to know where to pick up from
next time.

> +	} else {
> +		ret = ethnl_perphy_dump_all_dev(skb, ctx, genl_info_dump(cb));
> +	}
> +
> +	return ret;
> +}

Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Maxime Chevallier]

Hi Jakub,

On Thu, 24 Apr 2025 18:03:33 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 22 Apr 2025 18:17:14 +0200 Maxime Chevallier wrote:
> > +/* perphy ->start() handler for GET requests */  
> 
> Just because I think there are real bugs, I will allow myself an
> uber-nit of asking to spell the perphy as per-PHY or such in the
> comment? :)

No problem :) Thanks a lot for the review

> > +static int ethnl_perphy_start(struct netlink_callback *cb)
> > +{
> > +	struct ethnl_perphy_dump_ctx *phy_ctx = ethnl_perphy_dump_context(cb);
> > +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> > +	struct ethnl_dump_ctx *ctx = &phy_ctx->ethnl_ctx;
> > +	struct ethnl_reply_data *reply_data;
> > +	const struct ethnl_request_ops *ops;
> > +	struct ethnl_req_info *req_info;
> > +	struct genlmsghdr *ghdr;
> > +	int ret;
> > +
> > +	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
> > +
> > +	ghdr = nlmsg_data(cb->nlh);
> > +	ops = ethnl_default_requests[ghdr->cmd];
> > +	if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", ghdr->cmd))
> > +		return -EOPNOTSUPP;
> > +	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
> > +	if (!req_info)
> > +		return -ENOMEM;
> > +	reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL);
> > +	if (!reply_data) {
> > +		ret = -ENOMEM;
> > +		goto free_req_info;
> > +	}
> > +
> > +	/* Don't ignore the dev even for DUMP requests */  
> 
> another nit, this comment wasn't super clear without looking at the dump
> for non-per-phy case. Maybe:
> 
> 	/* Unlike per-dev dump, don't ignore dev. The dump handler
> 	 * will notice it and dump PHYs from given dev.
> 	 */
> ?

That's better indeed :)

> > +	ret = ethnl_default_parse(req_info, &info->info, ops, false);
> > +	if (ret < 0)
> > +		goto free_reply_data;
> > +
> > +	ctx->ops = ops;
> > +	ctx->req_info = req_info;
> > +	ctx->reply_data = reply_data;
> > +	ctx->pos_ifindex = 0;
> > +
> > +	return 0;
> > +
> > +free_reply_data:
> > +	kfree(reply_data);
> > +free_req_info:
> > +	kfree(req_info);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ethnl_perphy_dump_one_dev(struct sk_buff *skb,
> > +				     struct net_device *dev,
> > +				     struct ethnl_perphy_dump_ctx *ctx,
> > +				     const struct genl_info *info)
> > +{
> > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > +	struct phy_device_node *pdn;
> > +	int ret = 0;
> > +
> > +	if (!dev->link_topo)
> > +		return 0;  
> 
> Now for the bugs..
> 
> > +	xa_for_each_start(&dev->link_topo->phys, ctx->pos_phyindex, pdn,
> > +			  ctx->pos_phyindex) {
> > +		ethnl_ctx->req_info->phy_index = ctx->pos_phyindex;
> > +
> > +		/* We can re-use the original dump_one as ->prepare_data in
> > +		 * commands use ethnl_req_get_phydev(), which gets the PHY from
> > +		 * the req_info->phy_index
> > +		 */
> > +		ret = ethnl_default_dump_one(skb, dev, ethnl_ctx, info);
> > +		if (ret)
> > +			break;  
> 
> 		return ret;
> 
> > +	}  
> 
> 	ctx->pos_phyindex = 0;
> 
> 	return 0;
> 
> IOW I don't see you resetting the pos_phyindex, so I think you'd only
> dump correctly the first device? The next device will try to dump its
> PHYs starting from the last index of the previous dev's PHY? [1]

That is true... My mistake was to test on a system with one PHY only on
the first interface and a lot on the second, I'll adjust my tests and
fix that, thanks a lot for spotting !

> 
> > +	return ret;
> > +}
> > +
> > +static int ethnl_perphy_dump_all_dev(struct sk_buff *skb,
> > +				     struct ethnl_perphy_dump_ctx *ctx,
> > +				     const struct genl_info *info)
> > +{
> > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > +	struct net *net = sock_net(skb->sk);
> > +	netdevice_tracker dev_tracker;
> > +	struct net_device *dev;
> > +	int ret = 0;
> > +
> > +	rcu_read_lock();
> > +	for_each_netdev_dump(net, dev, ethnl_ctx->pos_ifindex) {
> > +		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
> > +		rcu_read_unlock();
> > +
> > +		/* per-PHY commands use ethnl_req_get_phydev(), which needs the
> > +		 * net_device in the req_info
> > +		 */
> > +		ethnl_ctx->req_info->dev = dev;
> > +		ret = ethnl_perphy_dump_one_dev(skb, dev, ctx, info);
> > +
> > +		rcu_read_lock();
> > +		netdev_put(dev, &dev_tracker);  
> 
> missing
> 
> 		ethnl_ctx->req_info->dev = NULL;
> 
> right? Otherwise if we need to send multiple skbs the "continuation"
> one will think we're doing a filtered dump.
> 
> Looking at commits 7c93a88785dae6 and c0111878d45e may be helpful,
> but I doubt you can test it on a real system, filling even 4kB
> may be hard for small messages :(

Ah damn, yes. I got that right I think in net/ethtool/phy.c but not
here.

As for testing, I do have a local patch to add PHY support to
netdevsim, allowing me to add an arbitrary number of PHYs to any nsim
devices. I'll make sure to test this case. I still plan to upstream the
netdevsim part at some point, but that still needs a bit of polishing...

> > +		if (ret < 0 && ret != -EOPNOTSUPP) {
> > +			if (likely(skb->len))
> > +				ret = skb->len;
> > +			break;
> > +		}
> > +		ret = 0;  
> 
> [1] or you can clear the pos_index here
> 
> > +	}
> > +	rcu_read_unlock();
> > +
> > +	return ret;
> > +}
> > +
> > +/* perphy ->dumpit() handler for GET requests. */
> > +static int ethnl_perphy_dumpit(struct sk_buff *skb,
> > +			       struct netlink_callback *cb)
> > +{
> > +	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
> > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > +	int ret = 0;
> > +
> > +	if (ethnl_ctx->req_info->dev) {
> > +		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
> > +						ctx, genl_info_dump(cb));
> > +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> > +			ret = skb->len;
> > +
> > +		netdev_put(ethnl_ctx->req_info->dev,
> > +			   &ethnl_ctx->req_info->dev_tracker);  
> 
> You have to release this in .done
> dumpit gets called multiple times until we run out of objects to dump.
> OTOH user may close the socket without finishing the dump operation.
> So all .dumpit implementations must be "balanced". The only state we
> should touch in them is the dump context to know where to pick up from
> next time.

Thanks for poiting it out.

Now that you say that, I guess that I should also move the reftracker
I'm using for the netdev_hold in ethnl_perphy_dump_one_dev() call to
struct ethnl_perphy_dump_ctx ? That way we make sure the netdev doesn't
go away in-between the multiple .dumpit() calls then...

Is that correct ?

> > +	} else {
> > +		ret = ethnl_perphy_dump_all_dev(skb, ctx, genl_info_dump(cb));
> > +	}
> > +
> > +	return ret;
> > +}  

Thanks a lot for the review, that's most helpful.

Maxime

Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Jakub Kicinski]

On Fri, 25 Apr 2025 09:01:53 +0200 Maxime Chevallier wrote:
> > > +/* perphy ->dumpit() handler for GET requests. */
> > > +static int ethnl_perphy_dumpit(struct sk_buff *skb,
> > > +			       struct netlink_callback *cb)
> > > +{
> > > +	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
> > > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > > +	int ret = 0;
> > > +
> > > +	if (ethnl_ctx->req_info->dev) {
> > > +		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
> > > +						ctx, genl_info_dump(cb));
> > > +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> > > +			ret = skb->len;
> > > +
> > > +		netdev_put(ethnl_ctx->req_info->dev,
> > > +			   &ethnl_ctx->req_info->dev_tracker);    
> > 
> > You have to release this in .done
> > dumpit gets called multiple times until we run out of objects to dump.
> > OTOH user may close the socket without finishing the dump operation.
> > So all .dumpit implementations must be "balanced". The only state we
> > should touch in them is the dump context to know where to pick up from
> > next time.  
> 
> Thanks for poiting it out.
> 
> Now that you say that, I guess that I should also move the reftracker
> I'm using for the netdev_hold in ethnl_perphy_dump_one_dev() call to
> struct ethnl_perphy_dump_ctx ? That way we make sure the netdev doesn't
> go away in-between the multiple .dumpit() calls then...
> 
> Is that correct ?

Probably not. That'd allow an unprivileged user space program to take 
a ref on a netdev, and never call recv() to make progress on the dump.

The possibility that the netdev may disappear half way thru is inherent
to the netlink dump model. We will pick back up within that netdev
based on its ifindex, no need to hold it.

Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Maxime Chevallier]

On Fri, 25 Apr 2025 17:10:44 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 25 Apr 2025 09:01:53 +0200 Maxime Chevallier wrote:
> > > > +/* perphy ->dumpit() handler for GET requests. */
> > > > +static int ethnl_perphy_dumpit(struct sk_buff *skb,
> > > > +			       struct netlink_callback *cb)
> > > > +{
> > > > +	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
> > > > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (ethnl_ctx->req_info->dev) {
> > > > +		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
> > > > +						ctx, genl_info_dump(cb));
> > > > +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> > > > +			ret = skb->len;
> > > > +
> > > > +		netdev_put(ethnl_ctx->req_info->dev,
> > > > +			   &ethnl_ctx->req_info->dev_tracker);      
> > > 
> > > You have to release this in .done
> > > dumpit gets called multiple times until we run out of objects to dump.
> > > OTOH user may close the socket without finishing the dump operation.
> > > So all .dumpit implementations must be "balanced". The only state we
> > > should touch in them is the dump context to know where to pick up from
> > > next time.    
> > 
> > Thanks for poiting it out.
> > 
> > Now that you say that, I guess that I should also move the reftracker
> > I'm using for the netdev_hold in ethnl_perphy_dump_one_dev() call to
> > struct ethnl_perphy_dump_ctx ? That way we make sure the netdev doesn't
> > go away in-between the multiple .dumpit() calls then...
> > 
> > Is that correct ?  
> 
> Probably not. That'd allow an unprivileged user space program to take 
> a ref on a netdev, and never call recv() to make progress on the dump.
> 
> The possibility that the netdev may disappear half way thru is inherent
> to the netlink dump model. We will pick back up within that netdev
> based on its ifindex, no need to hold it.

OK I'm good with that then ! Thanks for the explanations, makes a lot
of sense. I'll followup with a new version soon :)

Maxime

Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Maxime Chevallier]

Hi Jakub,

On Thu, 24 Apr 2025 18:03:33 -0700
Jakub Kicinski <kuba@kernel.org> wrote:


> > +
> > +/* perphy ->dumpit() handler for GET requests. */
> > +static int ethnl_perphy_dumpit(struct sk_buff *skb,
> > +			       struct netlink_callback *cb)
> > +{
> > +	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
> > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > +	int ret = 0;
> > +
> > +	if (ethnl_ctx->req_info->dev) {
> > +		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
> > +						ctx, genl_info_dump(cb));
> > +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> > +			ret = skb->len;
> > +
> > +		netdev_put(ethnl_ctx->req_info->dev,
> > +			   &ethnl_ctx->req_info->dev_tracker);  
> 
> You have to release this in .done
> dumpit gets called multiple times until we run out of objects to dump.
> OTOH user may close the socket without finishing the dump operation.
> So all .dumpit implementations must be "balanced". The only state we
> should touch in them is the dump context to know where to pick up from
> next time.

Sorry for the delayed answer, I'm still wrapping my head around all
this. calling netdev_put in dumpit() is not correct, but won't moving
this into .done() make us subject to the scenario you described to me
in another mail where userspace would stall the dump operation by not
calling recv() ?

Maybe the safest path, as you mention in the other thread, would be to
store the requesting ifindex in .start(), call netdev_put() in start()
as well, and hold/dump/put in each .dumpit() then.

Maxime

Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP operations

[Jakub Kicinski]

On Wed, 30 Apr 2025 08:32:20 +0200 Maxime Chevallier wrote:
> > > +/* perphy ->dumpit() handler for GET requests. */
> > > +static int ethnl_perphy_dumpit(struct sk_buff *skb,
> > > +			       struct netlink_callback *cb)
> > > +{
> > > +	struct ethnl_perphy_dump_ctx *ctx = ethnl_perphy_dump_context(cb);
> > > +	struct ethnl_dump_ctx *ethnl_ctx = &ctx->ethnl_ctx;
> > > +	int ret = 0;
> > > +
> > > +	if (ethnl_ctx->req_info->dev) {
> > > +		ret = ethnl_perphy_dump_one_dev(skb, ethnl_ctx->req_info->dev,
> > > +						ctx, genl_info_dump(cb));
> > > +		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
> > > +			ret = skb->len;
> > > +
> > > +		netdev_put(ethnl_ctx->req_info->dev,
> > > +			   &ethnl_ctx->req_info->dev_tracker);    
> > 
> > You have to release this in .done
> > dumpit gets called multiple times until we run out of objects to dump.
> > OTOH user may close the socket without finishing the dump operation.
> > So all .dumpit implementations must be "balanced". The only state we
> > should touch in them is the dump context to know where to pick up from
> > next time.  
> 
> Sorry for the delayed answer, I'm still wrapping my head around all
> this. calling netdev_put in dumpit() is not correct, but won't moving
> this into .done() make us subject to the scenario you described to me
> in another mail where userspace would stall the dump operation by not
> calling recv() ?

Good catch, for the filtered dump you'll need to look up the netdev
on every dumpit call.