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

[Jakub Kicinski <kuba@kernel.org>]

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.