| Message ID | 20241029-b4-ovpn-v11-0-de4698c73a25@openvpn.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
It seems some little changes to ovpn.yaml were not reflected in the generated files I committed. Specifically I changed some U32 to BE32 (IPv4 addresses) and files were not regenerated before committing. (I saw the failure in patchwork about this) It seems I'll have to send v12 after all. Cheers, On 29/10/2024 11:47, Antonio Quartulli wrote: > Notable changes from v10: > * extended commit message of 23/23 with brief description of the output > * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@openvpn.net > > Please note that some patches were already reviewed by Andre Lunn, > Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag > since no major code modification has happened since the review. > > The latest code can also be found at: > > https://github.com/OpenVPN/linux-kernel-ovpn > > Thanks a lot! > Best Regards, > > Antonio Quartulli > OpenVPN Inc. > > --- > Antonio Quartulli (23): > netlink: add NLA_POLICY_MAX_LEN macro > net: introduce OpenVPN Data Channel Offload (ovpn) > ovpn: add basic netlink support > ovpn: add basic interface creation/destruction/management routines > ovpn: keep carrier always on > ovpn: introduce the ovpn_peer object > ovpn: introduce the ovpn_socket object > ovpn: implement basic TX path (UDP) > ovpn: implement basic RX path (UDP) > ovpn: implement packet processing > ovpn: store tunnel and transport statistics > ovpn: implement TCP transport > ovpn: implement multi-peer support > ovpn: implement peer lookup logic > ovpn: implement keepalive mechanism > ovpn: add support for updating local UDP endpoint > ovpn: add support for peer floating > ovpn: implement peer add/get/dump/delete via netlink > ovpn: implement key add/get/del/swap via netlink > ovpn: kill key and notify userspace in case of IV exhaustion > ovpn: notify userspace when a peer is deleted > ovpn: add basic ethtool support > testing/selftests: add test tool and scripts for ovpn module > > Documentation/netlink/specs/ovpn.yaml | 362 +++ > MAINTAINERS | 11 + > drivers/net/Kconfig | 14 + > drivers/net/Makefile | 1 + > drivers/net/ovpn/Makefile | 22 + > drivers/net/ovpn/bind.c | 54 + > drivers/net/ovpn/bind.h | 117 + > drivers/net/ovpn/crypto.c | 214 ++ > drivers/net/ovpn/crypto.h | 145 ++ > drivers/net/ovpn/crypto_aead.c | 386 ++++ > drivers/net/ovpn/crypto_aead.h | 33 + > drivers/net/ovpn/io.c | 462 ++++ > drivers/net/ovpn/io.h | 25 + > drivers/net/ovpn/main.c | 337 +++ > drivers/net/ovpn/main.h | 24 + > drivers/net/ovpn/netlink-gen.c | 212 ++ > drivers/net/ovpn/netlink-gen.h | 41 + > drivers/net/ovpn/netlink.c | 1135 ++++++++++ > drivers/net/ovpn/netlink.h | 18 + > drivers/net/ovpn/ovpnstruct.h | 61 + > drivers/net/ovpn/packet.h | 40 + > drivers/net/ovpn/peer.c | 1201 ++++++++++ > drivers/net/ovpn/peer.h | 165 ++ > drivers/net/ovpn/pktid.c | 130 ++ > drivers/net/ovpn/pktid.h | 87 + > drivers/net/ovpn/proto.h | 104 + > drivers/net/ovpn/skb.h | 56 + > drivers/net/ovpn/socket.c | 178 ++ > drivers/net/ovpn/socket.h | 55 + > drivers/net/ovpn/stats.c | 21 + > drivers/net/ovpn/stats.h | 47 + > drivers/net/ovpn/tcp.c | 506 +++++ > drivers/net/ovpn/tcp.h | 44 + > drivers/net/ovpn/udp.c | 406 ++++ > drivers/net/ovpn/udp.h | 26 + > include/net/netlink.h | 1 + > include/uapi/linux/if_link.h | 15 + > include/uapi/linux/ovpn.h | 109 + > include/uapi/linux/udp.h | 1 + > tools/net/ynl/ynl-gen-c.py | 4 +- > tools/testing/selftests/Makefile | 1 + > tools/testing/selftests/net/ovpn/.gitignore | 2 + > tools/testing/selftests/net/ovpn/Makefile | 17 + > tools/testing/selftests/net/ovpn/config | 10 + > tools/testing/selftests/net/ovpn/data64.key | 5 + > tools/testing/selftests/net/ovpn/ovpn-cli.c | 2370 ++++++++++++++++++++ > tools/testing/selftests/net/ovpn/tcp_peers.txt | 5 + > .../testing/selftests/net/ovpn/test-chachapoly.sh | 9 + > tools/testing/selftests/net/ovpn/test-float.sh | 9 + > tools/testing/selftests/net/ovpn/test-tcp.sh | 9 + > tools/testing/selftests/net/ovpn/test.sh | 183 ++ > tools/testing/selftests/net/ovpn/udp_peers.txt | 5 + > 52 files changed, 9494 insertions(+), 1 deletion(-) > --- > base-commit: ab101c553bc1f76a839163d1dc0d1e715ad6bb4e > change-id: 20241002-b4-ovpn-eeee35c694a2 > > Best regards,
On Thu, 31 Oct 2024 11:00:05 +0100 Antonio Quartulli wrote: > It seems some little changes to ovpn.yaml were not reflected in the > generated files I committed. > > Specifically I changed some U32 to BE32 (IPv4 addresses) and files were > not regenerated before committing. > > (I saw the failure in patchwork about this) > > It seems I'll have to send v12 after all. I'll apply patch 1 already, it's tangentially related, and no point rebasing.
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 29 Oct 2024 11:47:13 +0100 you wrote: > Notable changes from v10: > * extended commit message of 23/23 with brief description of the output > * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@openvpn.net > > Please note that some patches were already reviewed by Andre Lunn, > Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag > since no major code modification has happened since the review. > > [...] Here is the summary with links: - [net-next,v11,01/23] netlink: add NLA_POLICY_MAX_LEN macro https://git.kernel.org/netdev/net-next/c/4138e9ec0093 - [net-next,v11,02/23] net: introduce OpenVPN Data Channel Offload (ovpn) (no matching commit) - [net-next,v11,03/23] ovpn: add basic netlink support (no matching commit) - [net-next,v11,04/23] ovpn: add basic interface creation/destruction/management routines (no matching commit) - [net-next,v11,05/23] ovpn: keep carrier always on (no matching commit) - [net-next,v11,06/23] ovpn: introduce the ovpn_peer object (no matching commit) - [net-next,v11,07/23] ovpn: introduce the ovpn_socket object (no matching commit) - [net-next,v11,08/23] ovpn: implement basic TX path (UDP) (no matching commit) - [net-next,v11,09/23] ovpn: implement basic RX path (UDP) (no matching commit) - [net-next,v11,10/23] ovpn: implement packet processing (no matching commit) - [net-next,v11,11/23] ovpn: store tunnel and transport statistics (no matching commit) - [net-next,v11,12/23] ovpn: implement TCP transport (no matching commit) - [net-next,v11,13/23] ovpn: implement multi-peer support (no matching commit) - [net-next,v11,14/23] ovpn: implement peer lookup logic (no matching commit) - [net-next,v11,15/23] ovpn: implement keepalive mechanism (no matching commit) - [net-next,v11,16/23] ovpn: add support for updating local UDP endpoint (no matching commit) - [net-next,v11,17/23] ovpn: add support for peer floating (no matching commit) - [net-next,v11,18/23] ovpn: implement peer add/get/dump/delete via netlink (no matching commit) - [net-next,v11,19/23] ovpn: implement key add/get/del/swap via netlink (no matching commit) - [net-next,v11,20/23] ovpn: kill key and notify userspace in case of IV exhaustion (no matching commit) - [net-next,v11,21/23] ovpn: notify userspace when a peer is deleted (no matching commit) - [net-next,v11,22/23] ovpn: add basic ethtool support (no matching commit) - [net-next,v11,23/23] testing/selftests: add test tool and scripts for ovpn module (no matching commit) You are awesome, thank you!
Hi Antonio, On 29.10.2024 12:47, Antonio Quartulli wrote: > Notable changes from v10: > * extended commit message of 23/23 with brief description of the output > * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@openvpn.net > > Please note that some patches were already reviewed by Andre Lunn, > Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag > since no major code modification has happened since the review. > > The latest code can also be found at: > > https://github.com/OpenVPN/linux-kernel-ovpn As I promised many months ago I am starting publishing some nit picks regarding the series. The review was started when series was V3 "rebasing" the review to every next version to publish it at once. But I lost this race to the new version releasing velocity :) So, I am going to publish it patch-by-patch. Anyway you and all participants have done a great progress toward making accelerator part of the kernel. Most of considerable things already resolved so do not wait me please to finish picking every nit. Regarding "big" topics I have only two concerns: link creation using RTNL and a switch statement usage. In the corresponding thread, I asked Jiri to clarify that "should" regarding .newlink implementation. Hope he will have a chance to find a time to reply. For the 'switch' statement, I see a repeating pattern of handling mode-or family-specific cases like this: int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) { switch (ovpn->mode) { case OVPN_MODE_MP: return ovpn_peer_add_mp(ovpn, peer); case OVPN_MODE_P2P: return ovpn_peer_add_p2p(ovpn, peer); default: return -EOPNOTSUPP; } } or void ovpn_encrypt_post(void *data, int ret) { ... switch (peer->sock->sock->sk->sk_protocol) { case IPPROTO_UDP: ovpn_udp_send_skb(peer->ovpn, peer, skb); break; case IPPROTO_TCP: ovpn_tcp_send_skb(peer, skb); break; default: /* no transport configured yet */ goto err; } ... } or void ovpn_peer_keepalive_work(...) { ... switch (ovpn->mode) { case OVPN_MODE_MP: next_run = ovpn_peer_keepalive_work_mp(ovpn, now); break; case OVPN_MODE_P2P: next_run = ovpn_peer_keepalive_work_p2p(ovpn, now); break; } ... } Did you consider to implement mode specific operations as a set of operations like this: ovpn_ops { int (*peer_add)(struct ovpn_struct *ovpn, struct ovpn_peer *peer); int (*peer_del)(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason); void (*send_skb)(struct ovpn_peer *peer, struct sk_buff *skb); time64_t (*keepalive_work)(...); }; Initialize them during the interface creation and invoke these operations indirectly. E.g. int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) { return ovpn->ops->peer_add(ovpn, peer); } void ovpn_encrypt_post(void *data, int ret) { ... ovpn->ops->send_skb(peer, skb); ... } void ovpn_peer_keepalive_work(...) { ... next_run = ovpn->ops->keepalive_work(ovpn, now); ... } Anyway the module has all these option values in advance during the network interface creation phase and I believe replacing 'switch' statements with indirect calls can make code easy to read. -- Sergey
On 06/11/2024 02:18, Sergey Ryazanov wrote: > Hi Antonio, > > On 29.10.2024 12:47, Antonio Quartulli wrote: >> Notable changes from v10: >> * extended commit message of 23/23 with brief description of the output >> * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0- >> b87530777be7@openvpn.net >> >> Please note that some patches were already reviewed by Andre Lunn, >> Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag >> since no major code modification has happened since the review. >> >> The latest code can also be found at: >> >> https://github.com/OpenVPN/linux-kernel-ovpn > > As I promised many months ago I am starting publishing some nit picks > regarding the series. Thanks and welcome back! > The review was started when series was V3 > "rebasing" the review to every next version to publish it at once. But I > lost this race to the new version releasing velocity :) So, I am going > to publish it patch-by-patch. > > Anyway you and all participants have done a great progress toward making > accelerator part of the kernel. Most of considerable things already > resolved so do not wait me please to finish picking every nit. I'll go through them all and judge what's meaningful to add to v12 and what can be postponed for later improvements. > > Regarding "big" topics I have only two concerns: link creation using > RTNL and a switch statement usage. In the corresponding thread, I asked > Jiri to clarify that "should" regarding .newlink implementation. Hope he > will have a chance to find a time to reply. True, but to be honest at this point I am fine with sticking to RTNL, also because we will soon introduce the ability to create 'persistent' ifaces, which a user should be able to create before starting openvpn. Going through RTNL for this is the best choice IMHO, therefore we have an extra use case in favour of this approach (next to what Jiri already mentioned). > > For the 'switch' statement, I see a repeating pattern of handling mode- > or family-specific cases like this: > > int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) > { > switch (ovpn->mode) { > case OVPN_MODE_MP: > return ovpn_peer_add_mp(ovpn, peer); > case OVPN_MODE_P2P: > return ovpn_peer_add_p2p(ovpn, peer); > default: > return -EOPNOTSUPP; > } > } > > or > > void ovpn_encrypt_post(void *data, int ret) > { > ... > switch (peer->sock->sock->sk->sk_protocol) { > case IPPROTO_UDP: > ovpn_udp_send_skb(peer->ovpn, peer, skb); > break; > case IPPROTO_TCP: > ovpn_tcp_send_skb(peer, skb); > break; > default: > /* no transport configured yet */ > goto err; > } > ... > } > > or > > void ovpn_peer_keepalive_work(...) > { > ... > switch (ovpn->mode) { > case OVPN_MODE_MP: > next_run = ovpn_peer_keepalive_work_mp(ovpn, now); > break; > case OVPN_MODE_P2P: > next_run = ovpn_peer_keepalive_work_p2p(ovpn, now); > break; > } > ... > } > > Did you consider to implement mode specific operations as a set of > operations like this: > > ovpn_ops { > int (*peer_add)(struct ovpn_struct *ovpn, struct ovpn_peer *peer); > int (*peer_del)(struct ovpn_peer *peer, enum ovpn_del_peer_reason > reason); > void (*send_skb)(struct ovpn_peer *peer, struct sk_buff *skb); > time64_t (*keepalive_work)(...); > }; > > Initialize them during the interface creation and invoke these > operations indirectly. E.g. > > int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) > { > return ovpn->ops->peer_add(ovpn, peer); > } > > void ovpn_encrypt_post(void *data, int ret) > { > ... > ovpn->ops->send_skb(peer, skb); > ... > } > > void ovpn_peer_keepalive_work(...) > { > ... > next_run = ovpn->ops->keepalive_work(ovpn, now); > ... > } > > Anyway the module has all these option values in advance during the > network interface creation phase and I believe replacing 'switch' > statements with indirect calls can make code easy to read. I see this was already discussed with Sabrina under another patch and I have the same opinion. To me the switch/case approach looks cleaner and I truly like it, especially when enums are involved. ops/callbacks are fine when they can be redefined at runtime (i.e. a proto that can be registered by another module), but this is not the case here. I also feel that with ops it's not easy to understand what call is truly being made by just looking at the caller context and reading can be harder. So I truly prefer to stick to this schema. Thanks a lot for sharing your point though. Regards,
On 14.11.2024 17:33, Antonio Quartulli wrote: > On 06/11/2024 02:18, Sergey Ryazanov wrote: >> Regarding "big" topics I have only two concerns: link creation using >> RTNL and a switch statement usage. In the corresponding thread, I >> asked Jiri to clarify that "should" regarding .newlink implementation. >> Hope he will have a chance to find a time to reply. > > True, but to be honest at this point I am fine with sticking to RTNL, > also because we will soon introduce the ability to create 'persistent' > ifaces, which a user should be able to create before starting openvpn. Could you share the use case for this functionality? > Going through RTNL for this is the best choice IMHO, therefore we have > an extra use case in favour of this approach (next to what Jiri already > mentioned). In absence of arguments it's hard to understand, what's the "best" meaning. So, I'm still not sure is it worth to split uAPI between two interfaces. Anyway, it's up to maintainers to decide is it mergeable in this form or not. I just shared some arguments for the full management interface in GENL. -- Sergey
On 14/11/2024 23:10, Sergey Ryazanov wrote: > On 14.11.2024 17:33, Antonio Quartulli wrote: >> On 06/11/2024 02:18, Sergey Ryazanov wrote: >>> Regarding "big" topics I have only two concerns: link creation using >>> RTNL and a switch statement usage. In the corresponding thread, I >>> asked Jiri to clarify that "should" regarding .newlink >>> implementation. Hope he will have a chance to find a time to reply. >> >> True, but to be honest at this point I am fine with sticking to RTNL, >> also because we will soon introduce the ability to create 'persistent' >> ifaces, which a user should be able to create before starting openvpn. > > Could you share the use case for this functionality? This is better asked to the users mailing list, but, for example, we recently had a discussion about this with the VyOS guys, where they want to create the interface and have it fit the various firewall/routing/chachacha logic, before any daemon is started. In OpenVPN userspace we already support the --mktun directive to help with this specific use case, so it's a long existing use case. > >> Going through RTNL for this is the best choice IMHO, therefore we have >> an extra use case in favour of this approach (next to what Jiri >> already mentioned). > > In absence of arguments it's hard to understand, what's the "best" > meaning. well, that's why I added "IMHO" :) > So, I'm still not sure is it worth to split uAPI between two > interfaces. Anyway, it's up to maintainers to decide is it mergeable in > this form or not. I just shared some arguments for the full management > interface in GENL. well, doing it differently from all other virtual drivers also requires some important reason IMHO. Anyway, I like the idea that iproute2 can be used to create interfaces, without the need to have another userspace tool for that. Regards,
Notable changes from v10: * extended commit message of 23/23 with brief description of the output * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@openvpn.net Please note that some patches were already reviewed by Andre Lunn, Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag since no major code modification has happened since the review. The latest code can also be found at: https://github.com/OpenVPN/linux-kernel-ovpn Thanks a lot! Best Regards, Antonio Quartulli OpenVPN Inc. --- Antonio Quartulli (23): netlink: add NLA_POLICY_MAX_LEN macro net: introduce OpenVPN Data Channel Offload (ovpn) ovpn: add basic netlink support ovpn: add basic interface creation/destruction/management routines ovpn: keep carrier always on ovpn: introduce the ovpn_peer object ovpn: introduce the ovpn_socket object ovpn: implement basic TX path (UDP) ovpn: implement basic RX path (UDP) ovpn: implement packet processing ovpn: store tunnel and transport statistics ovpn: implement TCP transport ovpn: implement multi-peer support ovpn: implement peer lookup logic ovpn: implement keepalive mechanism ovpn: add support for updating local UDP endpoint ovpn: add support for peer floating ovpn: implement peer add/get/dump/delete via netlink ovpn: implement key add/get/del/swap via netlink ovpn: kill key and notify userspace in case of IV exhaustion ovpn: notify userspace when a peer is deleted ovpn: add basic ethtool support testing/selftests: add test tool and scripts for ovpn module Documentation/netlink/specs/ovpn.yaml | 362 +++ MAINTAINERS | 11 + drivers/net/Kconfig | 14 + drivers/net/Makefile | 1 + drivers/net/ovpn/Makefile | 22 + drivers/net/ovpn/bind.c | 54 + drivers/net/ovpn/bind.h | 117 + drivers/net/ovpn/crypto.c | 214 ++ drivers/net/ovpn/crypto.h | 145 ++ drivers/net/ovpn/crypto_aead.c | 386 ++++ drivers/net/ovpn/crypto_aead.h | 33 + drivers/net/ovpn/io.c | 462 ++++ drivers/net/ovpn/io.h | 25 + drivers/net/ovpn/main.c | 337 +++ drivers/net/ovpn/main.h | 24 + drivers/net/ovpn/netlink-gen.c | 212 ++ drivers/net/ovpn/netlink-gen.h | 41 + drivers/net/ovpn/netlink.c | 1135 ++++++++++ drivers/net/ovpn/netlink.h | 18 + drivers/net/ovpn/ovpnstruct.h | 61 + drivers/net/ovpn/packet.h | 40 + drivers/net/ovpn/peer.c | 1201 ++++++++++ drivers/net/ovpn/peer.h | 165 ++ drivers/net/ovpn/pktid.c | 130 ++ drivers/net/ovpn/pktid.h | 87 + drivers/net/ovpn/proto.h | 104 + drivers/net/ovpn/skb.h | 56 + drivers/net/ovpn/socket.c | 178 ++ drivers/net/ovpn/socket.h | 55 + drivers/net/ovpn/stats.c | 21 + drivers/net/ovpn/stats.h | 47 + drivers/net/ovpn/tcp.c | 506 +++++ drivers/net/ovpn/tcp.h | 44 + drivers/net/ovpn/udp.c | 406 ++++ drivers/net/ovpn/udp.h | 26 + include/net/netlink.h | 1 + include/uapi/linux/if_link.h | 15 + include/uapi/linux/ovpn.h | 109 + include/uapi/linux/udp.h | 1 + tools/net/ynl/ynl-gen-c.py | 4 +- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/net/ovpn/.gitignore | 2 + tools/testing/selftests/net/ovpn/Makefile | 17 + tools/testing/selftests/net/ovpn/config | 10 + tools/testing/selftests/net/ovpn/data64.key | 5 + tools/testing/selftests/net/ovpn/ovpn-cli.c | 2370 ++++++++++++++++++++ tools/testing/selftests/net/ovpn/tcp_peers.txt | 5 + .../testing/selftests/net/ovpn/test-chachapoly.sh | 9 + tools/testing/selftests/net/ovpn/test-float.sh | 9 + tools/testing/selftests/net/ovpn/test-tcp.sh | 9 + tools/testing/selftests/net/ovpn/test.sh | 183 ++ tools/testing/selftests/net/ovpn/udp_peers.txt | 5 + 52 files changed, 9494 insertions(+), 1 deletion(-) --- base-commit: ab101c553bc1f76a839163d1dc0d1e715ad6bb4e change-id: 20241002-b4-ovpn-eeee35c694a2 Best regards,