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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aRvWzC8qz3iXDAb3@zx2c4.com>
Date: Tue, 18 Nov 2025 03:15:40 +0100
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Asbjørn Sloth Tønnesen <ast@...erby.net>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Donald Hunter <donald.hunter@...il.com>,
	Simon Horman <horms@...nel.org>,
	Jacob Keller <jacob.e.keller@...el.com>,
	Andrew Lunn <andrew+netdev@...n.ch>, wireguard@...ts.zx2c4.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jordan Rife <jordan@...fe.io>
Subject: Re: [PATCH net-next v3 04/11] netlink: specs: add specification for
 wireguard

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