linux-kernel - Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8f3a9ca-698d-4b4e-ab4b-7d8aa651dddc@openvpn.net>
Date: Fri, 15 Nov 2024 11:05:24 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, 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>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 03/23] ovpn: add basic netlink support

On 09/11/2024 00:15, Sergey Ryazanov wrote:
> On 29.10.2024 12:47, Antonio Quartulli wrote:
>> This commit introduces basic netlink support with family
>> registration/unregistration functionalities and stub pre/post-doit.
>>
>> More importantly it introduces the YAML uAPI description along
>> with its auto-generated files:
>> - include/uapi/linux/ovpn.h
>> - drivers/net/ovpn/netlink-gen.c
>> - drivers/net/ovpn/netlink-gen.h
>>
>> Cc: donald.hunter@...il.com
>> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>
> 
> [skipped]
> 
>> diff --git a/Documentation/netlink/specs/ovpn.yaml b/Documentation/ 
>> netlink/specs/ovpn.yaml
> 
> [skipped]
> 
>> +attribute-sets:
>> +  -
>> +    name: peer
>> +    attributes:
>> +      -
>> +        name: id
>> +        type: u32
>> +        doc: |
>> +          The unique ID of the peer. To be used to identify peers during
>> +          operations
> 
> nit: could you specify the scope of uniqueness? I believe it is not 
> globally uniq, it is just interface uniq, right?

Yeah it's per interface/instance.
Will make it more clear, also for other IDs.

> 
>> +        checks:
>> +          max: 0xFFFFFF
> 
> [skipped]
> 
>> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
>> index 
>> 369a5a2b2fc1a497c8444e59f9b058eb40e49524..d5bdb0055f4dd3a6e32dc6e792bed1e7fd59e101 100644
>> --- a/drivers/net/ovpn/main.c
>> +++ b/drivers/net/ovpn/main.c
>> @@ -7,11 +7,15 @@
>>    *        James Yonan <james@...nvpn.net>
>>    */
>> +#include <linux/genetlink.h>
>>   #include <linux/module.h>
>>   #include <linux/netdevice.h>
>>   #include <net/rtnetlink.h>
>> +#include <uapi/linux/ovpn.h>
>> +#include "ovpnstruct.h"
>>   #include "main.h"
>> +#include "netlink.h"
>>   #include "io.h"
>>   /* Driver info */
>> @@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net, struct 
>> net_device *dev,
>>   }
>>   static struct rtnl_link_ops ovpn_link_ops = {
>> -    .kind = "ovpn",
>> +    .kind = OVPN_FAMILY_NAME,
> 
> nit: are you sure that the link kind is the same as the GENL family? I 
> mean, they are both deriviated from the protocol name that is common for 
> both entities, but is it making RTNL kind a derivative of GENL family?

I just want to use the same name everywhere and I thought it doesn't 
make sense to create a separate define (they can be decoupled later 
should see any need for that).
But I can add:

#define OVPN_RTNL_LINK_KIND OVPN_FAMILY_NAME

to make this relationship explicit?

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.