[PATCH net-next v3 04/11] netlink: specs: add specification for wireguard

Asbjørn Sloth Tønnesen posted 11 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Asbjørn Sloth Tønnesen 1 month, 1 week ago
This patch adds an near[1] complete YNL specification for wireguard,
documenting the protocol in a machine-readable format, than the
comment in wireguard.h, and eases usage from C and non-C programming
languages alike.

The generated C library will be featured in the next patch, so in
this patch I will use the in-kernel python client for examples.

This makes the documentation in the UAPI header redundant, and
it is therefore removed. The in-line documentation in the spec,
is based on the existing comment in wireguard.h, and once released
then it will be available in the kernel documentation at:
  https://docs.kernel.org/netlink/specs/wireguard.html
  (until then run: make htmldocs)

Generate wireguard.rst from this spec:
$ make -C tools/net/ynl/generated/ wireguard.rst

Query wireguard interface through pyynl:
$ sudo ./tools/net/ynl/pyynl/cli.py --family wireguard \
                                    --dump get-device \
                                    --json '{"ifindex":3}'
[{'fwmark': 0,
  'ifindex': 3,
  'ifname': 'wg-test',
  'listen-port': 54318,
  'peers': [{0: {'allowedips': [{0: {'cidr-mask': 0,
                                     'family': 2,
                                     'ipaddr': '0.0.0.0'}},
                                {0: {'cidr-mask': 0,
                                     'family': 10,
                                     'ipaddr': '::'}}],
                 'endpoint': b'[...]',
                 'last-handshake-time': {'nsec': 42, 'sec': 42},
                 'persistent-keepalive-interval': 42,
                 'preshared-key': '[...]',
                 'protocol-version': 1,
                 'public-key': '[...]',
                 'rx-bytes': 42,
                 'tx-bytes': 42}}],
  'private-key': '[...]',
  'public-key': '[...]'}]

Add another allowed IP prefix:
$ sudo ./tools/net/ynl/pyynl/cli.py --family wireguard \
  --do set-device --json '{"ifindex":3,"peers":[
    {"public-key":"6a df b1 83 a4 ..","allowedips":[
      {"cidr-mask":0,"family":10,"ipaddr":"::"}]}]}'

[1] As can be seen above, the "endpoint" is only decoded as binary data,
    as it can't be described fully in YNL. It's a struct sockaddr_in or
    struct sockaddr_in6 depending on the attribute length.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 Documentation/netlink/specs/wireguard.yaml | 301 +++++++++++++++++++++
 MAINTAINERS                                |   1 +
 include/uapi/linux/wireguard.h             | 129 ---------
 3 files changed, 302 insertions(+), 129 deletions(-)
 create mode 100644 Documentation/netlink/specs/wireguard.yaml

