| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a10dcebf-b8f1-4d9b-b417-cca7d0330e52@openvpn.net>
Date: Thu, 19 Sep 2024 13:57:51 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: andrew@...n.ch, antony.antony@...unet.com, edumazet@...gle.com,
kuba@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com,
ryazanov.s.a@...il.com, sd@...asysnail.net, steffen.klassert@...unet.com
Subject: Re: [PATCH net-next v7 03/25] net: introduce OpenVPN Data Channel
Offload (ovpn)
Hi Kuniyuki and thank you for chiming in.
On 19/09/2024 07:52, Kuniyuki Iwashima wrote:
> From: Antonio Quartulli <antonio@...nvpn.net>
> Date: Tue, 17 Sep 2024 03:07:12 +0200
>> +/* we register with rtnl to let core know that ovpn is a virtual driver and
>> + * therefore ifaces should be destroyed when exiting a netns
>> + */
>> +static struct rtnl_link_ops ovpn_link_ops = {
>> +};
>
> This looks like abusing rtnl_link_ops.
In some way, the inspiration came from
5b9e7e160795 ("openvswitch: introduce rtnl ops stub")
[which just reminded me that I wanted to fill the .kind field, but I
forgot to do so]
The reason for taking this approach was to avoid handling the iface
destruction upon netns exit inside the driver, when the core already has
all the code for taking care of this for us.
Originally I implemented pernet_operations.pre_exit, but Sabrina
suggested that letting the core handle the destruction was cleaner (and
I agreed).
However, after I removed the pre_exit implementation, we realized that
default_device_exit_batch/default_device_exit_net thought that an ovpn
device is a real NIC and was moving it to the global netns rather than
killing it.
One way to fix the above was to register rtnl_link_ops with netns_fund =
false (so the ops object you see in this patch is not truly "empty").
However, I then hit the bug which required patch 2 to get fixed.
Does it make sense to you?
Or you still think this is an rtnl_link_ops abuse?
The alternative was to change
default_device_exit_batch/default_device_exit_net to read some new
netdevice flag which would tell if the interface should be killed or
moved to global upon netns exit.
Regards,
>
> Instead of a hack to rely on default_device_exit_batch()
> and rtnl_link_unregister(), this should be implemented as
> struct pernet_operations.exit_batch_rtnl().
>
> Then, the patch 2 is not needed, which is confusing for
> all other rtnl_link_ops users.
>
> If we want to avoid extra RTNL in default_device_exit_batch(),
> I can post this patch after merge window.
>
> ---8<---
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1e740faf9e78..eacf6f5a6ace 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -11916,7 +11916,8 @@ static void __net_exit default_device_exit_net(struct net *net)
> }
> }
>
> -static void __net_exit default_device_exit_batch(struct list_head *net_list)
> +void __net_exit default_device_exit_batch(struct list_head *net_list,
> + struct list_head *dev_kill_list)
> {
> /* At exit all network devices most be removed from a network
> * namespace. Do this in the reverse order of registration.
> @@ -11925,9 +11926,7 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
> */
> struct net_device *dev;
> struct net *net;
> - LIST_HEAD(dev_kill_list);
>
> - rtnl_lock();
> list_for_each_entry(net, net_list, exit_list) {
> default_device_exit_net(net);
> cond_resched();
> @@ -11936,19 +11935,13 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
> list_for_each_entry(net, net_list, exit_list) {
> for_each_netdev_reverse(net, dev) {
> if (dev->rtnl_link_ops && dev->rtnl_link_ops->dellink)
> - dev->rtnl_link_ops->dellink(dev, &dev_kill_list);
> + dev->rtnl_link_ops->dellink(dev, dev_kill_list);
> else
> - unregister_netdevice_queue(dev, &dev_kill_list);
> + unregister_netdevice_queue(dev, dev_kill_list);
> }
> }
> - unregister_netdevice_many(&dev_kill_list);
> - rtnl_unlock();
> }
>
> -static struct pernet_operations __net_initdata default_device_ops = {
> - .exit_batch = default_device_exit_batch,
> -};
> -
> static void __init net_dev_struct_check(void)
> {
> /* TX read-mostly hotpath */
> @@ -12140,9 +12133,6 @@ static int __init net_dev_init(void)
> if (register_pernet_device(&loopback_net_ops))
> goto out;
>
> - if (register_pernet_device(&default_device_ops))
> - goto out;
> -
> open_softirq(NET_TX_SOFTIRQ, net_tx_action);
> open_softirq(NET_RX_SOFTIRQ, net_rx_action);
>
> diff --git a/net/core/dev.h b/net/core/dev.h
> index 5654325c5b71..d1feecab9c4a 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -99,6 +99,9 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags,
> void unregister_netdevice_many_notify(struct list_head *head,
> u32 portid, const struct nlmsghdr *nlh);
>
> +void default_device_exit_batch(struct list_head *net_list,
> + struct list_head *dev_kill_list);
> +
> static inline void netif_set_gso_max_size(struct net_device *dev,
> unsigned int size)
> {
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 11e4dd4f09ed..0a9bce599d54 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -27,6 +27,8 @@
> #include <net/net_namespace.h>
> #include <net/netns/generic.h>
>
> +#include "dev.h"
> +
> /*
> * Our network namespace constructor/destructor lists
> */
> @@ -380,6 +382,7 @@ static __net_init int setup_net(struct net *net)
> if (ops->exit_batch_rtnl)
> ops->exit_batch_rtnl(&net_exit_list, &dev_kill_list);
> }
> + default_device_exit_batch(&net_exit_list, &dev_kill_list);
> unregister_netdevice_many(&dev_kill_list);
> rtnl_unlock();
>
> @@ -618,6 +621,7 @@ static void cleanup_net(struct work_struct *work)
> if (ops->exit_batch_rtnl)
> ops->exit_batch_rtnl(&net_exit_list, &dev_kill_list);
> }
> + default_device_exit_batch(&net_exit_list, &dev_kill_list);
> unregister_netdevice_many(&dev_kill_list);
> rtnl_unlock();
>
> @@ -1214,6 +1218,7 @@ static void free_exit_list(struct pernet_operations *ops, struct list_head *net_
>
> rtnl_lock();
> ops->exit_batch_rtnl(net_exit_list, &dev_kill_list);
> + default_device_exit_batch(net_exit_list, &dev_kill_list);
> unregister_netdevice_many(&dev_kill_list);
> rtnl_unlock();
> }
> ---8<---
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists