netdev - Re: [PATCH ethtool 3/3] netlink: settings: expand linkstate_reply

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]

Date:   Tue, 30 Jun 2020 19:09:26 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Amit Cohen <amitc@...lanox.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        o.rempel@...gutronix.de, andrew@...n.ch, f.fainelli@...il.com,
        jacob.e.keller@...el.com, mlxsw@...lanox.com
Subject: Re: [PATCH ethtool 3/3] netlink: settings: expand
 linkstate_reply_cb() to support link extended state

On Tue, Jun 30, 2020 at 12:24:12PM +0300, Amit Cohen wrote:
> Print extended state in addition to link state.
> 
> In case that extended state is not provided, print state only.
> If extended substate is provided in addition to the extended state,
> print it also.
> 
> Signed-off-by: Amit Cohen <amitc@...lanox.com>
> ---
>  netlink/settings.c | 59 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 35ba2f5..a4d1908 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -604,6 +604,57 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  	return MNL_CB_OK;
>  }
>  
> +int linkstate_link_ext_substate_print(const struct nlattr *tb[],
> +				      struct nl_context *nlctx, uint8_t link_val,
> +				      uint8_t link_ext_state_val,
> +				      const char *link_ext_state_str)
> +{
> +	uint8_t link_ext_substate_val;
> +	const char *link_ext_substate_str;
> +
> +	if (!tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE])
> +		return -ENODATA;
> +
> +	link_ext_substate_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE]);
> +
> +	link_ext_substate_str = link_ext_substate_get(link_ext_state_val, link_ext_substate_val);
> +	if (!link_ext_substate_str)
> +		return -ENODATA;

This does not distinguish between missing attribute (substate not
provided by kernel) and unknown value which can happen when older
ethtool version is used on a newer kernel which returns a new substate
not known by ethtool. IMHO we should fall back to reporting the numeric
value in such case rather than behaving as if the information were not
provided.

> +
> +	print_banner(nlctx);
> +	printf("\tLink detected: %s (%s, %s)\n", link_val ? "yes" : "no",
> +	       link_ext_state_str, link_ext_substate_str);
> +
> +	return 0;
> +}
> +
> +int linkstate_link_ext_state_print(const struct nlattr *tb[],
> +				   struct nl_context *nlctx, uint8_t link_val)
> +{
> +	uint8_t link_ext_state_val;
> +	const char *link_ext_state_str;
> +	int ret;
> +
> +	if (!tb[ETHTOOL_A_LINKSTATE_EXT_STATE])
> +		return -ENODATA;
> +
> +	link_ext_state_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_STATE]);
> +
> +	link_ext_state_str = link_ext_state_get(link_ext_state_val);
> +	if (!link_ext_state_str)
> +		return -ENODATA;

The same problem as above.

> +
> +	ret = linkstate_link_ext_substate_print(tb, nlctx, link_val, link_ext_state_val,
> +						link_ext_state_str);
> +	if (ret < 0) {
> +		print_banner(nlctx);
> +		printf("\tLink detected: %s (%s)\n", link_val ? "yes" : "no",
> +		       link_ext_state_str);
> +	}
> +
> +	return 0;
> +}
> +
>  int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  {
>  	const struct nlattr *tb[ETHTOOL_A_LINKSTATE_MAX + 1] = {};
> @@ -622,9 +673,11 @@ int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  
>  	if (tb[ETHTOOL_A_LINKSTATE_LINK]) {
>  		uint8_t val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_LINK]);
> -
> -		print_banner(nlctx);
> -		printf("\tLink detected: %s\n", val ? "yes" : "no");
> +		ret = linkstate_link_ext_state_print(tb, nlctx, val);
> +		if (ret < 0) {
> +			print_banner(nlctx);
> +			printf("\tLink detected: %s\n", val ? "yes" : "no");
> +		}
>  	}
>  
>  	if (tb[ETHTOOL_A_LINKSTATE_SQI]) {

It's a bit impractical to have the banner and first part of the line
printed in three differente places. How about this:

  - linkstate_reply_cb() calls print_banner() and prints what it does
    now, only without the newline
  - linkstate_link_ext_state_print() prints " (%s" or " (%u" with
    extended state if it is provided or bails out if not
  - linkstate_link_ext_substate_print() prints ", %s" or ", %u" with
    extended substate if it is provided
  - linkstate_link_ext_state_print() prints ")"
  - linkstate_reply_cb() prints the newline

Michal