diff --git a/Documentation/netlink/specs/wireguard.yaml b/Documentation/netlink/specs/wireguard.yaml
new file mode 100644
index 000000000000..7dcb2dec3d06
--- /dev/null
+++ b/Documentation/netlink/specs/wireguard.yaml
@@ -0,0 +1,301 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+---
+name: wireguard
+protocol: genetlink-legacy
+
+doc: |
+  **Netlink protocol to control WireGuard network devices.**
+
+  The below enums and macros are for interfacing with WireGuard,
+  using generic netlink, with family ``WG_GENL_NAME`` and version
+  ``WG_GENL_VERSION``. It defines two commands: get and set.
+  Note that while they share many common attributes, these two
+  commands actually accept a slightly different set of inputs and
+  outputs. These differences are noted under the individual attributes.
+c-family-name: wg-genl-name
+c-version-name: wg-genl-version
+max-by-define: true
+
+definitions:
+  -
+    name-prefix: wg-
+    name: key-len
+    type: const
+    value: 32
+  -
+    name: --kernel-timespec
+    type: struct
+    header: linux/time_types.h
+    members:
+      -
+        name: sec
+        type: u64
+        doc: Number of seconds, since UNIX epoch.
+      -
+        name: nsec
+        type: u64
+        doc: Number of nanoseconds, after the second began.
+  -
+    name: wgdevice-flags
+    name-prefix: wgdevice-f-
+    enum-name: wgdevice-flag
+    type: flags
+    entries:
+      - replace-peers
+  -
+    name: wgpeer-flags
+    name-prefix: wgpeer-f-
+    enum-name: wgpeer-flag
+    type: flags
+    entries:
+      - remove-me
+      - replace-allowedips
+      - update-only
+  -
+    name: wgallowedip-flags
+    name-prefix: wgallowedip-f-
+    enum-name: wgallowedip-flag
+    type: flags
+    entries:
+      - remove-me
+
+attribute-sets:
+  -
+    name: wgdevice
+    enum-name: wgdevice-attribute
+    name-prefix: wgdevice-a-
+    attr-cnt-name: --wgdevice-a-last
+    attributes:
+      -
+        name: unspec
+        type: unused
+        value: 0
+      -
+        name: ifindex
+        type: u32
+      -
+        name: ifname
+        type: string
+        checks:
+          max-len: 15
+      -
+        name: private-key
+        type: binary
+        doc: Set to all zeros to remove.
+        display-hint: hex
+        checks:
+          exact-len: wg-key-len
+      -
+        name: public-key
+        type: binary
+        display-hint: hex
+        checks:
+          exact-len: wg-key-len
+      -
+        name: flags
+        type: u32
+        doc: |
+          ``0`` or ``WGDEVICE_F_REPLACE_PEERS`` if all current peers
+          should be removed prior to adding the list below.
+        enum: wgdevice-flags
+      -
+        name: listen-port
+        type: u16
+        doc: Set as ``0`` to choose randomly.
+      -
+        name: fwmark
+        type: u32
+        doc: Set as ``0`` to disable.
+      -
+        name: peers
+        type: indexed-array
+        sub-type: nest
+        nested-attributes: wgpeer
+        doc: The index is set as ``0`` in ``DUMP``, and unused in ``DO``.
+  -
+    name: wgpeer
+    enum-name: wgpeer-attribute
+    name-prefix: wgpeer-a-
+    attr-cnt-name: --wgpeer-a-last
+    attributes:
+      -
+        name: unspec
+        type: unused
+        value: 0
+      -
+        name: public-key
+        type: binary
+        display-hint: hex
+        checks:
+          exact-len: wg-key-len
+      -
+        name: preshared-key
+        type: binary
+        doc: Set as all zeros to remove.
+        display-hint: hex
+        checks:
+          exact-len: wg-key-len
+      -
+        name: flags
+        type: u32
+        doc: |
+          ``0`` and/or ``WGPEER_F_REMOVE_ME`` if the specified peer should not
+          exist at the end of the operation, rather than added/updated
+          and/or ``WGPEER_F_REPLACE_ALLOWEDIPS`` if all current allowed IPs
+          of this peer should be removed prior to adding the list below
+          and/or ``WGPEER_F_UPDATE_ONLY`` if the peer should only be set if
+          it already exists.
+        enum: wgpeer-flags
+      -
+        name: endpoint
+        type: binary
+        doc: struct sockaddr_in or struct sockaddr_in6
+        checks:
+          min-len: 16
+      -
+        name: persistent-keepalive-interval
+        type: u16
+        doc: Set as ``0`` to disable.
+      -
+        name: last-handshake-time
+        type: binary
+        struct: --kernel-timespec
+        checks:
+          exact-len: 16
+      -
+        name: rx-bytes
+        type: u64
+      -
+        name: tx-bytes
+        type: u64
+      -
+        name: allowedips
+        type: indexed-array
+        sub-type: nest
+        nested-attributes: wgallowedip
+        doc: The index is set as ``0`` in ``DUMP``, and unused in ``DO``.
+      -
+        name: protocol-version
+        type: u32
+        doc: |
+          Should not be set or used at all by most users of this API,
+          as the most recent protocol will be used when this is unset.
+          Otherwise, must be set to ``1``.
+  -
+    name: wgallowedip
+    enum-name: wgallowedip-attribute
+    name-prefix: wgallowedip-a-
+    attr-cnt-name: --wgallowedip-a-last
+    attributes:
+      -
+        name: unspec
+        type: unused
+        value: 0
+      -
+        name: family
+        type: u16
+        doc: IP family, either ``AF_INET`` or ``AF_INET6``.
+      -
+        name: ipaddr
+        type: binary
+        doc: Either ``struct in_addr`` or ``struct in6_addr``.
+        display-hint: ipv4-or-v6
+        checks:
+          min-len: 4
+      -
+        name: cidr-mask
+        type: u8
+      -
+        name: flags
+        type: u32
+        doc: |
+          ``WGALLOWEDIP_F_REMOVE_ME`` if the specified IP should be
+          removed; otherwise, this IP will be added if it is not
+          already present.
+        enum: wgallowedip-flags
+
+operations:
+  enum-name: wg-cmd
+  name-prefix: wg-cmd-
+  list:
+    -
+      name: get-device
+      value: 0
+      doc: |
+        Retrieve WireGuard device
+        ~~~~~~~~~~~~~~~~~~~~~~~~~
+
+        The command should be called with one but not both of:
+
+        - ``WGDEVICE_A_IFINDEX``
+        - ``WGDEVICE_A_IFNAME``
+
+        The kernel will then return several messages (``NLM_F_MULTI``).
+        It is possible that all of the allowed IPs of a single peer
+        will not fit within a single netlink message. In that case, the
+        same peer will be written in the following message, except it will
+        only contain ``WGPEER_A_PUBLIC_KEY`` and ``WGPEER_A_ALLOWEDIPS``.
+        This may occur several times in a row for the same peer.
+        It is then up to the receiver to coalesce adjacent peers.
+        Likewise, it is possible that all peers will not fit within a
+        single message.
+        So, subsequent peers will be sent in following messages,
+        except those will only contain ``WGDEVICE_A_IFNAME`` and
+        ``WGDEVICE_A_PEERS``. It is then up to the receiver to coalesce
+        these messages to form the complete list of peers.
+
+        While this command does accept the other ``WGDEVICE_A_*``
+        attributes, for compatibility reasons, but they are ignored
+        by this command, and should not be used in requests.
+
+        Since this is an ``NLA_F_DUMP`` command, the final message will
+        always be ``NLMSG_DONE``, even if an error occurs. However, this
+        ``NLMSG_DONE`` message contains an integer error code. It is
+        either zero or a negative error code corresponding to the errno.
+      attribute-set: wgdevice
+      flags: [uns-admin-perm]
+
+      dump:
+        pre: wireguard-nl-get-device-start
+        post: wireguard-nl-get-device-done
+        # request only uses ifindex | ifname, but keep .maxattr as is
+        request: &all-attrs
+          attributes:
+            - ifindex
+            - ifname
+            - private-key
+            - public-key
+            - flags
+            - listen-port
+            - fwmark
+            - peers
+        reply: *all-attrs
+    -
+      name: set-device
+      value: 1
+      doc: |
+        Set WireGuard device
+        ~~~~~~~~~~~~~~~~~~~~
+
+        This command should be called with a wgdevice set, containing one
+        but not both of ``WGDEVICE_A_IFINDEX`` and ``WGDEVICE_A_IFNAME``.
+
+        It is possible that the amount of configuration data exceeds that
+        of the maximum message length accepted by the kernel.
+        In that case, several messages should be sent one after another,
+        with each successive one filling in information not contained in
+        the prior.
+        Note that if ``WGDEVICE_F_REPLACE_PEERS`` is specified in the first
+        message, it probably should not be specified in fragments that come
+        after, so that the list of peers is only cleared the first time but
+        appended after.
+        Likewise for peers, if ``WGPEER_F_REPLACE_ALLOWEDIPS`` is specified
+        in the first message of a peer, it likely should not be specified
+        in subsequent fragments.
+
+        If an error occurs, ``NLMSG_ERROR`` will reply containing an errno.
+      attribute-set: wgdevice
+      flags: [uns-admin-perm]
+
+      do:
+        request: *all-attrs
diff --git a/MAINTAINERS b/MAINTAINERS
index bdf0a3a0dd36..35cd289899f7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -27649,6 +27649,7 @@ M:	Jason A. Donenfeld <Jason@zx2c4.com>
 L:	wireguard@lists.zx2c4.com
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/netlink/specs/wireguard.yaml
 F:	drivers/net/wireguard/
 F:	tools/testing/selftests/wireguard/
 
