netdev - Re: [PATCH ethtool-next v2 2/3] ethtool: Allow passing a PHY index for phy-targetting commands

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240904194504.44071f11@fedora.home>
Date: Wed, 4 Sep 2024 19:45:04 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Russell King
 <linux@...linux.org.uk>, linux-arm-kernel@...ts.infradead.org, Christophe
 Leroy <christophe.leroy@...roup.eu>, Herve Codina
 <herve.codina@...tlin.com>, Florian Fainelli <f.fainelli@...il.com>, Heiner
 Kallweit <hkallweit1@...il.com>, Vladimir Oltean <vladimir.oltean@....com>,
 Köry Maincent <kory.maincent@...tlin.com>, Jonathan Corbet
 <corbet@....net>, Marek Behún <kabel@...nel.org>,
 Piergiorgio Beruto <piergiorgio.beruto@...il.com>, Oleksij Rempel
 <o.rempel@...gutronix.de>, Nicolò Veronese
 <nicveronese@...il.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH ethtool-next v2 2/3] ethtool: Allow passing a PHY index
 for phy-targetting commands

Hello Michal,

On Mon, 2 Sep 2024 00:04:39 +0200
Michal Kubecek <mkubecek@...e.cz> wrote:

> On Wed, Aug 28, 2024 at 05:25:09PM +0200, Maxime Chevallier wrote:
> > With the introduction of PHY topology and the ability to list PHYs, we
> > can now target some netlink commands to specific PHYs. This is done by
> > passing a PHY index as a request parameter in the netlink GET command.
> > 
> > This is useful for PSE-PD, PLCA and Cable-testing operations when
> > multiple PHYs are on the link (e.g. when a PHY is used as an SFP
> > upstream controller, and when there's another PHY within the SFP
> > module).
> > 
> > Introduce a new, generic, option "--phy N" that can be used in
> > conjunction with PHY-targetting commands to pass the PHY index for the
> > targetted PHY.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> > ---
> >  ethtool.8.in         | 20 +++++++++++++++++
> >  ethtool.c            | 25 ++++++++++++++++++++-
> >  internal.h           |  1 +
> >  netlink/cable_test.c |  4 ++--
> >  netlink/msgbuff.c    | 52 ++++++++++++++++++++++++++++++++++----------
> >  netlink/msgbuff.h    |  3 +++
> >  netlink/nlsock.c     |  3 ++-
> >  netlink/plca.c       |  4 ++--
> >  netlink/pse-pd.c     |  4 ++--
> >  9 files changed, 96 insertions(+), 20 deletions(-)
> >   
> [...]
> > @@ -6550,6 +6559,16 @@ int main(int argc, char **argp)
> >  			argc -= 1;
> >  			continue;
> >  		}
> > +		if (*argp && !strcmp(*argp, "--phy")) {
> > +			char *eptr;
> > +
> > +			ctx.phy_index = strtoul(argp[1], &eptr, 0);
> > +			if (!argp[1][0] || *eptr)
> > +				exit_bad_args();
> > +			argp += 2;
> > +			argc -= 2;
> > +			continue;
> > +		}
> >  		break;
> >  	}
> >  	if (*argp && !strcmp(*argp, "--monitor")) {  
> 
> Could we have a meaningful error message that would tell user what was
> wrong instead?

Good point, I'll add one.

> 
> > @@ -6585,6 +6604,10 @@ int main(int argc, char **argp)
> >  	}
> >  	if (ctx.json && !args[k].json)
> >  		exit_bad_args_info("JSON output not available for this subcommand");
> > +
> > +	if (!args[k].targets_phy && ctx.phy_index)
> > +		exit_bad_args();
> > +
> >  	ctx.argc = argc;
> >  	ctx.argp = argp;
> >  	netlink_run_handler(&ctx, args[k].nlchk, args[k].nlfunc, !args[k].func);  
> 
> Same here.

Indeed, I'll update accordingly.

[...]

> > @@ -159,7 +151,9 @@ bool ethnla_fill_header(struct nl_msg_buff *msgbuff, uint16_t type,
> >  	if ((devname &&
> >  	     ethnla_put_strz(msgbuff, ETHTOOL_A_HEADER_DEV_NAME, devname)) ||
> >  	    (flags &&
> > -	     ethnla_put_u32(msgbuff, ETHTOOL_A_HEADER_FLAGS, flags)))
> > +	     ethnla_put_u32(msgbuff, ETHTOOL_A_HEADER_FLAGS, flags)) ||
> > +	    (phy_index &&
> > +	     ethnla_put_u32(msgbuff, ETHTOOL_A_HEADER_PHY_INDEX, phy_index)))
> >  		goto err;
> >  
> >  	ethnla_nest_end(msgbuff, nest);  
> 
> Just to be sure: are we sure the PHY index cannot ever be zero (or that
> we won't need to pass 0 index to kernel)?

I should better document that, sorry... The phy index assigned starts
at 1 at wraps-around back to 1 when we exhausted the indices, so it
can't ever be 0.

The netlink code in the kernel side interprets the fact that userspace
passes 0 as "use the default PHY", as if you didn't pass the parameter :

net/ethtool/netlink.c:

	if (!req_info->phy_index)
		return req_info->dev->phydev;

I'll send a patch to document that behaviour, thanks for pointing this
out.

Maxime