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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250425171044.4a9e1b4a@kernel.org>
Date: Fri, 25 Apr 2025 17:10:44 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, Andrew Lunn <andrew@...n.ch>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Heiner Kallweit
 <hkallweit1@...il.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
 linux-arm-kernel@...ts.infradead.org, Christophe Leroy
 <christophe.leroy@...roup.eu>, Herve Codina <herve.codina@...tlin.com>,
 Florian Fainelli <f.fainelli@...il.com>, Russell King
 <linux@...linux.org.uk>, Vladimir Oltean <vladimir.oltean@....com>,
 Köry Maincent <kory.maincent@...tlin.com>, Oleksij Rempel
 <o.rempel@...gutronix.de>, Simon Horman <horms@...nel.org>, Romain Gantois
 <romain.gantois@...tlin.com>, Piergiorgio Beruto
 <piergiorgio.beruto@...il.com>
Subject: Re: [PATCH net-next v7 1/3] net: ethtool: Introduce per-PHY DUMP
 operations

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.