diff --git a/include/uapi/linux/wireguard.h b/include/uapi/linux/wireguard.h
index 8c26391196d5..dee4401e0b5d 100644
--- a/include/uapi/linux/wireguard.h
+++ b/include/uapi/linux/wireguard.h
@@ -1,135 +1,6 @@
 /* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR MIT */
 /*
  * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
- *
- * Documentation
- * =============
- *
- * The below enums and macros are for interfacing with WireGuard, using generic
- * netlink, with family WG_GENL_NAME and version WG_GENL_VERSION. It defines two
- * methods: get and set. Note that while they share many common attributes,
- * these two functions actually accept a slightly different set of inputs and
- * outputs.
- *
- * WG_CMD_GET_DEVICE
- * -----------------
- *
- * May only be called via NLM_F_REQUEST | NLM_F_DUMP. The command should contain
- * one but not both of:
- *
- *    WGDEVICE_A_IFINDEX: NLA_U32
- *    WGDEVICE_A_IFNAME: NLA_NUL_STRING, maxlen IFNAMSIZ - 1
- *
- * The kernel will then return several messages (NLM_F_MULTI) containing the
- * following tree of nested items:
- *
- *    WGDEVICE_A_IFINDEX: NLA_U32
- *    WGDEVICE_A_IFNAME: NLA_NUL_STRING, maxlen IFNAMSIZ - 1
- *    WGDEVICE_A_PRIVATE_KEY: NLA_EXACT_LEN, len WG_KEY_LEN
- *    WGDEVICE_A_PUBLIC_KEY: NLA_EXACT_LEN, len WG_KEY_LEN
- *    WGDEVICE_A_LISTEN_PORT: NLA_U16
- *    WGDEVICE_A_FWMARK: NLA_U32
- *    WGDEVICE_A_PEERS: NLA_NESTED
- *        0: NLA_NESTED
- *            WGPEER_A_PUBLIC_KEY: NLA_EXACT_LEN, len WG_KEY_LEN
- *            WGPEER_A_PRESHARED_KEY: NLA_EXACT_LEN, len WG_KEY_LEN
- *            WGPEER_A_ENDPOINT: NLA_MIN_LEN(struct sockaddr), struct sockaddr_in or struct sockaddr_in6
- *            WGPEER_A_PERSISTENT_KEEPALIVE_INTERVAL: NLA_U16
- *            WGPEER_A_LAST_HANDSHAKE_TIME: NLA_EXACT_LEN, struct __kernel_timespec
- *            WGPEER_A_RX_BYTES: NLA_U64
- *            WGPEER_A_TX_BYTES: NLA_U64
- *            WGPEER_A_ALLOWEDIPS: NLA_NESTED
- *                0: NLA_NESTED
- *                    WGALLOWEDIP_A_FAMILY: NLA_U16
- *                    WGALLOWEDIP_A_IPADDR: NLA_MIN_LEN(struct in_addr), struct in_addr or struct in6_addr
- *                    WGALLOWEDIP_A_CIDR_MASK: NLA_U8
- *                0: NLA_NESTED
- *                    ...
- *                0: NLA_NESTED
- *                    ...
- *                ...
- *            WGPEER_A_PROTOCOL_VERSION: NLA_U32
- *        0: NLA_NESTED
- *            ...
- *        ...
- *
- * It is possible that all of the allowed IPs of a single peer will not
- * fit within a single netlink message. In that case, the same peer will
- * be written in the following message, except it will only contain
- * WGPEER_A_PUBLIC_KEY and WGPEER_A_ALLOWEDIPS. This may occur several
- * times in a row for the same peer. It is then up to the receiver to
- * coalesce adjacent peers. Likewise, it is possible that all peers will
- * not fit within a single message. So, subsequent peers will be sent
- * in following messages, except those will only contain WGDEVICE_A_IFNAME
- * and WGDEVICE_A_PEERS. It is then up to the receiver to coalesce these
- * messages to form the complete list of peers.
- *
- * Since this is an NLA_F_DUMP command, the final message will always be
- * NLMSG_DONE, even if an error occurs. However, this NLMSG_DONE message
- * contains an integer error code. It is either zero or a negative error
- * code corresponding to the errno.
- *
- * WG_CMD_SET_DEVICE
- * -----------------
- *
- * May only be called via NLM_F_REQUEST. The command should contain the
- * following tree of nested items, containing one but not both of
- * WGDEVICE_A_IFINDEX and WGDEVICE_A_IFNAME:
- *
- *    WGDEVICE_A_IFINDEX: NLA_U32
- *    WGDEVICE_A_IFNAME: NLA_NUL_STRING, maxlen IFNAMSIZ - 1
- *    WGDEVICE_A_FLAGS: NLA_U32, 0 or WGDEVICE_F_REPLACE_PEERS if all current
- *                      peers should be removed prior to adding the list below.
- *    WGDEVICE_A_PRIVATE_KEY: len WG_KEY_LEN, all zeros to remove
- *    WGDEVICE_A_LISTEN_PORT: NLA_U16, 0 to choose randomly
- *    WGDEVICE_A_FWMARK: NLA_U32, 0 to disable
- *    WGDEVICE_A_PEERS: NLA_NESTED
- *        0: NLA_NESTED
- *            WGPEER_A_PUBLIC_KEY: len WG_KEY_LEN
- *            WGPEER_A_FLAGS: NLA_U32, 0 and/or WGPEER_F_REMOVE_ME if the
- *                            specified peer should not exist at the end of the
- *                            operation, rather than added/updated and/or
- *                            WGPEER_F_REPLACE_ALLOWEDIPS if all current allowed
- *                            IPs of this peer should be removed prior to adding
- *                            the list below and/or WGPEER_F_UPDATE_ONLY if the
- *                            peer should only be set if it already exists.
- *            WGPEER_A_PRESHARED_KEY: len WG_KEY_LEN, all zeros to remove
- *            WGPEER_A_ENDPOINT: struct sockaddr_in or struct sockaddr_in6
- *            WGPEER_A_PERSISTENT_KEEPALIVE_INTERVAL: NLA_U16, 0 to disable
- *            WGPEER_A_ALLOWEDIPS: NLA_NESTED
- *                0: NLA_NESTED
- *                    WGALLOWEDIP_A_FAMILY: NLA_U16
- *                    WGALLOWEDIP_A_IPADDR: struct in_addr or struct in6_addr
- *                    WGALLOWEDIP_A_CIDR_MASK: NLA_U8
- *                    WGALLOWEDIP_A_FLAGS: NLA_U32, WGALLOWEDIP_F_REMOVE_ME if
- *                                         the specified IP should be removed;
- *                                         otherwise, this IP will be added if
- *                                         it is not already present.
- *                0: NLA_NESTED
- *                    ...
- *                0: NLA_NESTED
- *                    ...
- *                ...
- *            WGPEER_A_PROTOCOL_VERSION: NLA_U32, should not be set or used at
- *                                       all by most users of this API, as the
- *                                       most recent protocol will be used when
- *                                       this is unset. Otherwise, must be set
- *                                       to 1.
- *        0: NLA_NESTED
- *            ...
- *        ...
- *
- * It is possible that the amount of configuration data exceeds that of
- * the maximum message length accepted by the kernel. In that case, several
- * messages should be sent one after another, with each successive one
- * filling in information not contained in the prior. Note that if
- * WGDEVICE_F_REPLACE_PEERS is specified in the first message, it probably
- * should not be specified in fragments that come after, so that the list
- * of peers is only cleared the first time but appended after. Likewise for
- * peers, if WGPEER_F_REPLACE_ALLOWEDIPS is specified in the first message
- * of a peer, it likely should not be specified in subsequent fragments.
- *
- * If an error occurs, NLMSG_ERROR will reply containing an errno.
  */
 
 #ifndef _WG_UAPI_WIREGUARD_H
