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: <aRyLoy2iqbkUipZW@zx2c4.com>
Date: Tue, 18 Nov 2025 16:07:15 +0100
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Asbjørn Sloth Tønnesen <ast@...erby.net>
Cc: Jakub Kicinski <kuba@...nel.org>,
	"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 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