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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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