-- 
2.51.0

Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jason A. Donenfeld 1 month ago
On Wed, Nov 05, 2025 at 06:32:13PM +0000, Asbjørn Sloth Tønnesen wrote:
> +name: wireguard
> +protocol: genetlink-legacy

I'll need to do my own reading, I guess, but what is going on with this
"legacy" business? Is there some newer genetlink that falls outside of
versioning?

> +c-family-name: wg-genl-name
> +c-version-name: wg-genl-version
> +max-by-define: true
> +
> +definitions:
> +  -
> +    name-prefix: wg-

There's lots of control over the C output here. Why can't there also be
a top-level c-function-prefix attribute, so that patch 10/11 is
unnecessary? Stack traces for wireguard all include wg_; why pollute
this with the new "wireguard_" ones?

> +        doc: The index is set as ``0`` in ``DUMP``, and unused in ``DO``.

I think get/set might match the operations better than the underlying
netlink verbs? This is a doc comment specific to getting and setting.

On the other hand, maybe that's an implementation detail and doesn't
need to be specified? Or if you think rigidity is important, we should
specify 0 in both directions and then validate it to ensure userspace
sends 0 (all userspaces currently do).

> +        The kernel will then return several messages (``NLM_F_MULTI``).
> +        It is possible that all of the allowed IPs of a single peer
> +        will not fit within a single netlink message. In that case, the
> +        same peer will be written in the following message, except it will
> +        only contain ``WGPEER_A_PUBLIC_KEY`` and ``WGPEER_A_ALLOWEDIPS``.
> +        This may occur several times in a row for the same peer.
> +        It is then up to the receiver to coalesce adjacent peers.
> +        Likewise, it is possible that all peers will not fit within a
> +        single message.
> +        So, subsequent peers will be sent in following messages,
> +        except those will only contain ``WGDEVICE_A_IFNAME`` and
> +        ``WGDEVICE_A_PEERS``. It is then up to the receiver to coalesce
> +        these messages to form the complete list of peers.

