Re: [RFC PATCH 2/2] platform: add support for WireGuard links

Hi Thomas,

Thanks a lot for your feedback! Comments inline.

On Wed, Mar 14, 2018 at 3:15 PM, Thomas Haller <thaller redhat com> wrote:

You ask whether peer/allowed-ip should be their own NMPObject
implementations. Maybe, but it's not stringend.

I tend to think they should not be own objects. If they are their own
NMPObject instances, they maybe should not be linked via an embedded
CList, beause it means they cannot be shared, which is a bit unusual
for these objects.

When receiving these objects, you also commonly get them all in one go,
like for the VLAN egress-qos-map. That is another reason why it doesn't
make much sense to have them individual objects. You can just allocate
one large array and put all the parsed structs inside.

That looks simpler. I'll follow the example of egress-qos-map for v2.

+     allowedip->family = tba[IFLA_WG_ALLOWEDIP_FAMILY] ?
nla_get_u16 (tba[IFLA_WG_ALLOWEDIP_FAMILY]) : 0;
+     addr_len = (allowedip->family == AF_INET)

check that the family is at least AF_INET6, otherwise error out.

Will fix.

+static NMPObject *
+_parse_lnk_wireguard (const char *kind, const char *ifname)
+     nm_auto_nmpobj NMPObject *obj = NULL;
+     NMPObject *obj_result = NULL;
+     struct nl_sock *sock;
+     nm_auto_nlmsg struct nl_msg *msg = NULL;
+     struct nl_cb cb = {
+             .valid_cb = _wireguard_parse_getdevice,
+     };
+     static int family_id;
+     if (!nm_streq0 (kind, "wireguard"))
+             goto err;
+     sock = nl_socket_alloc ();
+     if (!sock)
+             goto err;
+     if (nl_connect (sock, NETLINK_GENERIC))

I think the genl socket should be cached and re-used by
NMLinuxPlatform. Likewise, family_id should be cached in

I'll look into it.

+             goto err_socket;

It's a bit odd that _parse_lnk_wireguard() opens a genl socket to
request additional data. Commonly the entire parsing of
_new_from_nl_link() is only supposed to get information that is present
in the netlink message. They only thing that they might do, is look
into the platform cache, and re-use information from there, for example

Basically, for every RTM_NEWLINK event, it re-fetches all wireguard
information from genetlink. That seems a bit heavy. Do we get a
notification on (ge)netlink when something about this interface

Otherwise, you could just look into the platform cache, see that the
information is already, and re-use it from there.

Yeah it does feel like an abuse of _new_from_nl_link().
I don't yet know if there are any options to subscribe to link changes.
I'll keep digging.

+     if (!family_id)
+             family_id = genl_ctrl_resolve (sock, "wireguard");
+     if (family_id <= 0)
+             goto err_socket;
+     msg = nlmsg_alloc ();
+     if (!genlmsg_put (msg, NL_AUTO_PORT, NL_AUTO_SEQ, family_id,
+                       0, NLM_F_DUMP, WG_CMD_GET_DEVICE, 1))
+             goto err_socket;
+     if (nla_put_string (msg, IFLA_WG_DEVICE_IFNAME, ifname) < 0)

