| Message ID | 20240304150914.11444-5-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
> +int ovpn_struct_init(struct net_device *dev) > +{ > + struct ovpn_struct *ovpn = netdev_priv(dev); > + int err; > + > + memset(ovpn, 0, sizeof(*ovpn)); Probably not required. When a netdev is created, it should of zeroed the priv. > +int ovpn_iface_create(const char *name, enum ovpn_mode mode, struct net *net) > +{ > + struct net_device *dev; > + struct ovpn_struct *ovpn; > + int ret; > + > + dev = alloc_netdev(sizeof(struct ovpn_struct), name, NET_NAME_USER, ovpn_setup); > + > + dev_net_set(dev, net); > + > + ret = ovpn_struct_init(dev); > + if (ret < 0) > + goto err; > + > + ovpn = netdev_priv(dev); > + ovpn->mode = mode; > + > + rtnl_lock(); > + > + ret = register_netdevice(dev); > + if (ret < 0) { > + netdev_dbg(dev, "cannot register interface %s: %d\n", dev->name, ret); > + rtnl_unlock(); > + goto err; > + } > + rtnl_unlock(); > + > + return ret; > + > +err: > + free_netdev(dev); > + return ret; > +} > + > +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) > +{ > + ASSERT_RTNL(); > + > + netif_carrier_off(ovpn->dev); You often see virtual devices turn their carrier off in there probe/create function, because it is unclear what state it is in after register_netdevice(). Andrew
On 04/03/2024 22:33, Andrew Lunn wrote: >> +int ovpn_struct_init(struct net_device *dev) >> +{ >> + struct ovpn_struct *ovpn = netdev_priv(dev); >> + int err; >> + >> + memset(ovpn, 0, sizeof(*ovpn)); > > Probably not required. When a netdev is created, it should of zeroed > the priv. ACK. There is a kvzalloc() involved. > >> +int ovpn_iface_create(const char *name, enum ovpn_mode mode, struct net *net) >> +{ >> + struct net_device *dev; >> + struct ovpn_struct *ovpn; >> + int ret; >> + >> + dev = alloc_netdev(sizeof(struct ovpn_struct), name, NET_NAME_USER, ovpn_setup); >> + >> + dev_net_set(dev, net); >> + >> + ret = ovpn_struct_init(dev); >> + if (ret < 0) >> + goto err; >> + >> + ovpn = netdev_priv(dev); >> + ovpn->mode = mode; >> + >> + rtnl_lock(); >> + >> + ret = register_netdevice(dev); >> + if (ret < 0) { >> + netdev_dbg(dev, "cannot register interface %s: %d\n", dev->name, ret); >> + rtnl_unlock(); >> + goto err; >> + } >> + rtnl_unlock(); >> + >> + return ret; >> + >> +err: >> + free_netdev(dev); >> + return ret; >> +} >> + >> +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) >> +{ >> + ASSERT_RTNL(); >> + >> + netif_carrier_off(ovpn->dev); > > You often see virtual devices turn their carrier off in there > probe/create function, because it is unclear what state it is in after > register_netdevice(). Are you suggesting to turn it off both here and in the create function? Or should I remove the invocation above? Regards,
> > > +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) > > > +{ > > > + ASSERT_RTNL(); > > > + > > > + netif_carrier_off(ovpn->dev); > > > > You often see virtual devices turn their carrier off in there > > probe/create function, because it is unclear what state it is in after > > register_netdevice(). > > Are you suggesting to turn it off both here and in the create function? > Or should I remove the invocation above? I noticed it in the _destruct function and went back to look at create. You probably want it in both, unless as part of destruct, you first disconnect all peers, which should set the carrier to off when the last peer disconnects? Andrew
On Mon, 4 Mar 2024 16:08:55 +0100 Antonio Quartulli wrote: > + dev = alloc_netdev(sizeof(struct ovpn_struct), name, NET_NAME_USER, ovpn_setup); > + > + dev_net_set(dev, net); the dev allocation never fails? Or the error is handles somewhere else?
On 05/03/2024 17:27, Andrew Lunn wrote: >>>> +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) >>>> +{ >>>> + ASSERT_RTNL(); >>>> + >>>> + netif_carrier_off(ovpn->dev); >>> >>> You often see virtual devices turn their carrier off in there >>> probe/create function, because it is unclear what state it is in after >>> register_netdevice(). >> >> Are you suggesting to turn it off both here and in the create function? >> Or should I remove the invocation above? > > I noticed it in the _destruct function and went back to look at > create. You probably want it in both, unless as part of destruct, you > first disconnect all peers, which should set the carrier to off when > the last peer disconnects? I think keeping the carrier on while no peer is connected is better for OpenVPN. I will then turn the carrier off in the create function as well. Thanks!
On 05/03/2024 20:40, Jakub Kicinski wrote: > On Mon, 4 Mar 2024 16:08:55 +0100 Antonio Quartulli wrote: >> + dev = alloc_netdev(sizeof(struct ovpn_struct), name, NET_NAME_USER, ovpn_setup); >> + >> + dev_net_set(dev, net); > > the dev allocation never fails? Or the error is handles somewhere else? It definitely can fail and dev_net_set will crash. My bad. Thanks for reporting this. Regards,
On Wed, Mar 06, 2024 at 03:49:50PM +0100, Antonio Quartulli wrote: > On 05/03/2024 17:27, Andrew Lunn wrote: > > > > > +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) > > > > > +{ > > > > > + ASSERT_RTNL(); > > > > > + > > > > > + netif_carrier_off(ovpn->dev); > > > > > > > > You often see virtual devices turn their carrier off in there > > > > probe/create function, because it is unclear what state it is in after > > > > register_netdevice(). > > > > > > Are you suggesting to turn it off both here and in the create function? > > > Or should I remove the invocation above? > > > > I noticed it in the _destruct function and went back to look at > > create. You probably want it in both, unless as part of destruct, you > > first disconnect all peers, which should set the carrier to off when > > the last peer disconnects? > > I think keeping the carrier on while no peer is connected is better for > OpenVPN. I then have to wounder what carrier actually means? Some routing protocols will kick off determining routes when the carrier goes down. Can you put team/bonding on top of openvpn? If the peer has gone, you want team to fall over to the active backup? Andrew
On 06/03/2024 20:31, Andrew Lunn wrote: > On Wed, Mar 06, 2024 at 03:49:50PM +0100, Antonio Quartulli wrote: >> On 05/03/2024 17:27, Andrew Lunn wrote: >>>>>> +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) >>>>>> +{ >>>>>> + ASSERT_RTNL(); >>>>>> + >>>>>> + netif_carrier_off(ovpn->dev); >>>>> >>>>> You often see virtual devices turn their carrier off in there >>>>> probe/create function, because it is unclear what state it is in after >>>>> register_netdevice(). >>>> >>>> Are you suggesting to turn it off both here and in the create function? >>>> Or should I remove the invocation above? >>> >>> I noticed it in the _destruct function and went back to look at >>> create. You probably want it in both, unless as part of destruct, you >>> first disconnect all peers, which should set the carrier to off when >>> the last peer disconnects? >> >> I think keeping the carrier on while no peer is connected is better for >> OpenVPN. > > I then have to wounder what carrier actually means? > > Some routing protocols will kick off determining routes when the > carrier goes down. Can you put team/bonding on top of openvpn? If the > peer has gone, you want team to fall over to the active backup? IIRC team/bonding requires a L2 interface, but ovpn creates a L3 device. So I don't think this case applies here. To be honest we don't have any real concept for the carrier off. Especially on a server, I hardly believe it would be any useful. However, on a client or on a p2p link, where there is exactly one remote host on the other side, it may make sense to turn the carrier off when that remote peer is lost. There is an extra detail to consider: if the user wants to, the ovpn device should remain configured (with all IPs and routes) even if openvpn has fully disconnected and it is attempting a reconnection to another server. Reason being avoiding data leaks by accidentally removing routes to the tunnel (traffic that should go through the tunnel would rather go to the local network). With all this being said, it may make sense to just keep the carrier on all time long. Should we come up with something smarter in the future, we can still improve this behaviour. Regards, > > Andrew
> To be honest we don't have any real concept for the carrier off. > Especially on a server, I hardly believe it would be any useful. > > However, on a client or on a p2p link, where there is exactly one remote > host on the other side, it may make sense to turn the carrier off when that > remote peer is lost. > > There is an extra detail to consider: if the user wants to, the ovpn device > should remain configured (with all IPs and routes) even if openvpn has fully > disconnected and it is attempting a reconnection to another server. Reason > being avoiding data leaks by accidentally removing routes to the tunnel > (traffic that should go through the tunnel would rather go to the local > network). > > With all this being said, it may make sense to just keep the carrier on all > time long. > > Should we come up with something smarter in the future, we can still improve > this behaviour. O.K, so.. You already have more patches than recommended for a series, but... I suggest you make the carrier_on in probe() a patch of its own with a good commit message based on the above. We then have a clear explanation in git why the carrier is always on. Andrew
On 08/03/2024 14:13, Andrew Lunn wrote: > You already have more patches than recommended for a series, but... I > suggest you make the carrier_on in probe() a patch of its own with a > good commit message based on the above. We then have a clear > explanation in git why the carrier is always on. ACK. will do. Thanks for the recommendation. Regards,
diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index a1e19402e36d..b283449ba479 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c @@ -8,11 +8,37 @@ */ #include "io.h" +#include "ovpnstruct.h" +#include "netlink.h" #include <linux/netdevice.h> #include <linux/skbuff.h> +int ovpn_struct_init(struct net_device *dev) +{ + struct ovpn_struct *ovpn = netdev_priv(dev); + int err; + + memset(ovpn, 0, sizeof(*ovpn)); + + ovpn->dev = dev; + + err = ovpn_nl_init(ovpn); + if (err < 0) + return err; + + dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats); + if (!dev->tstats) + return -ENOMEM; + + err = security_tun_dev_alloc_security(&ovpn->security); + if (err < 0) + return err; + + return 0; +} + /* Send user data to the network */ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev) diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h index 0a076d14f721..e7728718c8a9 100644 --- a/drivers/net/ovpn/io.h +++ b/drivers/net/ovpn/io.h @@ -14,6 +14,7 @@ struct sk_buff; +int ovpn_struct_init(struct net_device *dev); netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev); #endif /* _NET_OVPN_OVPN_H_ */ diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index 3769f99cfe6f..1be0fd50c356 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -10,15 +10,20 @@ #include "main.h" #include "netlink.h" #include "io.h" +#include "ovpnstruct.h" +#include "packet.h" #include <linux/genetlink.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/types.h> +#include <linux/lockdep.h> #include <linux/net.h> #include <linux/inetdevice.h> #include <linux/netdevice.h> #include <linux/version.h> +#include <net/ip.h> +#include <uapi/linux/if_arp.h> #include <uapi/linux/ovpn.h> @@ -28,6 +33,16 @@ #define DRV_DESCRIPTION "OpenVPN data channel offload (ovpn)" #define DRV_COPYRIGHT "(C) 2020-2024 OpenVPN, Inc." +static LIST_HEAD(dev_list); + +static void ovpn_struct_free(struct net_device *net) +{ + struct ovpn_struct *ovpn = netdev_priv(net); + + security_tun_dev_free_security(ovpn->security); + free_percpu(net->tstats); +} + /* Net device open */ static int ovpn_net_open(struct net_device *dev) { @@ -62,28 +77,120 @@ static const struct net_device_ops ovpn_netdev_ops = { .ndo_get_stats64 = dev_get_tstats64, }; +static void ovpn_setup(struct net_device *dev) +{ + /* compute the overhead considering AEAD encryption */ + const int overhead = sizeof(u32) + NONCE_WIRE_SIZE + 16 + sizeof(struct udphdr) + + max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); + + netdev_features_t feat = NETIF_F_SG | NETIF_F_LLTX | + NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_GSO | + NETIF_F_GSO_SOFTWARE | NETIF_F_HIGHDMA; + + dev->needs_free_netdev = true; + + dev->netdev_ops = &ovpn_netdev_ops; + + dev->priv_destructor = ovpn_struct_free; + + dev->hard_header_len = 0; + dev->addr_len = 0; + dev->mtu = ETH_DATA_LEN - overhead; + dev->min_mtu = IPV4_MIN_MTU; + dev->max_mtu = IP_MAX_MTU - overhead; + + dev->type = ARPHRD_NONE; + dev->flags = IFF_POINTOPOINT | IFF_NOARP; + + dev->features |= feat; + dev->hw_features |= feat; + dev->hw_enc_features |= feat; + + dev->needed_headroom = OVPN_HEAD_ROOM; + dev->needed_tailroom = OVPN_MAX_PADDING; +} + +int ovpn_iface_create(const char *name, enum ovpn_mode mode, struct net *net) +{ + struct net_device *dev; + struct ovpn_struct *ovpn; + int ret; + + dev = alloc_netdev(sizeof(struct ovpn_struct), name, NET_NAME_USER, ovpn_setup); + + dev_net_set(dev, net); + + ret = ovpn_struct_init(dev); + if (ret < 0) + goto err; + + ovpn = netdev_priv(dev); + ovpn->mode = mode; + + rtnl_lock(); + + ret = register_netdevice(dev); + if (ret < 0) { + netdev_dbg(dev, "cannot register interface %s: %d\n", dev->name, ret); + rtnl_unlock(); + goto err; + } + rtnl_unlock(); + + return ret; + +err: + free_netdev(dev); + return ret; +} + +void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_netdev) +{ + ASSERT_RTNL(); + + netif_carrier_off(ovpn->dev); + + list_del(&ovpn->dev_list); + ovpn->registered = false; + + if (unregister_netdev) + unregister_netdevice(ovpn->dev); + + synchronize_net(); +} + 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 */ + list_add(&ovpn->dev_list, &dev_list); + ovpn->registered = true; break; case NETDEV_UNREGISTER: /* can be delivered multiple times, so check registered flag, then * destroy the interface */ + if (!ovpn->registered) + return NOTIFY_DONE; + + ovpn_iface_destruct(ovpn, false); break; case NETDEV_POST_INIT: case NETDEV_GOING_DOWN: case NETDEV_DOWN: case NETDEV_UP: case NETDEV_PRE_UP: + break; default: return NOTIFY_DONE; } diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h index 932eb90953e0..c2cdfd6f84b3 100644 --- a/drivers/net/ovpn/ovpnstruct.h +++ b/drivers/net/ovpn/ovpnstruct.h @@ -10,11 +10,27 @@ #ifndef _NET_OVPN_OVPNSTRUCT_H_ #define _NET_OVPN_OVPNSTRUCT_H_ +#include <uapi/linux/ovpn.h> #include <linux/netdevice.h> +#include <linux/types.h> /* Our state per ovpn interface */ struct ovpn_struct { struct net_device *dev; + + /* whether this device is still registered with netdev or not */ + bool registered; + + /* device operation mode (i.e. P2P, MP) */ + enum ovpn_mode mode; + + unsigned int max_tun_queue_len; + + netdev_features_t set_features; + + void *security; + + struct list_head dev_list; }; #endif /* _NET_OVPN_OVPNSTRUCT_H_ */
Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/io.c | 26 +++++++++ drivers/net/ovpn/io.h | 1 + drivers/net/ovpn/main.c | 107 ++++++++++++++++++++++++++++++++++ drivers/net/ovpn/ovpnstruct.h | 16 +++++ 4 files changed, 150 insertions(+)