There's an extra line break before the "So," that wasn't there in the
original.


> +        While this command does accept the other ``WGDEVICE_A_*``
> +        attributes, for compatibility reasons, but they are ignored
> +        by this command, and should not be used in requests.

Either "While" or ", but" but not both.

However, can we actually just make this strict? No userspaces send
random attributes in a GET. Nothing should break.

> +      dump:
> +        pre: wireguard-nl-get-device-start
> +        post: wireguard-nl-get-device-done

Oh, or, the wg_ prefix can be defined here (instead of wireguard_, per
my 10/11 comment above).

> +        # request only uses ifindex | ifname, but keep .maxattr as is
> +        request: &all-attrs
> +          attributes:
> +            - ifindex
> +            - ifname
> +            - private-key
> +            - public-key
> +            - flags
> +            - listen-port
> +            - fwmark
> +            - peers

As mentioned earlier, maybe this can be reduced to ifindex+ifname.

> +        It is possible that the amount of configuration data exceeds that
> +        of the maximum message length accepted by the kernel.
> +        In that case, several messages should be sent one after another,
> +        with each successive one filling in information not contained in
> +        the prior.
> +        Note that if ``WGDEVICE_F_REPLACE_PEERS`` is specified in the first
> +        message, it probably should not be specified in fragments that come
> +        after, so that the list of peers is only cleared the first time but
> +        appended after.
> +        Likewise for peers, if ``WGPEER_F_REPLACE_ALLOWEDIPS`` is specified
> +        in the first message of a peer, it likely should not be specified
> +        in subsequent fragments.

More odd linebreaks added. Either it's a new paragraph or it isn't. Keep
this how it was before?

Jason
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Asbjørn Sloth Tønnesen 1 month ago
Hi Jason,

Thanks for the review.

On 11/18/25 2:15 AM, Jason A. Donenfeld wrote:
> On Wed, Nov 05, 2025 at 06:32:13PM +0000, Asbjørn Sloth Tønnesen wrote:
>> +name: wireguard
>> +protocol: genetlink-legacy
> 
> I'll need to do my own reading, I guess, but what is going on with this
> "legacy" business? Is there some newer genetlink that falls outside of
> versioning?

There's a few reasons why the stricter genetlink doesn't fit:
- Less flexible with C naming (breaking UAPI).
- Doesn't allow C struct types.

diff -Naur Documentation/netlink/genetlink{,-legacy}.yaml
>> +c-family-name: wg-genl-name
>> +c-version-name: wg-genl-version
>> +max-by-define: true
>> +
>> +definitions:
>> +  -
>> +    name-prefix: wg-
> 
> There's lots of control over the C output here. Why can't there also be
> a top-level c-function-prefix attribute, so that patch 10/11 is
> unnecessary? Stack traces for wireguard all include wg_; why pollute
> this with the new "wireguard_" ones?

It could also be just "c-prefix".

Jakub, WDYT?

>> +        doc: The index is set as ``0`` in ``DUMP``, and unused in ``DO``.
> 
> I think get/set might match the operations better than the underlying
> netlink verbs? This is a doc comment specific to getting and setting.

Sure, I could change that. Originally opted for the C-style, as I kept the
C-style from your original text in the rest of the doc comments.

> On the other hand, maybe that's an implementation detail and doesn't
> need to be specified? Or if you think rigidity is important, we should
> specify 0 in both directions and then validate it to ensure userspace
> sends 0 (all userspaces currently do).

As is, the YNL-based clients are taking advantage of it not being validated.
Changing that would require adding some new YNL type properties.
See this series[1] for my earlier attempt to extend YNL in this area.

