On Fri, 21 Mar 2025 17:31:17 +0100
Paolo Abeni [off-list ref] wrote:

On 3/13/25 7:26 PM, Maxime Chevallier wrote:

quoted

We have a number of netlink commands in the ethnl family that may have
multiple objects to dump even for a single net_device, including :

 - PLCA, PSE-PD, phy: one message per PHY device
 - tsinfo: one message per timestamp source (netdev + phys)
 - rss: One per RSS context

To get this behaviour, these netlink commands need to roll a custom  
->dumpit().  

To prepare making per-netdev DUMP more generic in ethnl, introduce a
member in the ethnl ops to indicate if a given command may allow
pernetdev DUMPs (also referred to as filtered DUMPs).

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/netlink.c | 45 +++++++++++++++++++++++++++++--------------
 net/ethtool/netlink.h |  2 ++
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index a163d40c6431..7adede5e4ff1 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c

@@ -587,21 +587,38 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
 	int ret = 0;
 
 	rcu_read_lock();  

Maintain the RCU read lock here is IMHO confusing...

quoted

-	for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
-		dev_hold(dev);
+	if (ctx->req_info->dev) {
+		dev = ctx->req_info->dev;  

.. as this is refcounted.

I suggest to move the rcu_read_lock inside the if.

Indeed, maybe not the best place indeed. I'll address that, thanks for
pointing this out

quoted

 		rcu_read_unlock();
+		/* Filtered DUMP request targeted to a single netdev. We already
+		 * hold a ref to the netdev from ->start()
+		 */
+		ret = ethnl_default_dump_one(skb, dev, ctx,
+					     genl_info_dump(cb));
+		rcu_read_lock();
+		netdev_put(ctx->req_info->dev, &ctx->req_info->dev_tracker);
 
-		ret = ethnl_default_dump_one(skb, dev, ctx, genl_info_dump(cb));
+		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
+			ret = skb->len;
 
-		rcu_read_lock();
-		dev_put(dev);
+	} else {
+		for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
+			dev_hold(dev);
+			rcu_read_unlock();
+
+			ret = ethnl_default_dump_one(skb, dev, ctx,
+						     genl_info_dump(cb));
+
+			rcu_read_lock();
+			dev_put(dev);
 
-		if (ret < 0 && ret != -EOPNOTSUPP) {
-			if (likely(skb->len))
-				ret = skb->len;
-			break;
+			if (ret < 0 && ret != -EOPNOTSUPP) {
+				if (likely(skb->len))
+					ret = skb->len;  

IMHO a bit too many levels of indentation. It's possibly better to move
this code in a separate helper.

That's true, not the prettiest piece of that patch. I'll refactor this
better then.

Thanks for the review,

Maxime