[net-next,v5,05/22] ethtool: introduce ethtool netlink interface

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

Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.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@suse.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(&ethtool_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
>

On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:
> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
> >+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/

Will fix.

> >+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 is the nesting attribute (e.g. ETHA_SETTINGS_DEV), ETHA_DEV_*
(ETHA_DEV_INDEX and ETHA_DEV_NAME) are in the nest.

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

I'll drop one of them.

> >+List of message types
> >+---------------------
> >+
> >+All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
> 
> Why "usually"? Why not "always"?

Right, it's always. And if it changes one day, the sentence will have to
be rewritten anyway.

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

As stated right below, the aim is to use the same format for replies to
GET requests as userspace uses for related SET requests. We could use
different id (genlmsghdr::cmd) but that seemed like a waste for no actual
gain.

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

> >+
> >+enum {
> >+	ETHNL_CMD_NOOP,
> >+
> 
> Usually headers have something like:
> /* add new commands above here */
> here.

OK

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

I would like to make the ioctl interface optional as well, eventually.
As someone noted in one of the earlier discussions, there may be some
special minimalistic setups where ethtool interface may be of no use.

> >+struct genl_family ethtool_genl_family = {
> >+	.hdrsize	= 0,
> 
> No need to set 0.

OK

> >+
> >+extern struct genl_family ethtool_genl_family;
> 
> Why? You need this just within "netlink.c", don't you?

In the submitted part, yes. But one of the later patches adds specific
notify handler (different from ethnl_std_notify()) which is not in
netlink.c and needs to use pointer to ethtool_genl_family for a call to
genlmsg_put() and genlmsg_multicast().

But I can make it static for now and change to extern when it's needed.

Michal

Tue, Mar 26, 2019 at 02:24:27PM CET, mkubecek@suse.cz wrote:
>On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:
>> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
>> >+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/
>
>Will fix.
>
>> >+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 is the nesting attribute (e.g. ETHA_SETTINGS_DEV), ETHA_DEV_*
>(ETHA_DEV_INDEX and ETHA_DEV_NAME) are in the nest.

Yeah. I wonder why you need to duplicate this. Can this be in top-lever
attr enum that is shared among all commands? It is there anyway and
looks a bit silly to have "DEV" attr separate for every command.
Something like this:

ATTR_IFINDEX
ATTR_IFNAME
ATTR_SOMEOTHER (flags perhaps)
ATTR_CMD_SPECIFIC_NEST_START
  ATTR_CMDX_SOMETHING
  ATTR_CMDX_SOMETHING2
  ATTR_CMDX_SOMETHING3
ATTR_CMD_SPECIFIC_NEST_END

>
>> 
>> 
>> >+
>> >+    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.
>
>I'll drop one of them.
>
>> >+List of message types
>> >+---------------------
>> >+
>> >+All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
>> 
>> Why "usually"? Why not "always"?
>
>Right, it's always. And if it changes one day, the sentence will have to
>be rewritten anyway.

Okay.


>
>> >+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.
>
>As stated right below, the aim is to use the same format for replies to
>GET requests as userspace uses for related SET requests. We could use
>different id (genlmsghdr::cmd) but that seemed like a waste for no actual
>gain.

I understand. I just wonder if the replies/notifications could use the
same name, not having "set" in it. I know we have it like this in many
netlink ifaces, it is however confusing to users. So once we are doing
this from scratch, we can do it differently.


>
>> >+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.
>
>> >+
>> >+enum {
>> >+	ETHNL_CMD_NOOP,
>> >+
>> 
>> Usually headers have something like:
>> /* add new commands above here */
>> here.
>
>OK
>
>> >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.
>
>I would like to make the ioctl interface optional as well, eventually.
>As someone noted in one of the earlier discussions, there may be some
>special minimalistic setups where ethtool interface may be of no use.

Okay, fair enough.


>
>> >+struct genl_family ethtool_genl_family = {
>> >+	.hdrsize	= 0,
>> 
>> No need to set 0.
>
>OK
>
>> >+
>> >+extern struct genl_family ethtool_genl_family;
>> 
>> Why? You need this just within "netlink.c", don't you?
>
>In the submitted part, yes. But one of the later patches adds specific
>notify handler (different from ethnl_std_notify()) which is not in
>netlink.c and needs to use pointer to ethtool_genl_family for a call to
>genlmsg_put() and genlmsg_multicast().
>
>But I can make it static for now and change to extern when it's needed.

Please do.


>
>Michal
>

Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.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@suse.cz>
>---

[...]


>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[] = {

Please be consistent with prefixes. Either use "ethtool_" or "ethnl_"
for all functions and variables in this code.


>+};
>+
>+struct genl_family ethtool_genl_family = {
>+	.hdrsize	= 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(&ethtool_genl_family);
>+	if (WARN(ret < 0, "ethtool: genetlink family registration failed"))

Why do you need this warning? Please avoid it.


>+		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;
>+
>+#endif /* _NET_ETHTOOL_NETLINK_H */
>-- 
>2.21.0
>

On Tue, Mar 26, 2019 at 05:36:40PM +0100, Jiri Pirko wrote:
> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
> >+/* genetlink setup */
> >+
> >+static const struct genl_ops ethtool_genl_ops[] = {
> 
> Please be consistent with prefixes. Either use "ethtool_" or "ethnl_"
> for all functions and variables in this code.

OK

> >+/* module setup */
> >+
> >+static int __init ethnl_init(void)
> >+{
> >+	int ret;
> >+
> >+	ret = genl_register_family(&ethtool_genl_family);
> >+	if (WARN(ret < 0, "ethtool: genetlink family registration failed"))
> 
> Why do you need this warning? Please avoid it.

I'm confused now... few days ago you replied "+1" to the idea:

  http://lkml.kernel.org/r/20190321162105.GU2087@nanopsycho

I agreed that panic() (which is what e.g. rtnetlink does) would be an
overkill but I would be definitely opposed to not having anything in the
log at all and just silently going on without the interface (which may
result in misconfigured network). I believe that if this fails, it is
a sign of something going very wrong inside the kernel so that the "W"
taint flag would be appropriate.

Michal

Tue, Mar 26, 2019 at 06:32:15PM CET, mkubecek@suse.cz wrote:
>On Tue, Mar 26, 2019 at 05:36:40PM +0100, Jiri Pirko wrote:
>> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
>> >+/* genetlink setup */
>> >+
>> >+static const struct genl_ops ethtool_genl_ops[] = {
>> 
>> Please be consistent with prefixes. Either use "ethtool_" or "ethnl_"
>> for all functions and variables in this code.
>
>OK
>
>> >+/* module setup */
>> >+
>> >+static int __init ethnl_init(void)
>> >+{
>> >+	int ret;
>> >+
>> >+	ret = genl_register_family(&ethtool_genl_family);
>> >+	if (WARN(ret < 0, "ethtool: genetlink family registration failed"))
>> 
>> Why do you need this warning? Please avoid it.
>
>I'm confused now... few days ago you replied "+1" to the idea:
>
>  http://lkml.kernel.org/r/20190321162105.GU2087@nanopsycho
>
>I agreed that panic() (which is what e.g. rtnetlink does) would be an
>overkill but I would be definitely opposed to not having anything in the
>log at all and just silently going on without the interface (which may
>result in misconfigured network). I believe that if this fails, it is
>a sign of something going very wrong inside the kernel so that the "W"
>taint flag would be appropriate.

Okay. Fair. It's just that I looked over the code and did not find any
other genl_register_family() call with WARN.


>
>Michal

On Tue, Mar 26, 2019 at 02:42:51PM +0100, Jiri Pirko wrote:
> Tue, Mar 26, 2019 at 02:24:27PM CET, mkubecek@suse.cz wrote:
> >On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:
> >> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
> >> >+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 is the nesting attribute (e.g. ETHA_SETTINGS_DEV), ETHA_DEV_*
> >(ETHA_DEV_INDEX and ETHA_DEV_NAME) are in the nest.
> 
> Yeah. I wonder why you need to duplicate this. Can this be in top-lever
> attr enum that is shared among all commands? It is there anyway and
> looks a bit silly to have "DEV" attr separate for every command.
> Something like this:
> 
> ATTR_IFINDEX
> ATTR_IFNAME
> ATTR_SOMEOTHER (flags perhaps)
> ATTR_CMD_SPECIFIC_NEST_START
>   ATTR_CMDX_SOMETHING
>   ATTR_CMDX_SOMETHING2
>   ATTR_CMDX_SOMETHING3
> ATTR_CMD_SPECIFIC_NEST_END

I would rather prefer the opposite:

ATTR_HEADER
    ATTR_IFINDEX
    ATTR_IFNAME
    ATTR_INFO_MASK
    ATTR_PER_QUEUE
ATTR_CMDX_SOMETHING
ATTR_CMDX_SOMETHING2
ATTR_CMDX_SOMETHING3
...

This way the "header" with universal attributes (not specfic to
a particular message type) would be kept together at the beginning even
after we need to add some more later and command specific attributes
would still have fixed numbers (starting from 2). I'll think about it
some more and check what would be pros and cons of the two variants
when parsing and generating the messages.

> >> >+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.
> >
> >As stated right below, the aim is to use the same format for replies to
> >GET requests as userspace uses for related SET requests. We could use
> >different id (genlmsghdr::cmd) but that seemed like a waste for no actual
> >gain.
> 
> I understand. I just wonder if the replies/notifications could use the
> same name, not having "set" in it. I know we have it like this in many
> netlink ifaces, it is however confusing to users. So once we are doing
> this from scratch, we can do it differently.

How about

  ETHTOOL_MSG_GET_FOO  for get requests
  ETHTOOL_MSG_FOO      for get replies, notifications and set requests
  ETHTOOL_MSG_ACT_FOO  for actions (renegotiation, reset, blinking, ...)

?

Michal

Wed, Mar 27, 2019 at 10:26:04AM CET, mkubecek@suse.cz wrote:
>On Tue, Mar 26, 2019 at 02:42:51PM +0100, Jiri Pirko wrote:
>> Tue, Mar 26, 2019 at 02:24:27PM CET, mkubecek@suse.cz wrote:
>> >On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:
>> >> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
>> >> >+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 is the nesting attribute (e.g. ETHA_SETTINGS_DEV), ETHA_DEV_*
>> >(ETHA_DEV_INDEX and ETHA_DEV_NAME) are in the nest.
>> 
>> Yeah. I wonder why you need to duplicate this. Can this be in top-lever
>> attr enum that is shared among all commands? It is there anyway and
>> looks a bit silly to have "DEV" attr separate for every command.
>> Something like this:
>> 
>> ATTR_IFINDEX
>> ATTR_IFNAME
>> ATTR_SOMEOTHER (flags perhaps)
>> ATTR_CMD_SPECIFIC_NEST_START
>>   ATTR_CMDX_SOMETHING
>>   ATTR_CMDX_SOMETHING2
>>   ATTR_CMDX_SOMETHING3
>> ATTR_CMD_SPECIFIC_NEST_END
>
>I would rather prefer the opposite:
>
>ATTR_HEADER
>    ATTR_IFINDEX
>    ATTR_IFNAME
>    ATTR_INFO_MASK
>    ATTR_PER_QUEUE
>ATTR_CMDX_SOMETHING
>ATTR_CMDX_SOMETHING2
>ATTR_CMDX_SOMETHING3
>...
>
>This way the "header" with universal attributes (not specfic to
>a particular message type) would be kept together at the beginning even
>after we need to add some more later and command specific attributes
>would still have fixed numbers (starting from 2). I'll think about it
>some more and check what would be pros and cons of the two variants
>when parsing and generating the messages.

Okay, so what you suggest is per-cmd top level attr enum. That leads to
duplications of common attributes:
You would have to have HEADER attr defined in every cmd enum:

enum cmdx {
ATTR_CMDX_HEADER
ATTR_CMDX_SOMETHING
ATTR_CMDX_SOMETHING2
ATTR_CMDX_SOMETHING3
};

enum cmdy {
ATTR_CMDY_HEADER
ATTR_CMDY_SOMETHING
ATTR_CMDY_SOMETHING2
ATTR_CMDY_SOMETHING3
};

That is odd. TC has it and I hate it there :)

I think that the rtnetlink example is better. The generic things are in
generic top level enum. Then you have IFLA_LINKINFO with per-type enums.

>
>> >> >+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.
>> >
>> >As stated right below, the aim is to use the same format for replies to
>> >GET requests as userspace uses for related SET requests. We could use
>> >different id (genlmsghdr::cmd) but that seemed like a waste for no actual
>> >gain.
>> 
>> I understand. I just wonder if the replies/notifications could use the
>> same name, not having "set" in it. I know we have it like this in many
>> netlink ifaces, it is however confusing to users. So once we are doing
>> this from scratch, we can do it differently.
>
>How about
>
>  ETHTOOL_MSG_GET_FOO  for get requests
>  ETHTOOL_MSG_FOO      for get replies, notifications and set requests
>  ETHTOOL_MSG_ACT_FOO  for actions (renegotiation, reset, blinking, ...)
>
>?

Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
ordering of words thought, but it is cosmetics:
ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
ETHTOOL_MSG_FOO_GET
ETHTOOL_MSG_FOO_SET
ETHTOOL_MSG_FOO_ACT

What do you think?

>
>Michal

On 3/27/2019 2:50 AM, Jiri Pirko wrote:
> 
> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
> ordering of words thought, but it is cosmetics:
> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
> ETHTOOL_MSG_FOO_GET
> ETHTOOL_MSG_FOO_SET
> ETHTOOL_MSG_FOO_ACT
> 
> What do you think?

We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.

Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com wrote:
>
>
>On 3/27/2019 2:50 AM, Jiri Pirko wrote:
>> 
>> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
>> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
>> ordering of words thought, but it is cosmetics:
>> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
>> ETHTOOL_MSG_FOO_GET
>> ETHTOOL_MSG_FOO_SET
>> ETHTOOL_MSG_FOO_ACT
>> 
>> What do you think?
>
>We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
>or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.

Sound good. Something like:

ETHTOOL_MSG_FOO_GET
ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
ETHTOOL_MSG_FOO_SET
ETHTOOL_MSG_FOO_ACT
ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */

On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:
> Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com wrote:
> >
> >
> >On 3/27/2019 2:50 AM, Jiri Pirko wrote:
> >> 
> >> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
> >> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
> >> ordering of words thought, but it is cosmetics:
> >> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
> >> ETHTOOL_MSG_FOO_GET
> >> ETHTOOL_MSG_FOO_SET
> >> ETHTOOL_MSG_FOO_ACT
> >> 
> >> What do you think?
> >
> >We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
> >or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.
> 
> Sound good. Something like:
> 
> ETHTOOL_MSG_FOO_GET
> ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
> ETHTOOL_MSG_FOO_SET
> ETHTOOL_MSG_FOO_ACT
> ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */

The names sound fine to me and having different message ids would still
allow processing messages by the same handler easily.

But there is one potential issue I would like to point out: this way we
spend 4 message ids for a get/set pair rather than 2. These message ids
(genlmsghdr::cmd) are u8, i.e. the resource is not as infinite as one
would wish. There are 80 ioctl commands (43 "get" and 29 "set") at the
moment.

Netlink API should be less greedy in general. I already combined some
ioctl commands into one netlink request type and with an easy way to add
new attributes to existing commands, we won't need to add new commands
as often (certainly not in a way which left us with 9 "get" and 9 "set"
ioctl commands for netdev features). So even with 4 ids per get/set
pair, we might be safe for reasonably long time. But it's still
something to keep in mind.

Michal

Thu, Mar 28, 2019 at 10:37:46AM CET, mkubecek@suse.cz wrote:
>On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:
>> Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com wrote:
>> >
>> >
>> >On 3/27/2019 2:50 AM, Jiri Pirko wrote:
>> >> 
>> >> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
>> >> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
>> >> ordering of words thought, but it is cosmetics:
>> >> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
>> >> ETHTOOL_MSG_FOO_GET
>> >> ETHTOOL_MSG_FOO_SET
>> >> ETHTOOL_MSG_FOO_ACT
>> >> 
>> >> What do you think?
>> >
>> >We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
>> >or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.
>> 
>> Sound good. Something like:
>> 
>> ETHTOOL_MSG_FOO_GET
>> ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
>> ETHTOOL_MSG_FOO_SET
>> ETHTOOL_MSG_FOO_ACT
>> ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */
>
>The names sound fine to me and having different message ids would still
>allow processing messages by the same handler easily.
>
>But there is one potential issue I would like to point out: this way we
>spend 4 message ids for a get/set pair rather than 2. These message ids
>(genlmsghdr::cmd) are u8, i.e. the resource is not as infinite as one
>would wish. There are 80 ioctl commands (43 "get" and 29 "set") at the
>moment.
>
>Netlink API should be less greedy in general. I already combined some
>ioctl commands into one netlink request type and with an easy way to add
>new attributes to existing commands, we won't need to add new commands
>as often (certainly not in a way which left us with 9 "get" and 9 "set"
>ioctl commands for netdev features). So even with 4 ids per get/set
>pair, we might be safe for reasonably long time. But it's still
>something to keep in mind.

There are still 16 bits reserve in genl msg header:
struct genlmsghdr {
        __u8    cmd;
        __u8    version;
        __u16   reserved;
};

On 3/28/19 6:23 AM, Jiri Pirko wrote:
> Thu, Mar 28, 2019 at 10:37:46AM CET, mkubecek@suse.cz wrote:
>> On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:
>>> Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com wrote:
>>>>
>>>>
>>>> On 3/27/2019 2:50 AM, Jiri Pirko wrote:
>>>>>
>>>>> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
>>>>> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
>>>>> ordering of words thought, but it is cosmetics:
>>>>> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
>>>>> ETHTOOL_MSG_FOO_GET
>>>>> ETHTOOL_MSG_FOO_SET
>>>>> ETHTOOL_MSG_FOO_ACT
>>>>>
>>>>> What do you think?
>>>>
>>>> We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
>>>> or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.
>>>
>>> Sound good. Something like:
>>>
>>> ETHTOOL_MSG_FOO_GET
>>> ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
>>> ETHTOOL_MSG_FOO_SET
>>> ETHTOOL_MSG_FOO_ACT
>>> ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */
>>
>> The names sound fine to me and having different message ids would still
>> allow processing messages by the same handler easily.
>>
>> But there is one potential issue I would like to point out: this way we
>> spend 4 message ids for a get/set pair rather than 2. These message ids
>> (genlmsghdr::cmd) are u8, i.e. the resource is not as infinite as one
>> would wish. There are 80 ioctl commands (43 "get" and 29 "set") at the
>> moment.
>>
>> Netlink API should be less greedy in general. I already combined some
>> ioctl commands into one netlink request type and with an easy way to add
>> new attributes to existing commands, we won't need to add new commands
>> as often (certainly not in a way which left us with 9 "get" and 9 "set"
>> ioctl commands for netdev features). So even with 4 ids per get/set
>> pair, we might be safe for reasonably long time. But it's still
>> something to keep in mind.
> 
> There are still 16 bits reserve in genl msg header:
> struct genlmsghdr {
>         __u8    cmd;
>         __u8    version;
>         __u16   reserved;
> };
> 

And you know not all message IDs will be making sense depending on the
direction, so aliasing specific message IDs to an existing value should
be fine?

Thu, Mar 28, 2019 at 06:00:20PM CET, f.fainelli@gmail.com wrote:
>On 3/28/19 6:23 AM, Jiri Pirko wrote:
>> Thu, Mar 28, 2019 at 10:37:46AM CET, mkubecek@suse.cz wrote:
>>> On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:
>>>> Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com wrote:
>>>>>
>>>>>
>>>>> On 3/27/2019 2:50 AM, Jiri Pirko wrote:
>>>>>>
>>>>>> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
>>>>>> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
>>>>>> ordering of words thought, but it is cosmetics:
>>>>>> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
>>>>>> ETHTOOL_MSG_FOO_GET
>>>>>> ETHTOOL_MSG_FOO_SET
>>>>>> ETHTOOL_MSG_FOO_ACT
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
>>>>> or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.
>>>>
>>>> Sound good. Something like:
>>>>
>>>> ETHTOOL_MSG_FOO_GET
>>>> ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
>>>> ETHTOOL_MSG_FOO_SET
>>>> ETHTOOL_MSG_FOO_ACT
>>>> ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */
>>>
>>> The names sound fine to me and having different message ids would still
>>> allow processing messages by the same handler easily.
>>>
>>> But there is one potential issue I would like to point out: this way we
>>> spend 4 message ids for a get/set pair rather than 2. These message ids
>>> (genlmsghdr::cmd) are u8, i.e. the resource is not as infinite as one
>>> would wish. There are 80 ioctl commands (43 "get" and 29 "set") at the
>>> moment.
>>>
>>> Netlink API should be less greedy in general. I already combined some
>>> ioctl commands into one netlink request type and with an easy way to add
>>> new attributes to existing commands, we won't need to add new commands
>>> as often (certainly not in a way which left us with 9 "get" and 9 "set"
>>> ioctl commands for netdev features). So even with 4 ids per get/set
>>> pair, we might be safe for reasonably long time. But it's still
>>> something to keep in mind.
>> 
>> There are still 16 bits reserve in genl msg header:
>> struct genlmsghdr {
>>         __u8    cmd;
>>         __u8    version;
>>         __u16   reserved;
>> };
>> 
>
>And you know not all message IDs will be making sense depending on the
>direction, so aliasing specific message IDs to an existing value should
>be fine?

You are right, good idea. There can be 2 enums one for in, one for out.