Messages in this thread Patch in this message |  | | Date | Sat, 6 Oct 2018 08:58:19 +0200 | From | Jiri Pirko <> | Subject | Re: [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);
[...]
|  |