[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 at 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