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

Asbjørn Sloth Tønnesen
2025-11-18T12:19:15Z

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.