[1] https://lore.kernel.org/r/20251022182701.250897-1-ast@fiberby.net/

The modern way would be to use multi-attrs, but I don't think it's worth it
to transition, you mainly save a few bytes of overhead.

WGDEVICE_A_IFINDEX
WGDEVICE_A_PEERS2: NLA_NESTED
   WGPEER_A_PUBLIC_KEY
   [..]
   WGPEER_A_ALLOWEDIPS2: NLA_NESTED
     WGALLOWEDIP_A_FAMILY
     [..]
   WGPEER_A_ALLOWEDIPS2: NLA_NESTED
     WGALLOWEDIP_A_FAMILY
     [..]
WGDEVICE_A_PEERS2: NLA_NESTED
   [..]

>> +        The kernel will then return several messages (``NLM_F_MULTI``).
>> +        It is possible that all of the allowed IPs of a single peer
>> +        will not fit within a single netlink message. In that case, the
>> +        same peer will be written in the following message, except it will
>> +        only contain ``WGPEER_A_PUBLIC_KEY`` and ``WGPEER_A_ALLOWEDIPS``.
>> +        This may occur several times in a row for the same peer.
>> +        It is then up to the receiver to coalesce adjacent peers.
>> +        Likewise, it is possible that all peers will not fit within a
>> +        single message.
>> +        So, subsequent peers will be sent in following messages,
>> +        except those will only contain ``WGDEVICE_A_IFNAME`` and
>> +        ``WGDEVICE_A_PEERS``. It is then up to the receiver to coalesce
>> +        these messages to form the complete list of peers.
> 
> There's an extra line break before the "So," that wasn't there in the
> original.

It's collapsed when rendered, added them to reduce diff stat for future changes.
A new paragraph in .rst requires a double line break, but I can remove them.

>> +        While this command does accept the other ``WGDEVICE_A_*``
>> +        attributes, for compatibility reasons, but they are ignored
>> +        by this command, and should not be used in requests.
> 
> Either "While" or ", but" but not both.
> 
> However, can we actually just make this strict? No userspaces send
> random attributes in a GET. Nothing should break.

I agree that nothing should break, just tried to avoid changing UAPI in the
spec commit, but by moving the split ops conversion patch, then I can eliminate
this before adding the spec.

>> +      dump:
>> +        pre: wireguard-nl-get-device-start
>> +        post: wireguard-nl-get-device-done
> 
> Oh, or, the wg_ prefix can be defined here (instead of wireguard_, per
> my 10/11 comment above).

The key here is the missing ones, I renamed these for alignment with
doit and dumpit which can't be customized at this time.
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jason A. Donenfeld 1 month ago
Hi Asbjørn,

On Tue, Nov 18, 2025 at 12:08:20PM +0000, Asbjørn Sloth Tønnesen wrote:
> > I'll need to do my own reading, I guess, but what is going on with this
> > "legacy" business? Is there some newer genetlink that falls outside of
> > versioning?
> 
> There's a few reasons why the stricter genetlink doesn't fit:
> - Less flexible with C naming (breaking UAPI).
> - Doesn't allow C struct types.
> 
> diff -Naur Documentation/netlink/genetlink{,-legacy}.yaml

Oh, thanks, useful diff. So the protocol didn't change, persay, but the
non-legacy one just firms up some of the floppiness of what was done
before. Makes sense.

> > There's lots of control over the C output here. Why can't there also be
> > a top-level c-function-prefix attribute, so that patch 10/11 is
> > unnecessary? Stack traces for wireguard all include wg_; why pollute
> > this with the new "wireguard_" ones?
> 
> It could also be just "c-prefix".

Works for me.

> >> +      dump:
> >> +        pre: wireguard-nl-get-device-start
> >> +        post: wireguard-nl-get-device-done
> > 
> > Oh, or, the wg_ prefix can be defined here (instead of wireguard_, per
> > my 10/11 comment above).
> 
> The key here is the missing ones, I renamed these for alignment with
> doit and dumpit which can't be customized at this time.

Oh, interesting. So actually, the c-prefix thing would let you ditch
this too, and it'd be more consistent.

> > On the other hand, maybe that's an implementation detail and doesn't
> > need to be specified? Or if you think rigidity is important, we should
> > specify 0 in both directions and then validate it to ensure userspace
> > sends 0 (all userspaces currently do).
> 
> As is, the YNL-based clients are taking advantage of it not being validated.
> Changing that would require adding some new YNL type properties.
> See this series[1] for my earlier attempt to extend YNL in this area.
> 
> [1] https://lore.kernel.org/r/20251022182701.250897-1-ast@fiberby.net/
> 
> The modern way would be to use multi-attrs, but I don't think it's worth it
> to transition, you mainly save a few bytes of overhead.

Oh, huh, interesting. In libmnl, this parameter is referred to as "type"
instead of index, so it was natural to stick 0 there. It looks like so
does Go's netlink library, but in wgctrl-go, Matt stuck index there.

So... darn. We're stuck with this being poorly defined. I guess we can
have as the text something like:

    The index/type parameter is unused on SET_DEVICE operations and is zero on GET_DEVICE operations.

