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: <bwh3s7vcingnkhnnvucak656sj2u2vikwupysgihvfdcshixtf@nymosaa2eth6>
Date: Mon, 2 Sep 2024 00:04:39 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
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

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?

> @@ -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.

[...]
> diff --git a/netlink/msgbuff.c b/netlink/msgbuff.c
> index 216f5b9..2275840 100644
> --- a/netlink/msgbuff.c
> +++ b/netlink/msgbuff.c
> @@ -138,17 +138,9 @@ struct nlattr *ethnla_nest_start(struct nl_msg_buff *msgbuff, uint16_t type)
>  	return NULL;
>  }
>  
> -/**
> - * ethnla_fill_header() - write standard ethtool request header to message
> - * @msgbuff: message buffer
> - * @type:    attribute type for header nest
> - * @devname: device name (NULL to omit)
> - * @flags:   request flags (omitted if 0)
> - *
> - * Return: pointer to the nest attribute or null of error
> - */
> -bool ethnla_fill_header(struct nl_msg_buff *msgbuff, uint16_t type,
> -			const char *devname, uint32_t flags)
> +static bool __ethnla_fill_header_phy(struct nl_msg_buff *msgbuff, uint16_t type,
> +				     const char *devname, uint32_t phy_index,
> +				     uint32_t flags)
>  {
>  	struct nlattr *nest;
>  
> @@ -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)?

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)