can we avoid lookup by ifname? The ifname can be changed, only ifindex
is the unchangable, unique ID (mostly, not really either :( )

Checking WireGuard code now - yes we can lookup by ifindex.
I'll make that change.

+const char *
+nm_platform_lnk_wireguard_to_string (const NMPlatformLnkWireguard
*lnk, char *buf, gsize len)
+     gchar *public_b64;
+     if (!nm_utils_to_string_buffer_init_null (lnk, &buf, &len))
+             return buf;
+     public_b64 = g_base64_encode (lnk->public_key, sizeof (lnk-
+     g_snprintf (buf, len,
+                 "wireguard "
+                 "public_key %s "
+                 "private_key (hidden) "
+                 "listen_port %u "
+                 "fwmark 0x%x",
+                 public_b64,
+                 lnk->listen_port,
+                 lnk->fwmark);
+     g_free (public_b64);

don't explicitly free. Use one of the gs_* or nm_auto_* macros.
In this case, gs_free.


Commonly, the to-string methods should print *all* aspects of the
object. It's right to hide the private-key, I am talking about the

The sole purpose of the to-string methods if for debugging. Currently,
the peers/allowed-ips are never printed, maybe they should.

Note also that nmp_object_to_string() has an arguemnt to_string_mode,
for printing common fields (NMP_OBJECT_TO_STRING_PUBLIC) vs. all
I think at least for NMP_OBJECT_TO_STRING_ALL, NMPObjectLnkWireguard
should print all peers/allowed-ips. This means to implement

Sure. What about:

* Device: public key, port, fwmark, peers...
* Each peer: public key, endpoint, keepalive interval, allowed IPs..
* Each allowed IP: CIDR notation address

* All of the above plus every single field (including all stats).

+const char *
+nm_platform_wireguard_peer_to_string (const NMPlatformWireguardPeer
*peer, char *buf, gsize len)
+     gchar *public_b64;
+     char s_endpoint[NI_MAXHOST + NI_MAXSERV + sizeof("endpoint
[]:") + 1];
+     if (!nm_utils_to_string_buffer_init_null (peer, &buf, &len))
+             return buf;
+     if (peer->endpoint.addr.sa_family == AF_INET || peer-
endpoint.addr.sa_family == AF_INET6) {
+             char host[NI_MAXHOST];
+             char service[NI_MAXSERV];
+             socklen_t addr_len = 0;
+             if (peer->endpoint.addr.sa_family == AF_INET)
+                     addr_len = sizeof (struct sockaddr_in);
+             else if (peer->endpoint.addr.sa_family == AF_INET6)
+                     addr_len = sizeof (struct sockaddr_in6);
+             if (!getnameinfo (&peer->endpoint.addr, addr_len,
host, sizeof(host), service, sizeof(service), NI_DGRAM |

I don't think we should resolve the addresses just for logging.
Just convert them to string with nm_utils_inet_ntop().

I understood getnameinfo() with NI_NUMERIC* didn't do resolution.
Anyhow, I can use that utils function if you prefer.

+typedef struct {
+     guint8 private_key[NM_WG_PUBLIC_KEY_LEN];
+     guint8 public_key[NM_WG_PUBLIC_KEY_LEN];
+     guint16 listen_port;
+     guint32 fwmark;
+     CList peers_lst_head;

The public part of the NMPObjects (NMPlatformLnkWireguard vs.
NMPObjectLnkWireguard) is supposed to be copyable directly. That means,
it cannot have pointers (except for example NMPlatformLink.kind, which
is however a static string, so it's safe to copy).

This means, the CList part must go into NMPObjectLnkWireguard.

Together with I said above about nm_platform_lnk_wireguard_to_string()
to print all fields, it means, the list of peers/allowed-ips is not
accessible to NMPlatformLnkWireguard, so there is nothing to print.
But it certainly should implement cmd_obj_to_string().

Got it - I'll go the NMPObjectLnkVlan route instead as you suggest.

+             .parent                             =
+             .obj_type                           =
+             .sizeof_data                        = sizeof
+             .sizeof_public                      = sizeof
+             .obj_type_name                      = "wireguard",
+             .lnk_link_type                      =
+             .cmd_plobj_to_string                = (const char
*(*) (const NMPlatformObject *obj, char *buf, gsize len))

the types that are put inside the platform cache (like
NMPObjectLnkWireguard), they must implement equality/hash operators.

NMPObjectLnkWireguard is not unlike NMPObjectLnkVlan. See what
functions are implemented there.

Will do.

(Actually had these functions in before, following the macsec example,
but then I thought I'd just make the patch simpler until I was sure what
the wireguard data structures would look like.)

On Wed, Mar 14, 2018 at 5:20 PM, Thomas Haller <thaller redhat com> wrote:
+static NMPObject *
+_parse_lnk_wireguard (const char *kind, const char *ifname)
+     nm_auto_nmpobj NMPObject *obj = NULL;
+     NMPObject *obj_result = NULL;
+     struct nl_sock *sock;
+     nm_auto_nlmsg struct nl_msg *msg = NULL;
+     struct nl_cb cb = {
+             .valid_cb = _wireguard_parse_getdevice,
+     };
+     static int family_id;


all the "parsing" done here is basically requesting/querying everyhing
via genl.

It would be really nice, to only request new data when necessary, and
otherwise keep the current from the cache.

But how to notice changes?

Maybe the wireguard netlink API should expose a version id both on rtnl
and genl. Then, if we receive a new (different) version id on rtnl, we
know we need to refetch everything.

Or does the wireguard API already provide some functionality about
this? Should we instead register to signals on genl? That would be
doable, but cumbersome.

Not sure, I'd wait for Jason's answer on this one. I can see a
device_update_gen counter within struct wireguard_device, but I don't
think that is meant to be reported via rtnl.

Again, thanks for your time!

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]