netdev - Re: [PATCH net-next v11 00/23] Introducing OpenVPN Data Channel Offload

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: <2828411f-f2e5-4dfc-80ff-577eb5fd359a@gmail.com>
Date: Wed, 6 Nov 2024 03:18:53 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
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,
 steffen.klassert@...unet.com, antony.antony@...unet.com,
 Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH net-next v11 00/23] Introducing OpenVPN Data Channel
 Offload

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