lkml.org 
[lkml]   [2018]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
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);

[...]

\
 
 \ /
  Last update: 2018-10-06 09:03    [W:0.221 / U:17.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site