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

[Asbjørn Sloth Tønnesen]

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@xxxxxxxxxxx/

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.