> WGDEVICE_A_IFINDEX
> WGDEVICE_A_PEERS2: NLA_NESTED
>    WGPEER_A_PUBLIC_KEY
>    [..]
>    WGPEER_A_ALLOWEDIPS2: NLA_NESTED
>      WGALLOWEDIP_A_FAMILY
>      [..]
>    WGPEER_A_ALLOWEDIPS2: NLA_NESTED
>      WGALLOWEDIP_A_FAMILY
>      [..]
> WGDEVICE_A_PEERS2: NLA_NESTED
>    [..]

Def not worth it. But good to know about for future protocols.

> >> +        While this command does accept the other ``WGDEVICE_A_*``
> >> +        attributes, for compatibility reasons, but they are ignored
> >> +        by this command, and should not be used in requests.
> > 
> > Either "While" or ", but" but not both.
> > 
> > However, can we actually just make this strict? No userspaces send
> > random attributes in a GET. Nothing should break.
> 
> I agree that nothing should break, just tried to avoid changing UAPI in the
> spec commit, but by moving the split ops conversion patch, then I can eliminate
> this before adding the spec.

Okay, great, let's do that.

Thanks a bunch.

Jason
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Asbjørn Sloth Tønnesen 1 month ago
On 11/18/25 3:07 PM, Jason A. Donenfeld wrote:
> On Tue, Nov 18, 2025 at 12:08:20PM +0000, Asbjørn Sloth Tønnesen wrote:
>>> There's lots of control over the C output here. Why can't there also be
>>> a top-level c-function-prefix attribute, so that patch 10/11 is
>>> unnecessary? Stack traces for wireguard all include wg_; why pollute
>>> this with the new "wireguard_" ones?
>>
>> It could also be just "c-prefix".
> 
> Works for me.

Unfortunately, it isn't that simple.

The functions are defined as:
name = c_lower(f"{family.ident_name}-nl-{op_name}-doit")
name = c_lower(f"{family.ident_name}-nl-{op_name}-dumpit")
and
name = c_lower(f"{family.ident_name}-nl-{op_name}-{op_mode}it")

The "c-prefix" would replace "family.ident_name" aka. "wireguard",
but the "-nl-" would remain, which isn't in the current naming.

So "c-function-prefix" or something might work better.

My idea with "c-prefix" was to also cover the family and version defines,
but they are eg. WG_GENL_NAME where the default would be *_FAMILY_NAME.

>>>> +      dump:
>>>> +        pre: wireguard-nl-get-device-start
>>>> +        post: wireguard-nl-get-device-done
>>>
>>> Oh, or, the wg_ prefix can be defined here (instead of wireguard_, per
>>> my 10/11 comment above).
>>
>> The key here is the missing ones, I renamed these for alignment with
>> doit and dumpit which can't be customized at this time.
> 
> Oh, interesting. So actually, the c-prefix thing would let you ditch
> this too, and it'd be more consistent.

The pre and post still needs to be defined as they aren't used by default.
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jason A. Donenfeld 1 month ago
On Tue, Nov 18, 2025 at 09:59:45PM +0000, Asbjørn Sloth Tønnesen wrote:
> So "c-function-prefix" or something might work better.

Also fine with me. I'd just like consistent function naming, one way or
another.
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jakub Kicinski 1 month ago
On Tue, 18 Nov 2025 23:52:30 +0100 Jason A. Donenfeld wrote:
> On Tue, Nov 18, 2025 at 09:59:45PM +0000, Asbjørn Sloth Tønnesen wrote:
> > So "c-function-prefix" or something might work better.  
> 
> Also fine with me. I'd just like consistent function naming, one way or
> another.

IIUC we're talking about the prefix for the kernel C codegen?
Feels a bit like a one-off feature to me, but if we care deeply about
it let's add it as a CLI param to the codegen. I don't think it's
necessary to include this in the YAML spec.
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Asbjørn Sloth Tønnesen 4 weeks, 1 day ago
On 11/19/25 12:50 AM, Jakub Kicinski wrote:
> On Tue, 18 Nov 2025 23:52:30 +0100 Jason A. Donenfeld wrote:
>> On Tue, Nov 18, 2025 at 09:59:45PM +0000, Asbjørn Sloth Tønnesen wrote:
>>> So "c-function-prefix" or something might work better.
>>
>> Also fine with me. I'd just like consistent function naming, one way or
>> another.
> 
> IIUC we're talking about the prefix for the kernel C codegen?
> Feels a bit like a one-off feature to me, but if we care deeply about
> it let's add it as a CLI param to the codegen. I don't think it's
> necessary to include this in the YAML spec.

IIUC then adding it as a CLI param is more work, and just moves family details
to ynl-regen, might as well skip the CLI param then and hack it in the codegen.

Before posting any new patches, I would like to get consensus on this.

Options:

A) As-is and all 4 functions gets renamed.

    Stacktraces, gdb scripts, tracing etc. changes due to the 4 function renames.

B) Add a "operations"->"function-prefix" in YAML, only one funtion gets renamed.

    wg_get_device_start(), wg_get_device_dump() and wg_get_device_done() keep
    their names, while wg_set_device() gets renamed to wg_set_device_doit().

    This compliments the existing "name-prefix" (which is used for the UAPI enum names).

    Documentation/netlink/genetlink-legacy.yaml |  6 ++++++
    tools/net/ynl/pyynl/ynl_gen_c.py            | 13 +++++++++----
    2 files changed, 15 insertions(+), 4 deletions(-)

