Re: [PATCH net-next v3 04/24] ovpn: add basic interface creation/destruction/management routines
From: Sabrina Dubroca <sd@queasysnail.net>
Date: 2024-05-08 14:52:35
2024-05-06, 03:16:17 +0200, Antonio Quartulli wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index ad3813419c33..338e99dfe886 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c@@ -11,6 +11,26 @@ #include <linux/skbuff.h> #include "io.h" +#include "ovpnstruct.h" +#include "netlink.h" + +int ovpn_struct_init(struct net_device *dev)
nit: Should this be in main.c? It's only used there, and I think it would make more sense to drop it next to ovpn_struct_free.
+{
+ struct ovpn_struct *ovpn = netdev_priv(dev);
+ int err;
+[...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index 33c0b004ce16..584cd7286aff 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c
[...]
+static void ovpn_struct_free(struct net_device *net)
+{
+ struct ovpn_struct *ovpn = netdev_priv(net);
+
+ rtnl_lock();->priv_destructor can run from register_netdevice (already under RTNL), this doesn't look right.
+ list_del(&ovpn->dev_list);
And if this gets called from register_netdevice, the list_add from ovpn_iface_create hasn't run yet, so this will probably do strange things?
+ rtnl_unlock();
+
+ free_percpu(net->tstats);
+}
+
+static int ovpn_net_open(struct net_device *dev)
+{
+ struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
+
+ if (dev_v4) {
+ /* disable redirects as Linux gets confused by ovpn handling
+ * same-LAN routing
+ */
+ IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
+ IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;Jakub, are you ok with that? This feels a bit weird to have in the middle of a driver.
+ } + + netif_tx_start_all_queues(dev); + return 0; +}
[...]
+void ovpn_iface_destruct(struct ovpn_struct *ovpn)
+{
+ ASSERT_RTNL();
+
+ netif_carrier_off(ovpn->dev);
+
+ ovpn->registered = false;
+
+ unregister_netdevice(ovpn->dev);
+ synchronize_net();If this gets called from the loop in ovpn_netns_pre_exit, one synchronize_net per ovpn device would seem quite expensive.
+}
+
static int ovpn_netdev_notifier_call(struct notifier_block *nb,
unsigned long state, void *ptr)
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct ovpn_struct *ovpn;
if (!ovpn_dev_is_valid(dev))
return NOTIFY_DONE;
+ ovpn = netdev_priv(dev);
+
switch (state) {
case NETDEV_REGISTER:
- /* add device to internal list for later destruction upon
- * unregistration
- */
+ ovpn->registered = true;
break;
case NETDEV_UNREGISTER:
+ /* twiddle thumbs on netns device moves */
+ if (dev->reg_state != NETREG_UNREGISTERING)
+ break;
+
/* can be delivered multiple times, so check registered flag,
* then destroy the interface
*/
+ if (!ovpn->registered)
+ return NOTIFY_DONE;
+
+ ovpn_iface_destruct(ovpn);Maybe I'm misunderstanding this code. Why do you want to manually destroy a device that is already going away?
quoted hunk ↗ jump to hunk
break; case NETDEV_POST_INIT: case NETDEV_GOING_DOWN: case NETDEV_DOWN: case NETDEV_UP: case NETDEV_PRE_UP: + break; default: return NOTIFY_DONE; }@@ -62,6 +210,24 @@ static struct notifier_block ovpn_netdev_notifier = { .notifier_call = ovpn_netdev_notifier_call, }; +static void ovpn_netns_pre_exit(struct net *net) +{ + struct ovpn_struct *ovpn; + + rtnl_lock(); + list_for_each_entry(ovpn, &dev_list, dev_list) { + if (dev_net(ovpn->dev) != net) + continue; + + ovpn_iface_destruct(ovpn);
Is this needed? On netns destruction all devices within the ns will be destroyed by the networking core.
+ } + rtnl_unlock(); +}
-- Sabrina