netdev - Re: [PATCH net-next v2 3/9] ethtool: add a new command for reading standard stats

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: <YHsXnzqVDjL9Q0Bz@shredder.lan>
Date:   Sat, 17 Apr 2021 20:15:11 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, andrew@...n.ch,
        mkubecek@...e.cz, idosch@...dia.com, saeedm@...dia.com,
        michael.chan@...adcom.com
Subject: Re: [PATCH net-next v2 3/9] ethtool: add a new command for reading
 standard stats

On Fri, Apr 16, 2021 at 12:27:39PM -0700, Jakub Kicinski wrote:
> Add an interface for reading standard stats, including
> stats which don't have a corresponding control interface.
> 
> Start with IEEE 802.3 PHY stats. There seems to be only
> one stat to expose there.
> 
> Define API to not require user space changes when new
> stats or groups are added. Groups are based on bitset,
> stats have a string set associated.

I tried to understand how you add new groups without user space
changes and I think this statement is not entirely accurate.

At minimum, user space needs to know the names of these groups, but
currently there is no way to query the information, so it's added to
ethtool's help message:

ethtool [ FLAGS ] -S|--statistics DEVNAME       Show adapter statistics       
       [ --groups [eth-phy] [eth-mac] [eth-ctrl] [rmon] ]

I was thinking about adding a new command (e.g.,
ETHTOOL_MSG_STATS_GROUP_GET) to query available groups, but maybe it's
an overkill. How about adding a new flag to ethtool:

ethtool [ FLAGS ] -S|--statistics DEVNAME       Show adapter statistics       
       [ { --groups [eth-phy] [eth-mac] [eth-ctrl] [rmon] | --all-groups } ]

Which will be a new flag attribute (e.g., ETHTOOL_A_STATS_ALL_GROUPS) in
ETHTOOL_MSG_STATS_GET. Kernel will validate that
ETHTOOL_A_STATS_ALL_GROUPS and ETHTOOL_A_STATS_GROUPS are not passed
together.

It's not the end of the world to leave it as-is, but the new flag will
indeed allow you to continue using your existing ethtool binary when
upgrading the kernel and still getting all the new stats.

Actually, if we ever get an exporter to query this information, it will
probably want to use the new flag instead of having to be patched
whenever a new group is added.