C) Add a "call" in YAML to override the default doit/dumpit names.

    All 4 functions can keep their current names.

    This compliments the existing "pre" and "post" (which are only rendered when set).

    How these map to struct genl_split_ops:

       kind \ YAML | pre        | call    | post
      -------------+------------+---------+------------
       do          | .pre_doit  | .doit   | .post_doit
       dump        | .start     | .dumpit | .done

D) Add it as a ynl_gen_c.py CLI param and make ynl-regen set it for wireguard?

While A is a no-op, then B is simpler to implement than option C, as the names are
generated in multiple places, where as it's simple to just use a prefix.
So option C might require some more refactoring, than is worth it for an one-off feature.
OTOH option C is more flexible than option B.

Jason, would option B work for you?

Jakub, would option B or C be acceptable?

WDYT?
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jakub Kicinski 4 weeks, 1 day ago
On Wed, 19 Nov 2025 19:19:28 +0000 Asbjørn Sloth Tønnesen wrote:
> >> Also fine with me. I'd just like consistent function naming, one way or
> >> another.  
> > 
> > IIUC we're talking about the prefix for the kernel C codegen?
> > Feels a bit like a one-off feature to me, but if we care deeply about
> > it let's add it as a CLI param to the codegen. I don't think it's
> > necessary to include this in the YAML spec.  
> 
> IIUC then adding it as a CLI param is more work, and just moves family details
> to ynl-regen, might as well skip the CLI param then and hack it in the codegen.
> 
> Before posting any new patches, I would like to get consensus on this.

The reason other C naming tunables exist (for legacy families!)
is because we need to give enough hints to C code gen to be able
to use existing kernel uAPI headers.

Random "naming preferences" do not belong in the spec. The spec 
is primarily for user space, and it's used by 4 languages already.
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jason A. Donenfeld 4 weeks, 1 day ago
On Wed, Nov 19, 2025 at 8:20 PM Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote:
> B) Add a "operations"->"function-prefix" in YAML, only one funtion gets renamed.
>
>     wg_get_device_start(), wg_get_device_dump() and wg_get_device_done() keep
>     their names, while wg_set_device() gets renamed to wg_set_device_doit().
>
>     This compliments the existing "name-prefix" (which is used for the UAPI enum names).
>
>     Documentation/netlink/genetlink-legacy.yaml |  6 ++++++
>     tools/net/ynl/pyynl/ynl_gen_c.py            | 13 +++++++++----
>     2 files changed, 15 insertions(+), 4 deletions(-)
>
> Jason, would option B work for you?

So just wg_set_device() -> wg_set_device_doit()? That seems quite fine
to me. And it's probably a better name, too, given that it corresponds
with device_dump. It makes both of those follow the form "wg_{genl
verb}_{nl verb}". I like it.

Jason
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Asbjørn Sloth Tønnesen 4 weeks, 1 day ago
On 11/19/25 7:22 PM, Jason A. Donenfeld wrote:
> On Wed, Nov 19, 2025 at 8:20 PM Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote:
>> B) Add a "operations"->"function-prefix" in YAML, only one funtion gets renamed.
>>
>>      wg_get_device_start(), wg_get_device_dump() and wg_get_device_done() keep
>>      their names, while wg_set_device() gets renamed to wg_set_device_doit().
>>
>>      This compliments the existing "name-prefix" (which is used for the UAPI enum names).
>>
>>      Documentation/netlink/genetlink-legacy.yaml |  6 ++++++
>>      tools/net/ynl/pyynl/ynl_gen_c.py            | 13 +++++++++----
>>      2 files changed, 15 insertions(+), 4 deletions(-)
>>
>> Jason, would option B work for you?
> 
> So just wg_set_device() -> wg_set_device_doit()? 

Sorry, no, wg_get_device_dump{,it} too.

Did my test on top of my branch were they are still renamed, so overlooked that rename.
Re: [PATCH net-next v3 04/11] netlink: specs: add specification for wireguard
Posted by Jason A. Donenfeld 4 weeks, 1 day ago
On Wed, Nov 19, 2025 at 8:47 PM Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote:
>
> On 11/19/25 7:22 PM, Jason A. Donenfeld wrote:
> > On Wed, Nov 19, 2025 at 8:20 PM Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote:
> >> B) Add a "operations"->"function-prefix" in YAML, only one funtion gets renamed.
> >>
> >>      wg_get_device_start(), wg_get_device_dump() and wg_get_device_done() keep
> >>      their names, while wg_set_device() gets renamed to wg_set_device_doit().
> >>
> >>      This compliments the existing "name-prefix" (which is used for the UAPI enum names).
> >>
> >>      Documentation/netlink/genetlink-legacy.yaml |  6 ++++++
> >>      tools/net/ynl/pyynl/ynl_gen_c.py            | 13 +++++++++----
> >>      2 files changed, 15 insertions(+), 4 deletions(-)
> >>
> >> Jason, would option B work for you?
> >
> > So just wg_set_device() -> wg_set_device_doit()?
>
> Sorry, no, wg_get_device_dump{,it} too.

Fine by me.