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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f21458b6-f169-4cd3-bd1b-16255c78d6cd@fiberby.net>
Date: Tue, 18 Nov 2025 12:08:20 +0000
From: Asbjørn Sloth Tønnesen <ast@...erby.net>
To: "Jason A. Donenfeld" <Jason@...c4.com>, Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, 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

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.