Re: [PATCH net-next v9 04/23] ovpn: add basic interface creation/destruction/management routines

Jiri Pirko
2024-10-17T06:39:12Z

Wed, Oct 16, 2024 at 04:24:09PM CEST, antonio@openvpn.net wrote:
>On 16/10/2024 10:27, Jiri Pirko wrote:
>> Wed, Oct 16, 2024 at 03:03:04AM CEST, antonio@openvpn.net wrote:
>> > Add basic infrastructure for handling ovpn interfaces.
>> >
>> > Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>> > ---
>> > drivers/net/ovpn/main.c | 115 ++++++++++++++++++++++++++++++++++++++++--
>> > drivers/net/ovpn/main.h | 7 +++
>> > drivers/net/ovpn/ovpnstruct.h | 8 +++
>> > drivers/net/ovpn/packet.h | 40 +++++++++++++++
>> > include/uapi/linux/if_link.h | 15 ++++++
>> > 5 files changed, 180 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
>> > index d5bdb0055f4dd3a6e32dc6e792bed1e7fd59e101..eead7677b8239eb3c48bb26ca95492d88512b8d4 100644
>> > --- a/drivers/net/ovpn/main.c
>> > +++ b/drivers/net/ovpn/main.c
>> > @@ -10,18 +10,52 @@
>> > #include <linux/genetlink.h>
>> > #include <linux/module.h>
>> > #include <linux/netdevice.h>
>> > +#include <linux/inetdevice.h>
>> > +#include <net/ip.h>
>> > #include <net/rtnetlink.h>
>> > -#include <uapi/linux/ovpn.h>
>> > +#include <uapi/linux/if_arp.h>
>> >
>> > #include "ovpnstruct.h"
>> > #include "main.h"
>> > #include "netlink.h"
>> > #include "io.h"
>> > +#include "packet.h"
>> >
>> > /* Driver info */
>> > #define DRV_DESCRIPTION "OpenVPN data channel offload (ovpn)"
>> > #define DRV_COPYRIGHT "(C) 2020-2024 OpenVPN, Inc."
>> >
>> > +static void ovpn_struct_free(struct net_device *net)
>> > +{
>> > +}
>> > +
>> > +static int ovpn_net_open(struct net_device *dev)
>> > +{
>> > + netif_tx_start_all_queues(dev);
>> > + return 0;
>> > +}
>> > +
>> > +static int ovpn_net_stop(struct net_device *dev)
>> > +{
>> > + netif_tx_stop_all_queues(dev);
>> > + return 0;
>> > +}
>> > +
>> > +static const struct net_device_ops ovpn_netdev_ops = {
>> > + .ndo_open = ovpn_net_open,
>> > + .ndo_stop = ovpn_net_stop,
>> > + .ndo_start_xmit = ovpn_net_xmit,
>> > +};
>> > +
>> > +static const struct device_type ovpn_type = {
>> > + .name = OVPN_FAMILY_NAME,
>> > +};
>> > +
>> > +static const struct nla_policy ovpn_policy[IFLA_OVPN_MAX + 1] = {
>> > + [IFLA_OVPN_MODE] = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_P2P,
>> > + OVPN_MODE_MP),
>> > +};
>> > +
>> > /**
>> > * ovpn_dev_is_valid - check if the netdevice is of type 'ovpn'
>> > * @dev: the interface to check
>> > @@ -33,16 +67,76 @@ bool ovpn_dev_is_valid(const struct net_device *dev)
>> > return dev->netdev_ops->ndo_start_xmit == ovpn_net_xmit;
>> > }
>> >
>> > +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_HW_CSUM | NETIF_F_RXCSUM |
>> > + NETIF_F_GSO | NETIF_F_GSO_SOFTWARE |
>> > + NETIF_F_HIGHDMA;
>> > +
>> > + dev->needs_free_netdev = true;
>> > +
>> > + dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;
>> > +
>> > + 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->priv_flags |= IFF_NO_QUEUE;
>> > +
>> > + dev->lltx = true;
>> > + dev->features |= feat;
>> > + dev->hw_features |= feat;
>> > + dev->hw_enc_features |= feat;
>> > +
>> > + dev->needed_headroom = OVPN_HEAD_ROOM;
>> > + dev->needed_tailroom = OVPN_MAX_PADDING;
>> > +
>> > + SET_NETDEV_DEVTYPE(dev, &ovpn_type);
>> > +}
>> > +
>> > static int ovpn_newlink(struct net *src_net, struct net_device *dev,
>> > struct nlattr *tb[], struct nlattr *data[],
>> > struct netlink_ext_ack *extack)
>> > {
>> > - return -EOPNOTSUPP;
>> > + struct ovpn_struct *ovpn = netdev_priv(dev);
>> > + enum ovpn_mode mode = OVPN_MODE_P2P;
>> > +
>> > + if (data && data[IFLA_OVPN_MODE]) {
>> > + mode = nla_get_u8(data[IFLA_OVPN_MODE]);
>>
>> Some sanity check perhaps? "validate" op is here for that purpose.
>
>Isn't the parsing happening here enough
>
>https://elixir.bootlin.com/linux/v6.12-rc3/source/net/core/rtnetlink.c#L3659
>
>The IFINFO_DATA is parsed using the policy I provided (which comes with
>limits for the mode attribute).

You are right, that seems enough for mode.

>
>Or am I misreading the code and I still need to provide an implementation for
>.validate?
>
>Regards,
>
>>
>>
>> > + netdev_dbg(dev, "setting device mode: %u\n", mode);
>> > + }
>> > +
>> > + ovpn->dev = dev;
>> > + ovpn->mode = mode;
>> > +
>> > + /* turn carrier explicitly off after registration, this way state is
>> > + * clearly defined
>> > + */
>> > + netif_carrier_off(dev);
>> > +
>> > + return register_netdevice(dev);
>>
>> [...]
>
>--
>Antonio Quartulli
>OpenVPN Inc.
>