From: Jiri Pirko <jiri@resnulli.us>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
davem@davemloft.net, gregkh@linuxfoundation.org
Subject: Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
Date: Sat, 6 Oct 2018 08:58:19 +0200 [thread overview]
Message-ID: <20181006065819.GD3061@nanopsycho.orion> (raw)
In-Reply-To: <20181006025709.4019-29-Jason@zx2c4.com>
Sat, Oct 06, 2018 at 04:57:09AM CEST, Jason@zx2c4.com wrote:
[...]
>+}
>+
>+static const struct net_device_ops netdev_ops = {
>+ .ndo_open = open,
>+ .ndo_stop = stop,
>+ .ndo_start_xmit = xmit,
It would be nice to put the callbacks and other functions in this file
into a namespace by some common prefix.
If one sees "open/stop/xmit/destruct/setup/..." in a trace, that does
not tell much :/
>+ .ndo_get_stats64 = ip_tunnel_get_stats64
>+};
>+
>+static void destruct(struct net_device *dev)
>+{
>+ struct wireguard_device *wg = netdev_priv(dev);
>+
>+ rtnl_lock();
>+ list_del(&wg->device_list);
>+ rtnl_unlock();
>+ mutex_lock(&wg->device_update_lock);
>+ wg->incoming_port = 0;
>+ wg_socket_reinit(wg, NULL, NULL);
>+ wg_allowedips_free(&wg->peer_allowedips, &wg->device_update_lock);
>+ /* The final references are cleared in the below calls to destroy_workqueue. */
>+ wg_peer_remove_all(wg);
>+ destroy_workqueue(wg->handshake_receive_wq);
>+ destroy_workqueue(wg->handshake_send_wq);
>+ destroy_workqueue(wg->packet_crypt_wq);
>+ wg_packet_queue_free(&wg->decrypt_queue, true);
>+ wg_packet_queue_free(&wg->encrypt_queue, true);
>+ rcu_barrier_bh(); /* Wait for all the peers to be actually freed. */
>+ wg_ratelimiter_uninit();
>+ memzero_explicit(&wg->static_identity, sizeof(wg->static_identity));
>+ skb_queue_purge(&wg->incoming_handshakes);
>+ free_percpu(dev->tstats);
>+ free_percpu(wg->incoming_handshakes_worker);
>+ if (wg->have_creating_net_ref)
>+ put_net(wg->creating_net);
>+ mutex_unlock(&wg->device_update_lock);
>+
>+ pr_debug("%s: Interface deleted\n", dev->name);
>+ free_netdev(dev);
>+}
>+
>+static const struct device_type device_type = { .name = KBUILD_MODNAME };
>+
>+static void setup(struct net_device *dev)
>+{
>+ struct wireguard_device *wg = netdev_priv(dev);
>+ enum { WG_NETDEV_FEATURES = NETIF_F_HW_CSUM | NETIF_F_RXCSUM |
>+ NETIF_F_SG | NETIF_F_GSO |
>+ NETIF_F_GSO_SOFTWARE | NETIF_F_HIGHDMA };
>+
>+ dev->netdev_ops = &netdev_ops;
>+ dev->hard_header_len = 0;
>+ dev->addr_len = 0;
>+ dev->needed_headroom = DATA_PACKET_HEAD_ROOM;
>+ dev->needed_tailroom = noise_encrypted_len(MESSAGE_PADDING_MULTIPLE);
>+ dev->type = ARPHRD_NONE;
>+ dev->flags = IFF_POINTOPOINT | IFF_NOARP;
>+ dev->priv_flags |= IFF_NO_QUEUE;
>+ dev->features |= NETIF_F_LLTX;
>+ dev->features |= WG_NETDEV_FEATURES;
>+ dev->hw_features |= WG_NETDEV_FEATURES;
>+ dev->hw_enc_features |= WG_NETDEV_FEATURES;
>+ dev->mtu = ETH_DATA_LEN - MESSAGE_MINIMUM_LENGTH -
>+ sizeof(struct udphdr) -
>+ max(sizeof(struct ipv6hdr), sizeof(struct iphdr));
>+
>+ SET_NETDEV_DEVTYPE(dev, &device_type);
>+
>+ /* We need to keep the dst around in case of icmp replies. */
>+ netif_keep_dst(dev);
>+
>+ memset(wg, 0, sizeof(*wg));
>+ wg->dev = dev;
>+}
>+
>+static int newlink(struct net *src_net, struct net_device *dev,
>+ struct nlattr *tb[], struct nlattr *data[],
>+ struct netlink_ext_ack *extack)
>+{
>+ int ret = -ENOMEM;
>+ struct wireguard_device *wg = netdev_priv(dev);
>+
>+ wg->creating_net = src_net;
>+ init_rwsem(&wg->static_identity.lock);
>+ mutex_init(&wg->socket_update_lock);
>+ mutex_init(&wg->device_update_lock);
>+ skb_queue_head_init(&wg->incoming_handshakes);
>+ wg_pubkey_hashtable_init(&wg->peer_hashtable);
>+ wg_index_hashtable_init(&wg->index_hashtable);
>+ wg_allowedips_init(&wg->peer_allowedips);
>+ wg_cookie_checker_init(&wg->cookie_checker, wg);
>+ INIT_LIST_HEAD(&wg->peer_list);
>+ wg->device_update_gen = 1;
>+
>+ dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
>+ if (!dev->tstats)
>+ goto error_1;
Just "return -ENOMEM" here.
>+
>+ wg->incoming_handshakes_worker =
>+ wg_packet_alloc_percpu_multicore_worker(
>+ wg_packet_handshake_receive_worker, wg);
>+ if (!wg->incoming_handshakes_worker)
>+ goto error_2;
Please consider renaming the label to "what went wrong". In this case,
it would be "err_alloc_worker".
>+
>+ wg->handshake_receive_wq = alloc_workqueue("wg-kex-%s",
>+ WQ_CPU_INTENSIVE | WQ_FREEZABLE, 0, dev->name);
>+ if (!wg->handshake_receive_wq)
>+ goto error_3;
>+
>+ wg->handshake_send_wq = alloc_workqueue("wg-kex-%s",
>+ WQ_UNBOUND | WQ_FREEZABLE, 0, dev->name);
>+ if (!wg->handshake_send_wq)
>+ goto error_4;
>+
>+ wg->packet_crypt_wq = alloc_workqueue("wg-crypt-%s",
>+ WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM, 0, dev->name);
>+ if (!wg->packet_crypt_wq)
>+ goto error_5;
>+
>+ if (wg_packet_queue_init(&wg->encrypt_queue, wg_packet_encrypt_worker,
>+ true, MAX_QUEUED_PACKETS) < 0)
You need to have "int err" and always in cases like this to do:
err = wg_packet_queue_init()
if (err)
goto err_*
>+ goto error_6;
>+
>+ if (wg_packet_queue_init(&wg->decrypt_queue, wg_packet_decrypt_worker,
>+ true, MAX_QUEUED_PACKETS) < 0)
>+ goto error_7;
>+
>+ ret = wg_ratelimiter_init();
>+ if (ret < 0)
>+ goto error_8;
>+
>+ ret = register_netdevice(dev);
>+ if (ret < 0)
>+ goto error_9;
>+
>+ list_add(&wg->device_list, &device_list);
>+
>+ /* We wait until the end to assign priv_destructor, so that
>+ * register_netdevice doesn't call it for us if it fails.
>+ */
>+ dev->priv_destructor = destruct;
>+
>+ pr_debug("%s: Interface created\n", dev->name);
>+ return ret;
>+
>+error_9:
>+ wg_ratelimiter_uninit();
>+error_8:
>+ wg_packet_queue_free(&wg->decrypt_queue, true);
>+error_7:
>+ wg_packet_queue_free(&wg->encrypt_queue, true);
>+error_6:
>+ destroy_workqueue(wg->packet_crypt_wq);
>+error_5:
>+ destroy_workqueue(wg->handshake_send_wq);
>+error_4:
>+ destroy_workqueue(wg->handshake_receive_wq);
>+error_3:
>+ free_percpu(wg->incoming_handshakes_worker);
>+error_2:
>+ free_percpu(dev->tstats);
>+error_1:
>+ return ret;
>+}
>+
>+static struct rtnl_link_ops link_ops __read_mostly = {
>+ .kind = KBUILD_MODNAME,
>+ .priv_size = sizeof(struct wireguard_device),
>+ .setup = setup,
>+ .newlink = newlink,
>+};
>+
>+static int netdevice_notification(struct notifier_block *nb,
>+ unsigned long action, void *data)
>+{
>+ struct net_device *dev = ((struct netdev_notifier_info *)data)->dev;
>+ struct wireguard_device *wg = netdev_priv(dev);
>+
>+ ASSERT_RTNL();
>+
>+ if (action != NETDEV_REGISTER || dev->netdev_ops != &netdev_ops)
>+ return 0;
>+
>+ if (dev_net(dev) == wg->creating_net && wg->have_creating_net_ref) {
>+ put_net(wg->creating_net);
>+ wg->have_creating_net_ref = false;
>+ } else if (dev_net(dev) != wg->creating_net &&
>+ !wg->have_creating_net_ref) {
>+ wg->have_creating_net_ref = true;
>+ get_net(wg->creating_net);
>+ }
>+ return 0;
>+}
>+
>+static struct notifier_block netdevice_notifier = {
>+ .notifier_call = netdevice_notification
>+};
>+
>+int __init wg_device_init(void)
>+{
>+ int ret;
>+
>+#if defined(CONFIG_PM_SLEEP) && !defined(CONFIG_ANDROID)
>+ ret = register_pm_notifier(&pm_notifier);
>+ if (ret)
>+ return ret;
>+#endif
>+
>+ ret = register_netdevice_notifier(&netdevice_notifier);
>+ if (ret)
>+ goto error_pm;
>+
>+ ret = rtnl_link_register(&link_ops);
>+ if (ret)
>+ goto error_netdevice;
>+
>+ return 0;
>+
>+error_netdevice:
>+ unregister_netdevice_notifier(&netdevice_notifier);
>+error_pm:
>+#if defined(CONFIG_PM_SLEEP) && !defined(CONFIG_ANDROID)
>+ unregister_pm_notifier(&pm_notifier);
>+#endif
>+ return ret;
>+}
>+
>+void wg_device_uninit(void)
>+{
>+ rtnl_link_unregister(&link_ops);
>+ unregister_netdevice_notifier(&netdevice_notifier);
>+#if defined(CONFIG_PM_SLEEP) && !defined(CONFIG_ANDROID)
>+ unregister_pm_notifier(&pm_notifier);
>+#endif
>+ rcu_barrier_bh();
>+}
>diff --git a/drivers/net/wireguard/device.h b/drivers/net/wireguard/device.h
>new file mode 100644
>index 000000000000..2bd1429b5831
>--- /dev/null
>+++ b/drivers/net/wireguard/device.h
>@@ -0,0 +1,65 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
>+ */
>+
>+#ifndef _WG_DEVICE_H
>+#define _WG_DEVICE_H
>+
>+#include "noise.h"
>+#include "allowedips.h"
>+#include "hashtables.h"
>+#include "cookie.h"
>+
>+#include <linux/types.h>
>+#include <linux/netdevice.h>
>+#include <linux/workqueue.h>
>+#include <linux/mutex.h>
>+#include <linux/net.h>
>+#include <linux/ptr_ring.h>
>+
>+struct wireguard_device;
>+
>+struct multicore_worker {
>+ void *ptr;
>+ struct work_struct work;
>+};
>+
>+struct crypt_queue {
Similar to structure names. Please consider having a single prefix for
the struct and func names.
>+ struct ptr_ring ring;
>+ union {
>+ struct {
>+ struct multicore_worker __percpu *worker;
>+ int last_cpu;
>+ };
>+ struct work_struct work;
>+ };
>+};
>+
>+struct wireguard_device {
This is inconsistent. "wireguard_device" vs "wg_*". The name should be
rather something like "wg_device".
>+ struct net_device *dev;
>+ struct crypt_queue encrypt_queue, decrypt_queue;
>+ struct sock __rcu *sock4, *sock6;
>+ struct net *creating_net;
>+ struct noise_static_identity static_identity;
>+ struct workqueue_struct *handshake_receive_wq, *handshake_send_wq;
>+ struct workqueue_struct *packet_crypt_wq;
>+ struct sk_buff_head incoming_handshakes;
>+ int incoming_handshake_cpu;
>+ struct multicore_worker __percpu *incoming_handshakes_worker;
>+ struct cookie_checker cookie_checker;
>+ struct pubkey_hashtable peer_hashtable;
>+ struct index_hashtable index_hashtable;
>+ struct allowedips peer_allowedips;
>+ struct mutex device_update_lock, socket_update_lock;
>+ struct list_head device_list, peer_list;
>+ unsigned int num_peers, device_update_gen;
>+ u32 fwmark;
>+ u16 incoming_port;
>+ bool have_creating_net_ref;
>+};
[...]
>+static int __init mod_init(void)
>+{
>+ int ret;
>+
>+#ifdef DEBUG
>+ if (!wg_allowedips_selftest() || !wg_packet_counter_selftest() ||
>+ !wg_ratelimiter_selftest())
>+ return -ENOTRECOVERABLE;
>+#endif
>+ wg_noise_init();
>+
>+ ret = wg_device_init();
>+ if (ret < 0)
>+ goto err_device;
>+
>+ ret = wg_genetlink_init();
>+ if (ret < 0)
>+ goto err_netlink;
>+
>+ pr_info("WireGuard " WIREGUARD_VERSION " loaded. See www.wireguard.com for information.\n");
>+ pr_info("Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.\n");
It is not common to have this output for new modules. Please remove,
does not tell anything to the user.
>+
>+ return 0;
>+
>+err_netlink:
>+ wg_device_uninit();
>+err_device:
>+ return ret;
>+}
>+
>+static void __exit mod_exit(void)
>+{
>+ wg_genetlink_uninit();
>+ wg_device_uninit();
>+ pr_debug("WireGuard unloaded\n");
Same here.
>+}
>+
>+module_init(mod_init);
>+module_exit(mod_exit);
>+MODULE_LICENSE("GPL v2");
>+MODULE_DESCRIPTION("Fast, modern, and secure VPN tunnel");
Descrioption should be rather somethin like "WireGuard tunnel".
>+MODULE_AUTHOR("Jason A. Donenfeld <Jason@zx2c4.com>");
>+MODULE_VERSION(WIREGUARD_VERSION);
>+MODULE_ALIAS_RTNL_LINK(KBUILD_MODNAME);
>+MODULE_ALIAS_GENL_FAMILY(WG_GENL_NAME);
[...]
next prev parent reply other threads:[~2018-10-06 7:03 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-06 2:56 [PATCH net-next v7 00/28] WireGuard: Secure Network Tunnel Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 01/28] ARM: makefile: use ARMv3M mode for RiscPC Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 02/28] asm: simd context helper API Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 03/28] zinc: introduce minimal cryptography library Jason A. Donenfeld
2018-10-08 23:22 ` Eric Biggers
2018-10-06 2:56 ` [PATCH net-next v7 04/28] zinc: ChaCha20 generic C implementation and selftest Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 05/28] zinc: import Andy Polyakov's ChaCha20 x86_64 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 06/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 07/28] zinc: import Andy Polyakov's ChaCha20 ARM and ARM64 implementations Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 08/28] zinc: port " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 09/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 10/28] zinc: ChaCha20 MIPS32r2 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 11/28] zinc: Poly1305 generic C implementations and selftest Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 12/28] zinc: import Andy Polyakov's Poly1305 x86_64 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 13/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 14/28] zinc: import Andy Polyakov's Poly1305 ARM and ARM64 implementations Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 15/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 16/28] zinc: import Andy Polyakov's Poly1305 MIPS64 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 17/28] zinc: Poly1305 MIPS32r2 and MIPS64 implementations Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 18/28] zinc: ChaCha20Poly1305 construction and selftest Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 19/28] zinc: BLAKE2s generic C implementation " Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 20/28] zinc: BLAKE2s x86_64 implementation Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 21/28] zinc: Curve25519 generic C implementations and selftest Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 22/28] zinc: Curve25519 x86_64 implementation Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 23/28] zinc: import Bernstein and Schwabe's Curve25519 ARM implementation Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 24/28] zinc: " Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 25/28] crypto: port Poly1305 to Zinc Jason A. Donenfeld
2018-10-08 23:21 ` Eric Biggers
2018-10-09 0:02 ` Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 26/28] crypto: port ChaCha20 " Jason A. Donenfeld
2018-10-06 13:07 ` Martin Willi
2018-10-06 2:57 ` [PATCH net-next v7 27/28] security/keys: rewrite big_key crypto to use Zinc Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 28/28] net: WireGuard secure network tunnel Jason A. Donenfeld
2018-10-06 6:58 ` Jiri Pirko [this message]
2018-10-06 12:25 ` Jason A. Donenfeld
2018-10-07 17:26 ` Jason A. Donenfeld
2018-10-07 18:14 ` Lukas Wunner
2018-10-07 18:42 ` Andrew Lunn
2018-10-10 9:13 ` Jiri Pirko
2018-10-10 20:27 ` Jason A. Donenfeld
2018-10-11 5:36 ` Jiri Pirko
2018-10-06 19:43 ` Eugene Syromiatnikov
2018-10-08 1:06 ` Jason A. Donenfeld
2018-10-06 23:54 ` Andrew Lunn
2018-10-07 1:57 ` Jason A. Donenfeld
2018-10-07 16:48 ` Andrew Lunn
2018-10-07 16:55 ` Jason A. Donenfeld
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181006065819.GD3061@nanopsycho.orion \
--to=jiri@resnulli.us \
--cc=Jason@zx2c4.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).