Message-ID: <cfd45410-a7b2-4304-a376-1d7a3b443a13@openvpn.net>
Date: Fri, 15 Nov 2024 14:00:31 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 sd@...asysnail.net, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v11 04/23] ovpn: add basic interface
 creation/destruction/management routines

On 09/11/2024 02:01, Sergey Ryazanov wrote:
> On 29.10.2024 12:47, Antonio Quartulli wrote:
>> Add basic infrastructure for handling ovpn interfaces.
>>
>> Signed-off-by: Antonio Quartulli <antonio@...nvpn.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)
>> +{
>> +}
> 
> nit: since this handler is not mandatory, its introduction can be moved 
> to the later patch, which actually fills it with meaningful operations.


ehmm sure I will move it



> 
>> +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,
> 
> nit: same question here regarding name deriviation. Are you sure that 
> the device type name is the same as the GENL family name?

Like I said in the previous patch, I want all representative strings to 
be "ovpn", that is already the netlink family name.
But I can create another constant to document this explicitly.


> 
>> +};
>> +
>> +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 +
> 
> Where are these magic sizeof(u32) and '16' came from?

It's in the "nice diagram" you commented later in this patch :-)
But I can extend the comment.

[...]


>> @@ -51,26 +145,37 @@ 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);
> 
> nit: netdev_priv() returns only a pointer, it is safe to fetch the 
> pointer in advance, but do not dereference it until we are sure that an 
> event references the desired interface type. Takin this into 
> consideration, the assignment of private data pointer can be moved above 
> to the variable declaration. Just to make code couple of lines shorter.

I do it here because it seems more "logically correct" to retrieve the 
priv pointer after having confirmed that this is a ovpn interface with 
ovpn_dev_is_valid().

Moving it above kinda says "I already know there is a ovpn object here", 
but this is not the case until after the valid() check. So I prefer to 
keep it here.

[...]

>> --- a/drivers/net/ovpn/main.h
>> +++ b/drivers/net/ovpn/main.h
>> @@ -12,4 +12,11 @@
>>   bool ovpn_dev_is_valid(const struct net_device *dev);
>> +#define SKB_HEADER_LEN                                       \
>> +    (max(sizeof(struct iphdr), sizeof(struct ipv6hdr)) + \
>> +     sizeof(struct udphdr) + NET_SKB_PAD)
>> +
>> +#define OVPN_HEAD_ROOM ALIGN(16 + SKB_HEADER_LEN, 4)
> 
> Where is this magic '16' came from?

should be the same 16 af the over head above (it's the auth tag len)
Will make this more explicit with a comment.

> 
>> +#define OVPN_MAX_PADDING 16
>> +
>>   #endif /* _NET_OVPN_MAIN_H_ */
>> diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ 
>> ovpnstruct.h
>> index 
>> e3e4df6418b081436378fc51d98db5bd7b5d1fbe..211df871538d34fdff90d182f21a0b0fb11b28ad 100644
>> --- a/drivers/net/ovpn/ovpnstruct.h
>> +++ b/drivers/net/ovpn/ovpnstruct.h
>> @@ -11,15 +11,23 @@
>>   #define _NET_OVPN_OVPNSTRUCT_H_
>>   #include <net/net_trackers.h>
>> +#include <uapi/linux/if_link.h>
>> +#include <uapi/linux/ovpn.h>
>>   /**
>>    * struct ovpn_struct - per ovpn interface state
>>    * @dev: the actual netdev representing the tunnel
>>    * @dev_tracker: reference tracker for associated dev
>> + * @registered: whether dev is still registered with netdev or not
>> + * @mode: device operation mode (i.e. p2p, mp, ..)
>> + * @dev_list: entry for the module wide device list
>>    */
>>   struct ovpn_struct {
>>       struct net_device *dev;
>>       netdevice_tracker dev_tracker;
>> +    bool registered;
>> +    enum ovpn_mode mode;
>> +    struct list_head dev_list;
> 
> dev_list is no more used and should be deleted.

ACK

[...]

>> +
>> +/* OpenVPN nonce size */
>> +#define NONCE_SIZE 12
> 
> nit: is using the common 'OVPN_' prefix here and for other constants any 
> good idea? E.g. OVPN_NONCE_SIZE. It can give some hints where it comes 
> from for a code reader.

ACK

> 
> And another one question. Could you clarify in the comment to this 
> constant where it came from? AFAIU, these 12 bytes is the expectation of 
> the nonce size of AEAD crypto protocol, rigth?

Correct: 12bytes/96bits. Will extend the comment.

> 
>> +
>> +/* OpenVPN nonce size reduced by 8-byte nonce tail -- this is the
>> + * size of the AEAD Associated Data (AD) sent over the wire
>> + * and is normally the head of the IV
>> + */
>> +#define NONCE_WIRE_SIZE (NONCE_SIZE - sizeof(struct ovpn_nonce_tail))
> 
> If the headers and IV are defined as structures, we no more need this 
> constant since the header construction will be done by a compiler 
> according to the structure layout.

yap yap. Will do this later as explained in the other email.

> 
>> +/* Last 8 bytes of AEAD nonce
>> + * Provided by userspace and usually derived from
>> + * key material generated during TLS handshake
>> + */
>> +struct ovpn_nonce_tail {
>> +    u8 u8[OVPN_NONCE_TAIL_SIZE];
>> +};
> 
> Why do you need a dadicated structure for this array? Can we declare the 
> corresponding fields like this:
> 
> u8 nonce_tail_xmit[OVPN_NONCE_TAIL_SIZE];
> u8 nonce_tail_recv[OVPN_NONCE_TAIL_SIZE];
> 

I think the original reason was to have something to pass to sizeof() 
without making it harder for the reader.

At some point I also wanted to get rid of the struct,but something 
stopped me. Not sure what was though. Will give it a try.


Thanks a lot.
Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists