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:5.224 / U:0.588 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site