| 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
| ||
|
Date: Sun, 26 Apr 2020 22:25:43 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: netdev@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Chris Healy <cphealy@...il.com>
Subject: Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable
test reports
On Sat, Apr 25, 2020 at 08:06:16PM +0200, Andrew Lunn wrote:
> Add the attributes needed to report cable test results to userspace.
> The reports are expected to be per twisted pair. A nested property per
> pair can report the result of the cable test. A nested property can
> also report the length of the cable to any fault.
>
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
> Documentation/networking/ethtool-netlink.rst | 35 +++++++++++++++
> include/uapi/linux/ethtool_netlink.h | 47 +++++++++++++++++++-
> 2 files changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 0c354567e991..89fd321b0e29 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -967,6 +967,41 @@ Request contents:
> ``ETHTOOL_A_CABLE_TEST_HEADER`` nested request header
> ==================================== ====== ==========================
>
> +Notify contents:
Perhaps rather "Notification contents".
> +
> +An Ethernet cable typically contains 1, 2 or 4 pairs. The length of
> +the pair can only be measured when there is a fault in the pair and
> +hence a reflection. Information about the fault may not be available,
> +depends on the specific hardware. Hence the contents of the notify
> +message is mostly optional. The attributes can be repeated an
> +arbitrary number of times, in an arbitrary order, for an arbitrary
> +number of pairs.
> +
> +The example shows a T2 cable, i.e. two pairs. One pair is O.K, and
> +hence has no length information. The second pair has a fault and does
> +have length information.
> +
> + +-------------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_HEADER`` | nested | reply header |
> + +-------------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT`` | nested | cable test result |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR`` | u8 | pair number |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_CODE`` | u8 | result code |
> + +-+-----------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT`` | nested | cable test results |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR`` | u8 | pair number |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_CODE`` | u8 | result code |
> + +-+-----------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR`` | u8 | pair number |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM`` | u8 | length in cm |
> + +-+-----------------------------------------+--------+-----------------------+
Would it be complicated to gather all information for each pair
together? I.e. to have one nest for each pair with pair number, result
code and possibly other information (if available). I believe it would
make the message easier to process.
>
> Request translation
> ===================
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 598d0b502ebd..05ef5048e4fc 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -76,6 +76,7 @@ enum {
> ETHTOOL_MSG_EEE_NTF,
> ETHTOOL_MSG_TSINFO_GET_REPLY,
> ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
> + ETHTOOL_MSG_CABLE_TEST_NTF,
>
> /* add new constants above here */
> __ETHTOOL_MSG_KERNEL_CNT,
> @@ -403,7 +404,6 @@ enum {
> /* add new constants above here */
> __ETHTOOL_A_TSINFO_CNT,
> ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
> -
> };
>
> /* CABLE TEST */
> @@ -417,6 +417,51 @@ enum {
> ETHTOOL_A_CABLE_TEST_MAX = __ETHTOOL_A_CABLE_TEST_CNT - 1
> };
>
> +/* CABLE TEST NOTIFY */
> +enum {
> + ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC,
> + ETHTOOL_A_CABLE_RESULT_CODE_OK,
> + ETHTOOL_A_CABLE_RESULT_CODE_OPEN,
> + ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT,
> + ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT,
> +};
> +
> +enum {
> + ETHTOOL_A_CABLE_PAIR_0,
> + ETHTOOL_A_CABLE_PAIR_1,
> + ETHTOOL_A_CABLE_PAIR_2,
> + ETHTOOL_A_CABLE_PAIR_3,
> +};
Do we really need this enum, couldn't we simply use a number (possibly
with a sanity check of maximum value)?
> +
> +enum {
> + ETHTOOL_A_CABLE_RESULT_UNSPEC,
> + ETHTOOL_A_CABLE_RESULT_PAIR, /* u8 ETHTOOL_A_CABLE_PAIR_ */
> + ETHTOOL_A_CABLE_RESULT_CODE, /* u8 ETHTOOL_A_CABLE_RESULT_CODE_ */
> +
> + __ETHTOOL_A_CABLE_RESULT_CNT,
> + ETHTOOL_A_CABLE_RESULT_MAX = (__ETHTOOL_A_CABLE_RESULT_CNT - 1)
> +};
> +
> +enum {
> + ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC,
> + ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR, /* u8 ETHTOOL_A_CABLE_PAIR_ */
> + ETHTOOL_A_CABLE_FAULT_LENGTH_CM, /* u16 */
The example above says "u8" (which is obviously wrong). I would rather
suggest u32 here to be as future proof as possible. Using NLA_U16
doesn't save anything after all, as both NLA_U16 and NLA_U32 spend the
same space due to padding.
> +
> + __ETHTOOL_A_CABLE_FAULT_LENGTH_CNT,
> + ETHTOOL_A_CABLE_FAULT_LENGTH_MAX = (__ETHTOOL_A_CABLE_FAULT_LENGTH_CNT - 1)
> +};
> +
> +enum {
> + ETHTOOL_A_CABLE_TEST_NTF_UNSPEC,
> + ETHTOOL_A_CABLE_TEST_NTF_DEV, /* nest - ETHTOOL_A_DEV_* */
s/DEV/HEADER/g, I suppose (looks like a relic from an older version).
> + ETHTOOL_A_CABLE_TEST_NTF_NEST, /* nest - of results: */
This is missing in the example above.
> + ETHTOOL_A_CABLE_TEST_NTF_RESULT, /* nest - ETHTOOL_A_CABLE_RESULT_ */
> + ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH, /* nest - ETHTOOL_A_CABLE_FAULT_LENGTH_ */
> +
> + __ETHTOOL_A_CABLE_TEST_NTF_CNT,
> + ETHTOOL_A_CABLE_TEST_NTF_MAX = (__ETHTOOL_A_CABLE_TEST_NTF_CNT - 1)
> +};
One more idea: it would be IMHO useful to also send a notification when
the test is started. It could be distinguished by a status attribute
which would describe status of the test as a whole (not a specific
pair), e.g. started, finished, aborted.
Michal
> +
> /* generic netlink info */
> #define ETHTOOL_GENL_NAME "ethtool"
> #define ETHTOOL_GENL_VERSION 1
> --
> 2.26.1
>
Powered by blists - more mailing lists