| 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: Tue, 26 Mar 2019 13:09:09 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
Jakub Kicinski <jakub.kicinski@...ronome.com>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
John Linville <linville@...driver.com>,
Stephen Hemminger <stephen@...workplumber.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink
interface
Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@...e.cz wrote:
>Basic genetlink and init infrastructure for the netlink interface, register
>genetlink family "ethtool". Introduce CONFIG_ETHTOOL_NETLINK Kconfig
>option. Add interface description into Documentation/networking.
>
>Signed-off-by: Michal Kubecek <mkubecek@...e.cz>
>---
> Documentation/networking/ethtool-netlink.txt | 168 +++++++++++++++++++
> include/linux/ethtool_netlink.h | 9 +
> include/uapi/linux/ethtool_netlink.h | 25 +++
> net/Kconfig | 8 +
> net/ethtool/Makefile | 6 +-
> net/ethtool/netlink.c | 34 ++++
> net/ethtool/netlink.h | 12 ++
> 7 files changed, 261 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/networking/ethtool-netlink.txt
> create mode 100644 include/linux/ethtool_netlink.h
> create mode 100644 include/uapi/linux/ethtool_netlink.h
> create mode 100644 net/ethtool/netlink.c
> create mode 100644 net/ethtool/netlink.h
>
>diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
>new file mode 100644
>index 000000000000..377d64c9b7fa
>--- /dev/null
>+++ b/Documentation/networking/ethtool-netlink.txt
>@@ -0,0 +1,168 @@
>+ Netlink interface for ethtool
>+ =============================
>+
>+
>+Basic information
>+-----------------
>+
>+Netlink interface for ethtool uses generic netlink family "ethtool" (userspace
>+application should use macros ETHTOOL_GENL_NAME and ETHTOOL_GENL_VERSION
>+defined in <linux/ethtool_netlink.h> uapi header). This family does not use
>+a specific header, all information in requests and replies is passed using
>+netlink attributes.
>+
>+In requests, device can be identified by ifindex or by name; if both are used,
>+they must match. In replies, kernel fills both. The meaning of flags,
>+info_mask and index fields depends on request type.
>+
>+The ethtool netlink interface uses extended ACK for error and warning
>+reporting, userspace application developers are encouraged to make these
>+messages available to user in a suitable way.
>+
>+Requests can be divided into three categories: "get" (retrieving information),
>+"set" (setting parameters) and "action" (invoking an action).
>+
>+All "set" and "action" type requests require admin privileges (CAP_NET_ADMIN
>+in the namespace). Most "get" type request are allowed for anyone but there
s/request/requests/
>+are exceptions (where the response contains sensitive information). In some
>+cases, the request as such is allowed for anyone but unprivileged users have
>+attributes with sensitive information (e.g. wake-on-lan password) omitted.
>+
>+
>+Conventions
>+-----------
>+
>+Attributes which represent a boolean value usually use u8 type so that we can
>+distinguish three states: "on", "off" and "not present" (meaning the
>+information is not available in "get" requests or value is not to be changed
>+in "set" requests). For these attributes, the "true" value should be passed as
>+number 1 but any non-zero value should be understood as "true" by recipient.
>+
>+Some request types allow passing an attribute named ETHA_*_INFOMASK with
>+a bitmask telling kernel that we are only interested in some parts of the
>+information. If info mask is omitted, all available information is returned.
>+Meaning of info mask bits depends on request type and is listed below.
>+
>+
>+Device identification
>+---------------------
>+
>+When appropriate, network device is identified by a nested attribute named
>+ETHA_*_DEV. This attribute can contain
Isn't it ETHA_DEV_*? I must admit I'm a bit confused.
>+
>+ ETHA_DEV_INDEX (u32) device ifindex
>+ ETHA_DEV_NAME (string) device name
>+
>+In device related requests, one of these is sufficient; if both are used, they
>+must match (i.e. identify the same device). In device related replies both are
You say this now for the second time. First time this was said in second
para.
>+provided by kernel. In dump requests, device is not specified and kernel
>+replies with one message per network device (only those for which the request
>+is supported).
>+
>+
>+List of message types
>+---------------------
>+
>+All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
Why "usually"? Why not "always"?
>+to indicate the type.
>+
>+Messages of type "get" are used by userspace to request information and
>+usually do not contain any attributes (that may be added later for dump
>+filtering). Kernel response is in the form of corresponding "set" message;
Okay. Do we want reply to "*_cmd_something_get" command to be
"*_cmd_something_set". That sounds odd. Why reply has to be "cmd"? Why
not something like "reply" or "response"?
This should work for both "doit/dumpit" and notifications.
>+the same message can be also used to set (some of) the parameters, except for
>+messages marked as "response only" in the table above. "Get" messages with
>+NLM_F_DUMP flags and no device identification dump the information for all
>+devices supporting the request.
>+
>+Later sections describe the format and semantics of these request messages.
>+
>+
>+Request translation
>+-------------------
>+
>+The following table maps iosctl commands to netlink commands providing their
>+functionality. Entries with "n/a" in right column are commands which do not
>+have their netlink replacement yet.
>+
>+ioctl command netlink command
>+---------------------------------------------------------------------
>+ETHTOOL_GSET n/a
>+ETHTOOL_SSET n/a
>+ETHTOOL_GDRVINFO n/a
>+ETHTOOL_GREGS n/a
>+ETHTOOL_GWOL n/a
>+ETHTOOL_SWOL n/a
>+ETHTOOL_GMSGLVL n/a
>+ETHTOOL_SMSGLVL n/a
>+ETHTOOL_NWAY_RST n/a
>+ETHTOOL_GLINK n/a
>+ETHTOOL_GEEPROM n/a
>+ETHTOOL_SEEPROM n/a
>+ETHTOOL_GCOALESCE n/a
>+ETHTOOL_SCOALESCE n/a
>+ETHTOOL_GRINGPARAM n/a
>+ETHTOOL_SRINGPARAM n/a
>+ETHTOOL_GPAUSEPARAM n/a
>+ETHTOOL_SPAUSEPARAM n/a
>+ETHTOOL_GRXCSUM n/a
>+ETHTOOL_SRXCSUM n/a
>+ETHTOOL_GTXCSUM n/a
>+ETHTOOL_STXCSUM n/a
>+ETHTOOL_GSG n/a
>+ETHTOOL_SSG n/a
>+ETHTOOL_TEST n/a
>+ETHTOOL_GSTRINGS n/a
>+ETHTOOL_PHYS_ID n/a
>+ETHTOOL_GSTATS n/a
>+ETHTOOL_GTSO n/a
>+ETHTOOL_STSO n/a
>+ETHTOOL_GPERMADDR rtnetlink RTM_GETLINK
>+ETHTOOL_GUFO n/a
>+ETHTOOL_SUFO n/a
>+ETHTOOL_GGSO n/a
>+ETHTOOL_SGSO n/a
>+ETHTOOL_GFLAGS n/a
>+ETHTOOL_SFLAGS n/a
>+ETHTOOL_GPFLAGS n/a
>+ETHTOOL_SPFLAGS n/a
>+ETHTOOL_GRXFH n/a
>+ETHTOOL_SRXFH n/a
>+ETHTOOL_GGRO n/a
>+ETHTOOL_SGRO n/a
>+ETHTOOL_GRXRINGS n/a
>+ETHTOOL_GRXCLSRLCNT n/a
>+ETHTOOL_GRXCLSRULE n/a
>+ETHTOOL_GRXCLSRLALL n/a
>+ETHTOOL_SRXCLSRLDEL n/a
>+ETHTOOL_SRXCLSRLINS n/a
>+ETHTOOL_FLASHDEV n/a
>+ETHTOOL_RESET n/a
>+ETHTOOL_SRXNTUPLE n/a
>+ETHTOOL_GRXNTUPLE n/a
>+ETHTOOL_GSSET_INFO n/a
>+ETHTOOL_GRXFHINDIR n/a
>+ETHTOOL_SRXFHINDIR n/a
>+ETHTOOL_GFEATURES n/a
>+ETHTOOL_SFEATURES n/a
>+ETHTOOL_GCHANNELS n/a
>+ETHTOOL_SCHANNELS n/a
>+ETHTOOL_SET_DUMP n/a
>+ETHTOOL_GET_DUMP_FLAG n/a
>+ETHTOOL_GET_DUMP_DATA n/a
>+ETHTOOL_GET_TS_INFO n/a
>+ETHTOOL_GMODULEINFO n/a
>+ETHTOOL_GMODULEEEPROM n/a
>+ETHTOOL_GEEE n/a
>+ETHTOOL_SEEE n/a
>+ETHTOOL_GRSSH n/a
>+ETHTOOL_SRSSH n/a
>+ETHTOOL_GTUNABLE n/a
>+ETHTOOL_STUNABLE n/a
>+ETHTOOL_GPHYSTATS n/a
>+ETHTOOL_PERQUEUE n/a
>+ETHTOOL_GLINKSETTINGS n/a
>+ETHTOOL_SLINKSETTINGS n/a
>+ETHTOOL_PHY_GTUNABLE n/a
>+ETHTOOL_PHY_STUNABLE n/a
>+ETHTOOL_GFECPARAM n/a
>+ETHTOOL_SFECPARAM n/a
>diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
>new file mode 100644
>index 000000000000..0412adb4f42f
>--- /dev/null
>+++ b/include/linux/ethtool_netlink.h
>@@ -0,0 +1,9 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+
>+#ifndef _LINUX_ETHTOOL_NETLINK_H_
>+#define _LINUX_ETHTOOL_NETLINK_H_
>+
>+#include <uapi/linux/ethtool_netlink.h>
>+#include <linux/ethtool.h>
>+
>+#endif /* _LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
>new file mode 100644
>index 000000000000..6aa267451542
>--- /dev/null
>+++ b/include/uapi/linux/ethtool_netlink.h
>@@ -0,0 +1,25 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+/*
>+ * include/uapi/linux/ethtool_netlink.h - netlink interface for ethtool
>+ *
>+ * See Documentation/networking/ethtool-netlink.txt in kernel source tree for
>+ * doucumentation of the interface.
>+ */
>+
>+#ifndef _UAPI_LINUX_ETHTOOL_NETLINK_H_
>+#define _UAPI_LINUX_ETHTOOL_NETLINK_H_
>+
>+#include <linux/ethtool.h>
>+
>+enum {
>+ ETHNL_CMD_NOOP,
>+
Usually headers have something like:
/* add new commands above here */
here.
>+ __ETHNL_CMD_CNT,
>+ ETHNL_CMD_MAX = (__ETHNL_CMD_CNT - 1)
>+};
>+
>+/* generic netlink info */
>+#define ETHTOOL_GENL_NAME "ethtool"
>+#define ETHTOOL_GENL_VERSION 1
>+
>+#endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/net/Kconfig b/net/Kconfig
>index 3e8fdd688329..75c600b45775 100644
>--- a/net/Kconfig
>+++ b/net/Kconfig
>@@ -448,6 +448,14 @@ config FAILOVER
> migration of VMs with direct attached VFs by failing over to the
> paravirtual datapath when the VF is unplugged.
>
>+config ETHTOOL_NETLINK
>+ bool "Netlink interface for ethtool"
>+ default y
>+ help
>+ An alternative userspace interface for ethtool based on generic
>+ netlink. It provides better extensibility and some new features,
>+ e.g. notification messages.
>+
> endif # if NET
>
> # Used by archs to tell that they support BPF JIT compiler plus which flavour.
>diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
>index 3ebfab2bca66..f30e0da88be5 100644
>--- a/net/ethtool/Makefile
>+++ b/net/ethtool/Makefile
>@@ -1,3 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
>-obj-y += ioctl.o
>+obj-y += ioctl.o
>+
>+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
Why? I believe this should be always build-in same as ioctl.
>+
>+ethtool_nl-y := netlink.o
>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>new file mode 100644
>index 000000000000..85dd6dac71a2
>--- /dev/null
>+++ b/net/ethtool/netlink.c
>@@ -0,0 +1,34 @@
>+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
>+
>+#include <linux/ethtool_netlink.h>
>+#include "netlink.h"
>+
>+/* genetlink setup */
>+
>+static const struct genl_ops ethtool_genl_ops[] = {
>+};
>+
>+struct genl_family ethtool_genl_family = {
>+ .hdrsize = 0,
No need to set 0.
>+ .name = ETHTOOL_GENL_NAME,
>+ .version = ETHTOOL_GENL_VERSION,
>+ .netnsok = true,
>+ .parallel_ops = true,
>+ .ops = ethtool_genl_ops,
>+ .n_ops = ARRAY_SIZE(ethtool_genl_ops),
>+};
>+
>+/* module setup */
>+
>+static int __init ethnl_init(void)
>+{
>+ int ret;
>+
>+ ret = genl_register_family(ðtool_genl_family);
>+ if (WARN(ret < 0, "ethtool: genetlink family registration failed"))
>+ return ret;
>+
>+ return 0;
>+}
>+
>+subsys_initcall(ethnl_init);
>diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
>new file mode 100644
>index 000000000000..63063b582ca2
>--- /dev/null
>+++ b/net/ethtool/netlink.h
>@@ -0,0 +1,12 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+
>+#ifndef _NET_ETHTOOL_NETLINK_H
>+#define _NET_ETHTOOL_NETLINK_H
>+
>+#include <linux/ethtool_netlink.h>
>+#include <linux/netdevice.h>
>+#include <net/genetlink.h>
>+
>+extern struct genl_family ethtool_genl_family;
Why? You need this just within "netlink.c", don't you?
>+
>+#endif /* _NET_ETHTOOL_NETLINK_H */
>--
>2.21.0
>
Powered by blists - more mailing lists