Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
Hello Jiri, Sorry for the late reply. Could you elaborate a bit reasons for the RTNL interface implementation? Please find the questions inlined. On 08.10.2024 15:52, Jiri Pirko wrote: Tue, Oct 08, 2024 at 11:16:01AM CEST, anto...@openvpn.net wrote: On 08/10/2024 10:58, Jiri Pirko wrote: Tue, Oct 08, 2024 at 10:01:40AM CEST, anto...@openvpn.net wrote: Hi, On 07/10/24 17:32, Jiri Pirko wrote: Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote: [...] +operations: + list: +- + name: dev-new + attribute-set: ovpn + flags: [ admin-perm ] + doc: Create a new interface of type ovpn + do: +request: + attributes: +- ifname +- mode +reply: + attributes: +- ifname +- ifindex +- + name: dev-del Why you expose new and del here in ovn specific generic netlink iface? Why can't you use the exising RTNL api which is used for creation and destruction of other types of devices? That was my original approach in v1, but it was argued that an ovpn interface needs a userspace program to be configured and used in a meaningful way, therefore it was decided to concentrate all iface mgmt APIs along with the others in the netlink family and to not expose any RTNL ops. Can you please point me to the message id? from Sergey and subsequent replies. RTNL vs NL topic starts right after the definition of 'ovpn_link_ops' Yeah, does not make sense to me. All devices should implement common rtnl ops, the extra-config, if needed, could be on a separate channel. I don't find Sergey's argumentation valid. Do we consider word *should* in terms of RFC 2119: SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. I am asking because rtnl_link_register() allows ops without .newlink implementation. What makes .newlink implementation as least optional and gives a freedom in design. Let me briefly summarize my argumentation from the referenced thread. We have two classes of links point-to-point and point-to-multipoint. The major class is PtP and RTNL is perfectly suited to manage it. While PtMP is a minor class and it is an obstacle for RTNL due to need to manage multiple peers. What requires a different interface to manage these peers. Lets call it GENL interface. A PtMP-class netdev without any configured peer is useless, what makes GENL interface for peers management mandatory. Mandatory to implement in both user- and kernel-space. Link creation can be implemented using any of these (RTNL or GENL) interfaces. GENL interface is already mandatory to implement in a user-space software, while RTNL can be considered optional to implement. So, implementing the link creation using GENL requires only a new message support implementation. While implementing the the link creation using RTNL requires a whole new interface implementation (socket read/write, messages demux, etc.). My point is, GENL-only management gives us consolidated and clear solution, while GENL+RTNL requires code duplication and causes a complexity. That's it. Jiri, do you see big flaws in this reasoning? Recently Kuniyuki commented on this topic as well in: <20240919055259.17622-1-kun...@amazon.com> and that is why I added a default dellink implemetation. Having dellink without newlink implemented is just wrong. Could you clarify this statement please? I can not recall any documentation or a code block that enforces .newlink implementation in case of the .dellink presence. Generally speaking, I can understand a feel of irregularity when looking at code implementing delete operation without a link creation counterpart. This confusion can be resolved taking into consideration a difference in a nature of these operations. A new link can not be created automatically while an existing link can be removed automatically without any extra inputs. .newlink designated only for fulfilling user's requests since it requires extra information unavailable inside the kernel. While .dellink has two semantics: (a) user's requests fulfilling, (b) automatic cleanup of unneeded remainders. From that perspective, having an option to implement .dellink without .newlink implementation looks reasonable. However, recently we decided to add a dellink implementation for better integration with network namespaces and to allow the user to wipe a dangling interface. Hmm, one more argument to have symmetric add/del impletentation in RTNL In the future we are planning to also add the possibility to create a "persistent interface", that is an interface created before launching any userspace program and that survives when the latter is stopped. I can guess this functionality may be better suited for
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
On 08/10/2024 14:52, Jiri Pirko wrote:
Tue, Oct 08, 2024 at 11:16:01AM CEST, anto...@openvpn.net wrote:
On 08/10/2024 10:58, Jiri Pirko wrote:
Tue, Oct 08, 2024 at 10:01:40AM CEST, anto...@openvpn.net wrote:
Hi,
On 07/10/24 17:32, Jiri Pirko wrote:
Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote:
[...]
+operations:
+ list:
+-
+ name: dev-new
+ attribute-set: ovpn
+ flags: [ admin-perm ]
+ doc: Create a new interface of type ovpn
+ do:
+request:
+ attributes:
+- ifname
+- mode
+reply:
+ attributes:
+- ifname
+- ifindex
+-
+ name: dev-del
Why you expose new and del here in ovn specific generic netlink iface?
Why can't you use the exising RTNL api which is used for creation and
destruction of other types of devices?
That was my original approach in v1, but it was argued that an ovpn interface
needs a userspace program to be configured and used in a meaningful way,
therefore it was decided to concentrate all iface mgmt APIs along with the
others in the netlink family and to not expose any RTNL ops.
Can you please point me to the message id?
from
Sergey and subsequent replies.
RTNL vs NL topic starts right after the definition of 'ovpn_link_ops'
Yeah, does not make sense to me. All devices should implement common
rtnl ops, the extra-config, if needed, could be on a separate channel.
I don't find Sergey's argumentation valid.
Thanks a lot for taking the time to read our conversation.
Ok, considering all points we have discussed so far (including future
developments already on the roadmap), I think it makes sense to go back
to RTNL and drop the new/del-dev ops from the netlink family.
Will do that in v9.
Regards,
Recently Kuniyuki commented on this topic as well in:
<20240919055259.17622-1-kun...@amazon.com>
and that is why I added a default dellink implemetation.
Having dellink without newlink implemented is just wrong.
However, recently we decided to add a dellink implementation for better
integration with network namespaces and to allow the user to wipe a dangling
interface.
Hmm, one more argument to have symmetric add/del impletentation in RTNL
In the future we are planning to also add the possibility to create a
"persistent interface", that is an interface created before launching any
userspace program and that survives when the latter is stopped.
I can guess this functionality may be better suited for RTNL, but I am not
sure yet.
That would be quite confusing to have RTNL and genetlink iface to
add/del device. From what you described above, makes more sent to have
it just in RTNL
All in all I tend to agree.
@Jiri: do you have any particular opinion why we should use RTNL ops and not
netlink for creating/destroying interfaces? I feel this is mostly a matter of
taste, but maybe there are technical reasons we should consider.
Well. technically, you can probabaly do both. But it is quite common
that you can add/delete these kind of devices over RTNL. Lots of
examples. People are used to it, aligns with existing flows.
The only counterargument I see is the one brought by Sergey: "the ovpn
interface is not usable after creation, if no openvpn process is running".
However, allowing to create "persistent interfaces" will define a use-case
for having an ovpn device without any userspace process.
@Sergey what is your opinion here? I am not sure persistent interfaces were
discussed at the time you brought your point about RTNL vs NL.
Regards,
Thanks a lot for your contribution.
Regards,
ip link add [link DEV | parentdev NAME] [ name ] NAME
[ txqueuelen PACKETS ]
[ address LLADDR ]
[ broadcast LLADDR ]
[ mtu MTU ] [index IDX ]
[ numtxqueues QUEUE_COUNT ]
[ numrxqueues QUEUE_COUNT ]
[ netns { PID | NETNSNAME | NETNSFILE } ]
type TYPE [ ARGS ]
ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS ]
Lots of examples of existing types creation is for example here:
https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
+ attribute-set: ovpn
+ flags: [ admin-perm ]
+ doc: Delete existing interface of type ovpn
+ do:
+pre: ovpn-nl-pre-doit
+post: ovpn-nl-post-doit
+request:
+ attributes:
+- ifindex
[...]
--
Antonio Quartulli
OpenVPN Inc.
--
Antonio Quartulli
OpenVPN Inc.
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
Tue, Oct 08, 2024 at 11:16:01AM CEST, anto...@openvpn.net wrote:
>On 08/10/2024 10:58, Jiri Pirko wrote:
>> Tue, Oct 08, 2024 at 10:01:40AM CEST, anto...@openvpn.net wrote:
>> > Hi,
>> >
>> > On 07/10/24 17:32, Jiri Pirko wrote:
>> > > Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote:
>> > >
>> > > [...]
>> > >
>> > >
>> > > > +operations:
>> > > > + list:
>> > > > +-
>> > > > + name: dev-new
>> > > > + attribute-set: ovpn
>> > > > + flags: [ admin-perm ]
>> > > > + doc: Create a new interface of type ovpn
>> > > > + do:
>> > > > +request:
>> > > > + attributes:
>> > > > +- ifname
>> > > > +- mode
>> > > > +reply:
>> > > > + attributes:
>> > > > +- ifname
>> > > > +- ifindex
>> > > > +-
>> > > > + name: dev-del
>> > >
>> > > Why you expose new and del here in ovn specific generic netlink iface?
>> > > Why can't you use the exising RTNL api which is used for creation and
>> > > destruction of other types of devices?
>> >
>> > That was my original approach in v1, but it was argued that an ovpn
>> > interface
>> > needs a userspace program to be configured and used in a meaningful way,
>> > therefore it was decided to concentrate all iface mgmt APIs along with the
>> > others in the netlink family and to not expose any RTNL ops.
>>
>> Can you please point me to the message id?
>
> from
>Sergey and subsequent replies.
>RTNL vs NL topic starts right after the definition of 'ovpn_link_ops'
Yeah, does not make sense to me. All devices should implement common
rtnl ops, the extra-config, if needed, could be on a separate channel.
I don't find Sergey's argumentation valid.
>
>Recently Kuniyuki commented on this topic as well in:
><20240919055259.17622-1-kun...@amazon.com>
>and that is why I added a default dellink implemetation.
Having dellink without newlink implemented is just wrong.
>
>>
>>
>> >
>> > However, recently we decided to add a dellink implementation for better
>> > integration with network namespaces and to allow the user to wipe a
>> > dangling
>> > interface.
>>
>> Hmm, one more argument to have symmetric add/del impletentation in RTNL
>>
>>
>> >
>> > In the future we are planning to also add the possibility to create a
>> > "persistent interface", that is an interface created before launching any
>> > userspace program and that survives when the latter is stopped.
>> > I can guess this functionality may be better suited for RTNL, but I am not
>> > sure yet.
>>
>> That would be quite confusing to have RTNL and genetlink iface to
>> add/del device. From what you described above, makes more sent to have
>> it just in RTNL
>
>All in all I tend to agree.
>
>>
>> >
>> > @Jiri: do you have any particular opinion why we should use RTNL ops and
>> > not
>> > netlink for creating/destroying interfaces? I feel this is mostly a matter
>> > of
>> > taste, but maybe there are technical reasons we should consider.
>>
>> Well. technically, you can probabaly do both. But it is quite common
>> that you can add/delete these kind of devices over RTNL. Lots of
>> examples. People are used to it, aligns with existing flows.
>
>The only counterargument I see is the one brought by Sergey: "the ovpn
>interface is not usable after creation, if no openvpn process is running".
>
>However, allowing to create "persistent interfaces" will define a use-case
>for having an ovpn device without any userspace process.
>
>@Sergey what is your opinion here? I am not sure persistent interfaces were
>discussed at the time you brought your point about RTNL vs NL.
>
>
>Regards,
>
>
>>
>> >
>> > Thanks a lot for your contribution.
>> >
>> > Regards,
>> >
>> >
>> > >
>> > >
>> > > ip link add [link DEV | parentdev NAME] [ name ] NAME
>> > > [ txqueuelen PACKETS ]
>> > > [ address LLADDR ]
>> > > [ broadcast LLADDR ]
>> > > [ mtu MTU ] [index IDX ]
>> > > [ numtxqueues QUEUE_COUNT ]
>> > > [ numrxqueues QUEUE_COUNT ]
>> > > [ netns { PID | NETNSNAME | NETNSFILE } ]
>> > > type TYPE [ ARGS ]
>> > >
>> > > ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS
>> > > ]
>> > >
>> > > Lots of examples of existing types creation is for example here:
>> > > https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
>> > >
>> > >
>> > >
>> > > > + attribute-set: ovpn
>> > > > + flags: [ admin-perm ]
>> > > > + doc: Delete existing interface of type ovpn
>> > > > + do:
>> > > > +pre: ovpn-nl-pre-doit
>> > > > +post: ovpn-nl-post-doit
>> > > > +request:
>> > > > + attributes:
>> > > > +- ifindex
>> > >
>> > > [...]
>> >
>> > --
>> > Antonio Quartulli
>> > OpenVPN Inc.
>
>--
>Antonio Quartulli
>OpenVPN Inc.
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
On 08/10/2024 10:58, Jiri Pirko wrote:
Tue, Oct 08, 2024 at 10:01:40AM CEST, anto...@openvpn.net wrote:
Hi,
On 07/10/24 17:32, Jiri Pirko wrote:
Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote:
[...]
+operations:
+ list:
+-
+ name: dev-new
+ attribute-set: ovpn
+ flags: [ admin-perm ]
+ doc: Create a new interface of type ovpn
+ do:
+request:
+ attributes:
+- ifname
+- mode
+reply:
+ attributes:
+- ifname
+- ifindex
+-
+ name: dev-del
Why you expose new and del here in ovn specific generic netlink iface?
Why can't you use the exising RTNL api which is used for creation and
destruction of other types of devices?
That was my original approach in v1, but it was argued that an ovpn interface
needs a userspace program to be configured and used in a meaningful way,
therefore it was decided to concentrate all iface mgmt APIs along with the
others in the netlink family and to not expose any RTNL ops.
Can you please point me to the message id?
from Sergey and subsequent replies.
RTNL vs NL topic starts right after the definition of 'ovpn_link_ops'
Recently Kuniyuki commented on this topic as well in:
<20240919055259.17622-1-kun...@amazon.com>
and that is why I added a default dellink implemetation.
However, recently we decided to add a dellink implementation for better
integration with network namespaces and to allow the user to wipe a dangling
interface.
Hmm, one more argument to have symmetric add/del impletentation in RTNL
In the future we are planning to also add the possibility to create a
"persistent interface", that is an interface created before launching any
userspace program and that survives when the latter is stopped.
I can guess this functionality may be better suited for RTNL, but I am not
sure yet.
That would be quite confusing to have RTNL and genetlink iface to
add/del device. From what you described above, makes more sent to have
it just in RTNL
All in all I tend to agree.
@Jiri: do you have any particular opinion why we should use RTNL ops and not
netlink for creating/destroying interfaces? I feel this is mostly a matter of
taste, but maybe there are technical reasons we should consider.
Well. technically, you can probabaly do both. But it is quite common
that you can add/delete these kind of devices over RTNL. Lots of
examples. People are used to it, aligns with existing flows.
The only counterargument I see is the one brought by Sergey: "the ovpn
interface is not usable after creation, if no openvpn process is running".
However, allowing to create "persistent interfaces" will define a
use-case for having an ovpn device without any userspace process.
@Sergey what is your opinion here? I am not sure persistent interfaces
were discussed at the time you brought your point about RTNL vs NL.
Regards,
Thanks a lot for your contribution.
Regards,
ip link add [link DEV | parentdev NAME] [ name ] NAME
[ txqueuelen PACKETS ]
[ address LLADDR ]
[ broadcast LLADDR ]
[ mtu MTU ] [index IDX ]
[ numtxqueues QUEUE_COUNT ]
[ numrxqueues QUEUE_COUNT ]
[ netns { PID | NETNSNAME | NETNSFILE } ]
type TYPE [ ARGS ]
ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS ]
Lots of examples of existing types creation is for example here:
https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
+ attribute-set: ovpn
+ flags: [ admin-perm ]
+ doc: Delete existing interface of type ovpn
+ do:
+pre: ovpn-nl-pre-doit
+post: ovpn-nl-post-doit
+request:
+ attributes:
+- ifindex
[...]
--
Antonio Quartulli
OpenVPN Inc.
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
Tue, Oct 08, 2024 at 10:01:40AM CEST, anto...@openvpn.net wrote:
>Hi,
>
>On 07/10/24 17:32, Jiri Pirko wrote:
>> Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote:
>>
>> [...]
>>
>>
>> > +operations:
>> > + list:
>> > +-
>> > + name: dev-new
>> > + attribute-set: ovpn
>> > + flags: [ admin-perm ]
>> > + doc: Create a new interface of type ovpn
>> > + do:
>> > +request:
>> > + attributes:
>> > +- ifname
>> > +- mode
>> > +reply:
>> > + attributes:
>> > +- ifname
>> > +- ifindex
>> > +-
>> > + name: dev-del
>>
>> Why you expose new and del here in ovn specific generic netlink iface?
>> Why can't you use the exising RTNL api which is used for creation and
>> destruction of other types of devices?
>
>That was my original approach in v1, but it was argued that an ovpn interface
>needs a userspace program to be configured and used in a meaningful way,
>therefore it was decided to concentrate all iface mgmt APIs along with the
>others in the netlink family and to not expose any RTNL ops.
Can you please point me to the message id?
>
>However, recently we decided to add a dellink implementation for better
>integration with network namespaces and to allow the user to wipe a dangling
>interface.
Hmm, one more argument to have symmetric add/del impletentation in RTNL
>
>In the future we are planning to also add the possibility to create a
>"persistent interface", that is an interface created before launching any
>userspace program and that survives when the latter is stopped.
>I can guess this functionality may be better suited for RTNL, but I am not
>sure yet.
That would be quite confusing to have RTNL and genetlink iface to
add/del device. From what you described above, makes more sent to have
it just in RTNL
>
>@Jiri: do you have any particular opinion why we should use RTNL ops and not
>netlink for creating/destroying interfaces? I feel this is mostly a matter of
>taste, but maybe there are technical reasons we should consider.
Well. technically, you can probabaly do both. But it is quite common
that you can add/delete these kind of devices over RTNL. Lots of
examples. People are used to it, aligns with existing flows.
>
>Thanks a lot for your contribution.
>
>Regards,
>
>
>>
>>
>> ip link add [link DEV | parentdev NAME] [ name ] NAME
>> [ txqueuelen PACKETS ]
>> [ address LLADDR ]
>> [ broadcast LLADDR ]
>> [ mtu MTU ] [index IDX ]
>> [ numtxqueues QUEUE_COUNT ]
>> [ numrxqueues QUEUE_COUNT ]
>> [ netns { PID | NETNSNAME | NETNSFILE } ]
>> type TYPE [ ARGS ]
>>
>> ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS ]
>>
>> Lots of examples of existing types creation is for example here:
>> https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
>>
>>
>>
>> > + attribute-set: ovpn
>> > + flags: [ admin-perm ]
>> > + doc: Delete existing interface of type ovpn
>> > + do:
>> > +pre: ovpn-nl-pre-doit
>> > +post: ovpn-nl-post-doit
>> > +request:
>> > + attributes:
>> > +- ifindex
>>
>> [...]
>
>--
>Antonio Quartulli
>OpenVPN Inc.
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
Hi,
On 07/10/24 17:32, Jiri Pirko wrote:
Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote:
[...]
+operations:
+ list:
+-
+ name: dev-new
+ attribute-set: ovpn
+ flags: [ admin-perm ]
+ doc: Create a new interface of type ovpn
+ do:
+request:
+ attributes:
+- ifname
+- mode
+reply:
+ attributes:
+- ifname
+- ifindex
+-
+ name: dev-del
Why you expose new and del here in ovn specific generic netlink iface?
Why can't you use the exising RTNL api which is used for creation and
destruction of other types of devices?
That was my original approach in v1, but it was argued that an ovpn
interface needs a userspace program to be configured and used in a
meaningful way, therefore it was decided to concentrate all iface mgmt
APIs along with the others in the netlink family and to not expose any
RTNL ops.
However, recently we decided to add a dellink implementation for better
integration with network namespaces and to allow the user to wipe a
dangling interface.
In the future we are planning to also add the possibility to create a
"persistent interface", that is an interface created before launching
any userspace program and that survives when the latter is stopped.
I can guess this functionality may be better suited for RTNL, but I am
not sure yet.
@Jiri: do you have any particular opinion why we should use RTNL ops and
not netlink for creating/destroying interfaces? I feel this is mostly a
matter of taste, but maybe there are technical reasons we should consider.
Thanks a lot for your contribution.
Regards,
ip link add [link DEV | parentdev NAME] [ name ] NAME
[ txqueuelen PACKETS ]
[ address LLADDR ]
[ broadcast LLADDR ]
[ mtu MTU ] [index IDX ]
[ numtxqueues QUEUE_COUNT ]
[ numrxqueues QUEUE_COUNT ]
[ netns { PID | NETNSNAME | NETNSFILE } ]
type TYPE [ ARGS ]
ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS ]
Lots of examples of existing types creation is for example here:
https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
+ attribute-set: ovpn
+ flags: [ admin-perm ]
+ doc: Delete existing interface of type ovpn
+ do:
+pre: ovpn-nl-pre-doit
+post: ovpn-nl-post-doit
+request:
+ attributes:
+- ifindex
[...]
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
Wed, Oct 02, 2024 at 11:02:17AM CEST, anto...@openvpn.net wrote:
[...]
>+operations:
>+ list:
>+-
>+ name: dev-new
>+ attribute-set: ovpn
>+ flags: [ admin-perm ]
>+ doc: Create a new interface of type ovpn
>+ do:
>+request:
>+ attributes:
>+- ifname
>+- mode
>+reply:
>+ attributes:
>+- ifname
>+- ifindex
>+-
>+ name: dev-del
Why you expose new and del here in ovn specific generic netlink iface?
Why can't you use the exising RTNL api which is used for creation and
destruction of other types of devices?
ip link add [link DEV | parentdev NAME] [ name ] NAME
[ txqueuelen PACKETS ]
[ address LLADDR ]
[ broadcast LLADDR ]
[ mtu MTU ] [index IDX ]
[ numtxqueues QUEUE_COUNT ]
[ numrxqueues QUEUE_COUNT ]
[ netns { PID | NETNSNAME | NETNSFILE } ]
type TYPE [ ARGS ]
ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS ]
Lots of examples of existing types creation is for example here:
https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
>+ attribute-set: ovpn
>+ flags: [ admin-perm ]
>+ doc: Delete existing interface of type ovpn
>+ do:
>+pre: ovpn-nl-pre-doit
>+post: ovpn-nl-post-doit
>+request:
>+ attributes:
>+- ifindex
[...]
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
On 04/10/2024 18:13, Donald Hunter wrote: On Wed, 2 Oct 2024 at 10:03, Antonio Quartulli wrote: +definitions: + - +type: const +name: nonce-tail-size +value: 8 + - +type: enum +name: cipher-alg +value-start: 0 value-start defaults to 0 for enum so this is unnecessary. Same for the following enum definitions. ACK +entries: [ none, aes-gcm, chacha20-poly1305 ] + - +type: enum +name: del-peer-reason +value-start: 0 +entries: [ teardown, userspace, expired, transport-error, transport-disconnect ] + - +type: enum +name: key-slot +value-start: 0 +entries: [ primary, secondary ] + - +type: enum +name: mode +value-start: 0 +entries: [ p2p, mp ] + [...] +operations: + list: +- + name: dev-new + attribute-set: ovpn + flags: [ admin-perm ] + doc: Create a new interface of type ovpn + do: +request: + attributes: +- ifname +- mode +reply: + attributes: +- ifname +- ifindex +- + name: dev-del + attribute-set: ovpn + flags: [ admin-perm ] + doc: Delete existing interface of type ovpn + do: +pre: ovpn-nl-pre-doit +post: ovpn-nl-post-doit +request: + attributes: +- ifindex There's no dev-get do/dump op. I think there should be one for diagnostics and metrics. I am not sure how much information it can provide (as of now we only have the 'mode' that is being set upon creation). In any case, I am not against implementing the op now and extend it later as we see fit. +- + name: key-new + attribute-set: ovpn + flags: [ admin-perm ] + doc: Add a cipher key for a specific peer + do: +pre: ovpn-nl-pre-doit +post: ovpn-nl-post-doit +request: + attributes: +- ifindex +- keyconf +- + name: key-swap + attribute-set: ovpn + flags: [ admin-perm ] + doc: Swap primary and secondary session keys for a specific peer + do: +pre: ovpn-nl-pre-doit +post: ovpn-nl-post-doit +request: + attributes: +- ifindex +- keyconf +- + name: key-swap-ntf + notify: key-new This doesn't work because key-new doesn't have a reply. You should define it with an event: block instead. You can see the build errors here: make -C tools/net/ynl Oh, I wasn't aware of this subfolder. Thanks for pointing it out! I am thinking that it may make sense to implement a key-get op to extract non-sensible data about the keys (i.e. what cipher was configured). This may be useful for debugging as well. At that point the key-swap-ntf can re-use the key-get as notify. Cheers, CC ovpn-user.o In file included from ovpn-user.c:8: ovpn-user.h:1194:33: error: field ‘obj’ has incomplete type 1194 | struct ovpn_key_new_rsp obj __attribute__((aligned(8))); | ^~~ ovpn-user.c:835:35: error: ‘ovpn_key_new_rsp_parse’ undeclared here (not in a function); did you mean ‘ovpn_dev_new_rsp_parse’? 835 | .cb = ovpn_key_new_rsp_parse, | ^~ | ovpn_dev_new_rsp_parse make[1]: *** [Makefile:41: ovpn-user.o] Error 1 + doc: | +Notification about key having exhausted its IV space and requiring +renegotiation + mcgrp: peers +- + name: key-del + attribute-set: ovpn + flags: [ admin-perm ] + doc: Delete cipher key for a specific peer + do: +pre: ovpn-nl-pre-doit +post: ovpn-nl-post-doit +request: + attributes: +- ifindex +- keyconf + +mcast-groups: + list: +- + name: peers -- Antonio Quartulli OpenVPN Inc.
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
On Wed, 2 Oct 2024 at 10:03, Antonio Quartulli wrote: > > +definitions: > + - > +type: const > +name: nonce-tail-size > +value: 8 > + - > +type: enum > +name: cipher-alg > +value-start: 0 value-start defaults to 0 for enum so this is unnecessary. Same for the following enum definitions. > +entries: [ none, aes-gcm, chacha20-poly1305 ] > + - > +type: enum > +name: del-peer-reason > +value-start: 0 > +entries: [ teardown, userspace, expired, transport-error, > transport-disconnect ] > + - > +type: enum > +name: key-slot > +value-start: 0 > +entries: [ primary, secondary ] > + - > +type: enum > +name: mode > +value-start: 0 > +entries: [ p2p, mp ] > + [...] > +operations: > + list: > +- > + name: dev-new > + attribute-set: ovpn > + flags: [ admin-perm ] > + doc: Create a new interface of type ovpn > + do: > +request: > + attributes: > +- ifname > +- mode > +reply: > + attributes: > +- ifname > +- ifindex > +- > + name: dev-del > + attribute-set: ovpn > + flags: [ admin-perm ] > + doc: Delete existing interface of type ovpn > + do: > +pre: ovpn-nl-pre-doit > +post: ovpn-nl-post-doit > +request: > + attributes: > +- ifindex There's no dev-get do/dump op. I think there should be one for diagnostics and metrics. > +- > + name: key-new > + attribute-set: ovpn > + flags: [ admin-perm ] > + doc: Add a cipher key for a specific peer > + do: > +pre: ovpn-nl-pre-doit > +post: ovpn-nl-post-doit > +request: > + attributes: > +- ifindex > +- keyconf > +- > + name: key-swap > + attribute-set: ovpn > + flags: [ admin-perm ] > + doc: Swap primary and secondary session keys for a specific peer > + do: > +pre: ovpn-nl-pre-doit > +post: ovpn-nl-post-doit > +request: > + attributes: > +- ifindex > +- keyconf > +- > + name: key-swap-ntf > + notify: key-new This doesn't work because key-new doesn't have a reply. You should define it with an event: block instead. You can see the build errors here: make -C tools/net/ynl CC ovpn-user.o In file included from ovpn-user.c:8: ovpn-user.h:1194:33: error: field ‘obj’ has incomplete type 1194 | struct ovpn_key_new_rsp obj __attribute__((aligned(8))); | ^~~ ovpn-user.c:835:35: error: ‘ovpn_key_new_rsp_parse’ undeclared here (not in a function); did you mean ‘ovpn_dev_new_rsp_parse’? 835 | .cb = ovpn_key_new_rsp_parse, | ^~ | ovpn_dev_new_rsp_parse make[1]: *** [Makefile:41: ovpn-user.o] Error 1 > + doc: | > +Notification about key having exhausted its IV space and requiring > +renegotiation > + mcgrp: peers > +- > + name: key-del > + attribute-set: ovpn > + flags: [ admin-perm ] > + doc: Delete cipher key for a specific peer > + do: > +pre: ovpn-nl-pre-doit > +post: ovpn-nl-post-doit > +request: > + attributes: > +- ifindex > +- keyconf > + > +mcast-groups: > + list: > +- > + name: peers
Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support
Hi Antonio, kernel test robot noticed the following build warnings: [auto build test WARNING on 44badc908f2c85711cb18e45e13119c10ad3a05f] url: https://github.com/intel-lab-lkp/linux/commits/Antonio-Quartulli/netlink-add-NLA_POLICY_MAX_LEN-macro/20241002-172734 base: 44badc908f2c85711cb18e45e13119c10ad3a05f patch link: https://lore.kernel.org/r/20241002-b4-ovpn-v8-3-37ceffcffbde%40openvpn.net patch subject: [PATCH net-next v8 03/24] ovpn: add basic netlink support reproduce: (https://download.01.org/0day-ci/archive/20241002/202410022156.mxbrg3on-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202410022156.mxbrg3on-...@intel.com/ All warnings (new ones prefixed by >>): Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom >> Warning: MAINTAINERS references a file that doesn't exist: >> Documentation/netlink/spec/ovpn.yaml Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt Using alabaster theme -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
