Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 21.11.2024 23:23, Antonio Quartulli wrote:
On 21/11/2024 00:22, Sergey Ryazanov wrote:
On 13.11.2024 12:03, Sabrina Dubroca wrote:
2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote:
On 12.11.2024 19:31, Sabrina Dubroca wrote:
2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
An ovpn_peer object holds the whole status of a remote peer
(regardless whether it is a server or a client).
This includes status for crypto, tx/rx buffers, napi, etc.
Only support for one peer is introduced (P2P mode).
Multi peer support is introduced with a later patch.
Reviewing the peer creation/destroying code I came to a generic
question.
Did you consider keeping a single P2P peer in the peers table as
well?
Looks like such approach can greatly simply the code by dropping
all these
'switch (ovpn->mode)' checks and implementing a unified peer
management. The
'peer' field in the main private data structure can be kept to
accelerate
lookups, still using peers table for management tasks like
removing all the
peers on the interface teardown.
It would save a few 'switch(mode)', but force every client to allocate
the hashtable for no reason at all. That tradeoff doesn't look very
beneficial to me, the P2P-specific code is really simple. And if you
keep ovpn->peer to make lookups faster, you're not removing that many
'switch(mode)'.
Looking at the done review, I can retrospectively conclude that I
personally
do not like short 'switch' statements and special handlers :)
Seriously, this module has a highest density of switches per KLOC
from what
I have seen before and a major part of it dedicated to handle the
special
case of P2P connection.
I think it's fine. Either way there will be two implementations of
whatever mode-dependent operation needs to be done. switch doesn't
make it more complex than an ops structure.
If you're reading the current version and find ovpn_peer_add, you see
directly that it'll do either ovpn_peer_add_mp or
ovpn_peer_add_p2p. With an ops structure, you'd have a call to
ovpn->ops->peer_add, and you'd have to look up all possible ops
structures to know that it can be either ovpn_peer_add_mp or
ovpn_peer_add_p2p. If there's an undefined number of implementations
living in different modules (like net_device_ops, or L4 protocols),
you don't have a choice.
xfrm went the opposite way to what you're proposing a few years ago
(see commit 0c620e97b349 ("xfrm: remove output indirection from
xfrm_mode") and others), and it made the code simpler.
I checked this. Florian did a nice rework. And the way of
implementation looks reasonable since there are more than two
encapsulation modes and handling is more complex than just selecting a
function to call.
What I don't like about switches, that it requires extra lines of code
and pushes an author to introduce a default case with error handling.
It was mentioned that the module unlikely going to support more than
two modes. In this context shall we consider ternary operator usage.
E.g.:
the default case can actually be dropped. That way we can have the
compiler warn when one of the enum values is not handled in the switch
(should there be a new one at some point).
However, the default is just a sanity check against future code changes
which may introduce a bug.
next_run = ovpn->mode == OVPN_MODE_P2P ?
ovpn_peer_keepalive_work_p2p(...) :
ovpn_peer_keepalive_work_mp(...);
I find this ugly to read :-)
Yeah. Doesn't look pretty as well.
Just to conclude the discussion. Considering what we discussed here and
the Sabrina's point regarding the trampoline penalty for indirect
invocation, we do not have a better solution for now other than using
switches everywhere.
--
Sergey
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 20/11/2024 12:56, Sabrina Dubroca wrote: 2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote: +/** + * struct ovpn_peer - the main remote peer object + * @ovpn: main openvpn instance this peer belongs to + * @id: unique identifier + * @vpn_addrs: IP addresses assigned over the tunnel + * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel + * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel + * @dst_cache: cache for dst_entry used to send to peer + * @bind: remote peer binding + * @halt: true if ovpn_peer_mark_delete was called nit: It's initialized to false in ovpn_peer_new, but then never set to true nor read. Drop it? argh. leftover from some older version. Thanks + * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) + * @lock: protects binding to peer (bind) nit: as well as the keepalive values that are introduced later? (I guess the comment should be fixed up in patch 15 when the keepalive mechanism is added) ACK -- Antonio Quartulli OpenVPN Inc.
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 21/11/2024 00:22, Sergey Ryazanov wrote:
On 13.11.2024 12:03, Sabrina Dubroca wrote:
2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote:
On 12.11.2024 19:31, Sabrina Dubroca wrote:
2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
An ovpn_peer object holds the whole status of a remote peer
(regardless whether it is a server or a client).
This includes status for crypto, tx/rx buffers, napi, etc.
Only support for one peer is introduced (P2P mode).
Multi peer support is introduced with a later patch.
Reviewing the peer creation/destroying code I came to a generic
question.
Did you consider keeping a single P2P peer in the peers table as well?
Looks like such approach can greatly simply the code by dropping
all these
'switch (ovpn->mode)' checks and implementing a unified peer
management. The
'peer' field in the main private data structure can be kept to
accelerate
lookups, still using peers table for management tasks like removing
all the
peers on the interface teardown.
It would save a few 'switch(mode)', but force every client to allocate
the hashtable for no reason at all. That tradeoff doesn't look very
beneficial to me, the P2P-specific code is really simple. And if you
keep ovpn->peer to make lookups faster, you're not removing that many
'switch(mode)'.
Looking at the done review, I can retrospectively conclude that I
personally
do not like short 'switch' statements and special handlers :)
Seriously, this module has a highest density of switches per KLOC
from what
I have seen before and a major part of it dedicated to handle the
special
case of P2P connection.
I think it's fine. Either way there will be two implementations of
whatever mode-dependent operation needs to be done. switch doesn't
make it more complex than an ops structure.
If you're reading the current version and find ovpn_peer_add, you see
directly that it'll do either ovpn_peer_add_mp or
ovpn_peer_add_p2p. With an ops structure, you'd have a call to
ovpn->ops->peer_add, and you'd have to look up all possible ops
structures to know that it can be either ovpn_peer_add_mp or
ovpn_peer_add_p2p. If there's an undefined number of implementations
living in different modules (like net_device_ops, or L4 protocols),
you don't have a choice.
xfrm went the opposite way to what you're proposing a few years ago
(see commit 0c620e97b349 ("xfrm: remove output indirection from
xfrm_mode") and others), and it made the code simpler.
I checked this. Florian did a nice rework. And the way of implementation
looks reasonable since there are more than two encapsulation modes and
handling is more complex than just selecting a function to call.
What I don't like about switches, that it requires extra lines of code
and pushes an author to introduce a default case with error handling. It
was mentioned that the module unlikely going to support more than two
modes. In this context shall we consider ternary operator usage. E.g.:
the default case can actually be dropped. That way we can have the
compiler warn when one of the enum values is not handled in the switch
(should there be a new one at some point).
However, the default is just a sanity check against future code changes
which may introduce a bug.
next_run = ovpn->mode == OVPN_MODE_P2P ?
ovpn_peer_keepalive_work_p2p(...) :
ovpn_peer_keepalive_work_mp(...);
I find this ugly to read :-)
The switch is much more elegant and straightforward.
Do you agree this is getting more into a bike shed coloring discussion? :-D
Since there is not much gain in changing approach, I think it is better
if the maintainer picks a style that he finds more suitable (or simply
likes more). no?
And back to the hashtable(s) size for the MP mode. 8k-bins table looks a
good choice for a normal server with 1-2Gb uplink serving up to 1k
connections. But it sill unclear, how this choice can affect
installations
with a bigger number of connections? Or is this module applicable for
embedded solutions? E.g. running a couple of VPN servers on a home
router
with a few actual connections looks like a waste of RAM. I was about to
suggest to use rhashtable due to its dynamic sizing feature, but the
module
needs three tables. Any better idea?
For this initial implementation I think it's fine. Sure, converting to
rhashtable (or some other type of dynamically-sized hashtable, if
rhashtable doesn't fit) in the future would make sense. But I don't
think it's necessary to get the patches into net-next.
Agreed. It's in the pipe (along with other features that I have already
implemented), but it will come later.
Regards,
Make sense. Thanks for sharing these thoughts.
--
Sergey
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 13.11.2024 12:03, Sabrina Dubroca wrote:
2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote:
On 12.11.2024 19:31, Sabrina Dubroca wrote:
2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
An ovpn_peer object holds the whole status of a remote peer
(regardless whether it is a server or a client).
This includes status for crypto, tx/rx buffers, napi, etc.
Only support for one peer is introduced (P2P mode).
Multi peer support is introduced with a later patch.
Reviewing the peer creation/destroying code I came to a generic question.
Did you consider keeping a single P2P peer in the peers table as well?
Looks like such approach can greatly simply the code by dropping all these
'switch (ovpn->mode)' checks and implementing a unified peer management. The
'peer' field in the main private data structure can be kept to accelerate
lookups, still using peers table for management tasks like removing all the
peers on the interface teardown.
It would save a few 'switch(mode)', but force every client to allocate
the hashtable for no reason at all. That tradeoff doesn't look very
beneficial to me, the P2P-specific code is really simple. And if you
keep ovpn->peer to make lookups faster, you're not removing that many
'switch(mode)'.
Looking at the done review, I can retrospectively conclude that I personally
do not like short 'switch' statements and special handlers :)
Seriously, this module has a highest density of switches per KLOC from what
I have seen before and a major part of it dedicated to handle the special
case of P2P connection.
I think it's fine. Either way there will be two implementations of
whatever mode-dependent operation needs to be done. switch doesn't
make it more complex than an ops structure.
If you're reading the current version and find ovpn_peer_add, you see
directly that it'll do either ovpn_peer_add_mp or
ovpn_peer_add_p2p. With an ops structure, you'd have a call to
ovpn->ops->peer_add, and you'd have to look up all possible ops
structures to know that it can be either ovpn_peer_add_mp or
ovpn_peer_add_p2p. If there's an undefined number of implementations
living in different modules (like net_device_ops, or L4 protocols),
you don't have a choice.
xfrm went the opposite way to what you're proposing a few years ago
(see commit 0c620e97b349 ("xfrm: remove output indirection from
xfrm_mode") and others), and it made the code simpler.
I checked this. Florian did a nice rework. And the way of implementation
looks reasonable since there are more than two encapsulation modes and
handling is more complex than just selecting a function to call.
What I don't like about switches, that it requires extra lines of code
and pushes an author to introduce a default case with error handling. It
was mentioned that the module unlikely going to support more than two
modes. In this context shall we consider ternary operator usage. E.g.:
next_run = ovpn->mode == OVPN_MODE_P2P ?
ovpn_peer_keepalive_work_p2p(...) :
ovpn_peer_keepalive_work_mp(...);
And back to the hashtable(s) size for the MP mode. 8k-bins table looks a
good choice for a normal server with 1-2Gb uplink serving up to 1k
connections. But it sill unclear, how this choice can affect installations
with a bigger number of connections? Or is this module applicable for
embedded solutions? E.g. running a couple of VPN servers on a home router
with a few actual connections looks like a waste of RAM. I was about to
suggest to use rhashtable due to its dynamic sizing feature, but the module
needs three tables. Any better idea?
For this initial implementation I think it's fine. Sure, converting to
rhashtable (or some other type of dynamically-sized hashtable, if
rhashtable doesn't fit) in the future would make sense. But I don't
think it's necessary to get the patches into net-next.
Make sense. Thanks for sharing these thoughts.
--
Sergey
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote: > +/** > + * struct ovpn_peer - the main remote peer object > + * @ovpn: main openvpn instance this peer belongs to > + * @id: unique identifier > + * @vpn_addrs: IP addresses assigned over the tunnel > + * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel > + * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel > + * @dst_cache: cache for dst_entry used to send to peer > + * @bind: remote peer binding > + * @halt: true if ovpn_peer_mark_delete was called nit: It's initialized to false in ovpn_peer_new, but then never set to true nor read. Drop it? > + * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) > + * @lock: protects binding to peer (bind) nit: as well as the keepalive values that are introduced later? (I guess the comment should be fixed up in patch 15 when the keepalive mechanism is added) -- Sabrina
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 10/11/2024 20:52, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
[...]
+static void ovpn_peer_release(struct ovpn_peer *peer)
+{
+ ovpn_bind_reset(peer, NULL);
+
nit: this empty line after ovpn_bind_reset() is removed in the
'implement basic TX path (UDP)' patch. What tricks git and it produces a
sensless diff with 'ovpn_bind_reset(...)' line beeing removed and then
introduced again. If you do not like this empty line then remove it
here, please :)
Thanks! will make sure it won't be introduced at all.
Regards,
+ dst_cache_destroy(&peer->dst_cache);
+ netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
+ kfree_rcu(peer, rcu);
+}
--
Sergey
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
2024-11-13, 03:37:13 +0200, Sergey Ryazanov wrote:
> On 12.11.2024 19:31, Sabrina Dubroca wrote:
> > 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote:
> > > On 29.10.2024 12:47, Antonio Quartulli wrote:
> > > > An ovpn_peer object holds the whole status of a remote peer
> > > > (regardless whether it is a server or a client).
> > > >
> > > > This includes status for crypto, tx/rx buffers, napi, etc.
> > > >
> > > > Only support for one peer is introduced (P2P mode).
> > > > Multi peer support is introduced with a later patch.
> > >
> > > Reviewing the peer creation/destroying code I came to a generic question.
> > > Did you consider keeping a single P2P peer in the peers table as well?
> > >
> > > Looks like such approach can greatly simply the code by dropping all these
> > > 'switch (ovpn->mode)' checks and implementing a unified peer management.
> > > The
> > > 'peer' field in the main private data structure can be kept to accelerate
> > > lookups, still using peers table for management tasks like removing all
> > > the
> > > peers on the interface teardown.
> >
> > It would save a few 'switch(mode)', but force every client to allocate
> > the hashtable for no reason at all. That tradeoff doesn't look very
> > beneficial to me, the P2P-specific code is really simple. And if you
> > keep ovpn->peer to make lookups faster, you're not removing that many
> > 'switch(mode)'.
>
> Looking at the done review, I can retrospectively conclude that I personally
> do not like short 'switch' statements and special handlers :)
>
> Seriously, this module has a highest density of switches per KLOC from what
> I have seen before and a major part of it dedicated to handle the special
> case of P2P connection.
I think it's fine. Either way there will be two implementations of
whatever mode-dependent operation needs to be done. switch doesn't
make it more complex than an ops structure.
If you're reading the current version and find ovpn_peer_add, you see
directly that it'll do either ovpn_peer_add_mp or
ovpn_peer_add_p2p. With an ops structure, you'd have a call to
ovpn->ops->peer_add, and you'd have to look up all possible ops
structures to know that it can be either ovpn_peer_add_mp or
ovpn_peer_add_p2p. If there's an undefined number of implementations
living in different modules (like net_device_ops, or L4 protocols),
you don't have a choice.
xfrm went the opposite way to what you're proposing a few years ago
(see commit 0c620e97b349 ("xfrm: remove output indirection from
xfrm_mode") and others), and it made the code simpler.
> What together look too unusual, so it feels like a
> flaw in the design.
I don't think it's a flaw in the design, maybe just different needs
from other code you've seen (but similar in some ways to xfrm).
> I racked my brains to come up with a better solution and
> failed. So I took a different approach, inviting people to discuss item
> pieces of the code to find a solution collectively or to realize that there
> is no better solution for now.
Sure. And I think there is no better solution, so I'm answering this
thread to say that.
> The problem is that all these hash tables become inefficient with the single
> entry (P2P case). I was thinking about allocating a table with a single bin,
> but it still requires hash function run to access the indexed entry.
And the current implementation relies on fixed-size hashtables
(hash_for_each_safe -> HASH_SIZE -> ARRAY_SIZE -> sizeof).
> And back to the hashtable(s) size for the MP mode. 8k-bins table looks a
> good choice for a normal server with 1-2Gb uplink serving up to 1k
> connections. But it sill unclear, how this choice can affect installations
> with a bigger number of connections? Or is this module applicable for
> embedded solutions? E.g. running a couple of VPN servers on a home router
> with a few actual connections looks like a waste of RAM. I was about to
> suggest to use rhashtable due to its dynamic sizing feature, but the module
> needs three tables. Any better idea?
For this initial implementation I think it's fine. Sure, converting to
rhashtable (or some other type of dynamically-sized hashtable, if
rhashtable doesn't fit) in the future would make sense. But I don't
think it's necessary to get the patches into net-next.
--
Sabrina
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 12.11.2024 19:31, Sabrina Dubroca wrote: 2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: On 29.10.2024 12:47, Antonio Quartulli wrote: An ovpn_peer object holds the whole status of a remote peer (regardless whether it is a server or a client). This includes status for crypto, tx/rx buffers, napi, etc. Only support for one peer is introduced (P2P mode). Multi peer support is introduced with a later patch. Reviewing the peer creation/destroying code I came to a generic question. Did you consider keeping a single P2P peer in the peers table as well? Looks like such approach can greatly simply the code by dropping all these 'switch (ovpn->mode)' checks and implementing a unified peer management. The 'peer' field in the main private data structure can be kept to accelerate lookups, still using peers table for management tasks like removing all the peers on the interface teardown. It would save a few 'switch(mode)', but force every client to allocate the hashtable for no reason at all. That tradeoff doesn't look very beneficial to me, the P2P-specific code is really simple. And if you keep ovpn->peer to make lookups faster, you're not removing that many 'switch(mode)'. Looking at the done review, I can retrospectively conclude that I personally do not like short 'switch' statements and special handlers :) Seriously, this module has a highest density of switches per KLOC from what I have seen before and a major part of it dedicated to handle the special case of P2P connection. What together look too unusual, so it feels like a flaw in the design. I racked my brains to come up with a better solution and failed. So I took a different approach, inviting people to discuss item pieces of the code to find a solution collectively or to realize that there is no better solution for now. The problem is that all these hash tables become inefficient with the single entry (P2P case). I was thinking about allocating a table with a single bin, but it still requires hash function run to access the indexed entry. And back to the hashtable(s) size for the MP mode. 8k-bins table looks a good choice for a normal server with 1-2Gb uplink serving up to 1k connections. But it sill unclear, how this choice can affect installations with a bigger number of connections? Or is this module applicable for embedded solutions? E.g. running a couple of VPN servers on a home router with a few actual connections looks like a waste of RAM. I was about to suggest to use rhashtable due to its dynamic sizing feature, but the module needs three tables. Any better idea? -- Sergey
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
2024-11-10, 15:38:27 +0200, Sergey Ryazanov wrote: > On 29.10.2024 12:47, Antonio Quartulli wrote: > > An ovpn_peer object holds the whole status of a remote peer > > (regardless whether it is a server or a client). > > > > This includes status for crypto, tx/rx buffers, napi, etc. > > > > Only support for one peer is introduced (P2P mode). > > Multi peer support is introduced with a later patch. > > Reviewing the peer creation/destroying code I came to a generic question. > Did you consider keeping a single P2P peer in the peers table as well? > > Looks like such approach can greatly simply the code by dropping all these > 'switch (ovpn->mode)' checks and implementing a unified peer management. The > 'peer' field in the main private data structure can be kept to accelerate > lookups, still using peers table for management tasks like removing all the > peers on the interface teardown. It would save a few 'switch(mode)', but force every client to allocate the hashtable for no reason at all. That tradeoff doesn't look very beneficial to me, the P2P-specific code is really simple. And if you keep ovpn->peer to make lookups faster, you're not removing that many 'switch(mode)'. -- Sabrina
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 05/11/2024 14:12, Sabrina Dubroca wrote:
2024-10-30, 21:47:58 +0100, Antonio Quartulli wrote:
On 30/10/2024 17:37, Sabrina Dubroca wrote:
2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote:
+static void ovpn_peer_release(struct ovpn_peer *peer)
+{
+ ovpn_bind_reset(peer, NULL);
+
+ dst_cache_destroy(&peer->dst_cache);
Is it safe to destroy the cache at this time? In the same function, we
use rcu to free the peer, but AFAICT the dst_cache will be freed
immediately:
void dst_cache_destroy(struct dst_cache *dst_cache)
{
[...]
free_percpu(dst_cache->cache);
}
(probably no real issue because ovpn_udp_send_skb gets called while we
hold a reference to the peer?)
Right.
That was my assumption: release happens on refcnt = 0 only, therefore no
field should be in use anymore.
Anything that may still be in use will have its own refcounter.
My worry is that code changes over time, assumptions are forgotten,
and we end up with code that was a bit odd but safe not being safe
anymore.
Yeah, makes sense.
I'll move the call to dst_cache_destroy() and to kfree(peer) in a RCU
callback.
Thanks!
+ netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
+ kfree_rcu(peer, rcu);
+}
[...]
+static int ovpn_peer_del_p2p(struct ovpn_peer *peer,
+enum ovpn_del_peer_reason reason)
+ __must_hold(&peer->ovpn->lock)
+{
+ struct ovpn_peer *tmp;
+
+ tmp = rcu_dereference_protected(peer->ovpn->peer,
+ lockdep_is_held(&peer->ovpn->lock));
+ if (tmp != peer) {
+ DEBUG_NET_WARN_ON_ONCE(1);
+ if (tmp)
+ ovpn_peer_put(tmp);
Does peer->ovpn->peer need to be set to NULL here as well? Or is it
going to survive this _put?
First of all consider that this is truly something that we don't expect to
happen (hence the WARN_ON).
If this is happening it's because we are trying to delete a peer that is not
the one we are connected to (unexplainable scenario in p2p mode).
Still, should we hit this case (I truly can't see how), I'd say "leave
everything as is - maybe this call was just a mistake".
Yeah, true, let's leave it. Thanks.
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 29.10.2024 12:47, Antonio Quartulli wrote:
[...]
+static void ovpn_peer_release(struct ovpn_peer *peer)
+{
+ ovpn_bind_reset(peer, NULL);
+
nit: this empty line after ovpn_bind_reset() is removed in the
'implement basic TX path (UDP)' patch. What tricks git and it produces a
sensless diff with 'ovpn_bind_reset(...)' line beeing removed and then
introduced again. If you do not like this empty line then remove it
here, please :)
+ dst_cache_destroy(&peer->dst_cache);
+ netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
+ kfree_rcu(peer, rcu);
+}
--
Sergey
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 29.10.2024 12:47, Antonio Quartulli wrote:
An ovpn_peer object holds the whole status of a remote peer
(regardless whether it is a server or a client).
This includes status for crypto, tx/rx buffers, napi, etc.
Only support for one peer is introduced (P2P mode).
Multi peer support is introduced with a later patch.
Reviewing the peer creation/destroying code I came to a generic
question. Did you consider keeping a single P2P peer in the peers table
as well?
Looks like such approach can greatly simply the code by dropping all
these 'switch (ovpn->mode)' checks and implementing a unified peer
management. The 'peer' field in the main private data structure can be
kept to accelerate lookups, still using peers table for management tasks
like removing all the peers on the interface teardown.
Along with the ovpn_peer, also the ovpn_bind object is introcued
as the two are strictly related.
An ovpn_bind object wraps a sockaddr representing the local
coordinates being used to talk to a specific peer.
Signed-off-by: Antonio Quartulli
---
drivers/net/ovpn/Makefile | 2 +
drivers/net/ovpn/bind.c | 58 +++
drivers/net/ovpn/bind.h | 117 ++
Why do we need these bind.c/bind.h files? They contains a minimum of
code and still anyway references the peer object. Can we merge these
definitions and code into peer.c/peer.h?
drivers/net/ovpn/main.c | 11 ++
drivers/net/ovpn/main.h | 2 +
drivers/net/ovpn/ovpnstruct.h | 4 +
drivers/net/ovpn/peer.c | 354 ++
drivers/net/ovpn/peer.h | 79 ++
8 files changed, 627 insertions(+)
[...]
diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c
new file mode 100644
index
..b4d2ccec2ceddf43bc445b489cc62a578ef0ad0a
--- /dev/null
+++ b/drivers/net/ovpn/bind.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/* OpenVPN data channel offload
+ *
+ * Copyright (C) 2012-2024 OpenVPN, Inc.
+ *
+ * Author:James Yonan
+ * Antonio Quartulli
+ */
+
+#include
+#include
+
+#include "ovpnstruct.h"
+#include "bind.h"
+#include "peer.h"
+
+/**
+ * ovpn_bind_from_sockaddr - retrieve binding matching sockaddr
+ * @ss: the sockaddr to match
+ *
+ * Return: the bind matching the passed sockaddr if found, NULL otherwise
The function returns ERR_PTR() in case of error, the comment should be
updated.
+ */
+struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *ss)
+{
+ struct ovpn_bind *bind;
+ size_t sa_len;
+
+ if (ss->ss_family == AF_INET)
+ sa_len = sizeof(struct sockaddr_in);
+ else if (ss->ss_family == AF_INET6)
+ sa_len = sizeof(struct sockaddr_in6);
+ else
+ return ERR_PTR(-EAFNOSUPPORT);
+
+ bind = kzalloc(sizeof(*bind), GFP_ATOMIC);
+ if (unlikely(!bind))
+ return ERR_PTR(-ENOMEM);
+
+ memcpy(&bind->remote, ss, sa_len);
+
+ return bind;
+}
+
+/**
+ * ovpn_bind_reset - assign new binding to peer
+ * @peer: the peer whose binding has to be replaced
+ * @new: the new bind to assign
+ */
+void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *new)
+{
+ struct ovpn_bind *old;
+
+ spin_lock_bh(&peer->lock);
+ old = rcu_replace_pointer(peer->bind, new, true);
+ spin_unlock_bh(&peer->lock);
Locking will be removed from this function in the subsequent patch.
Should we move the peer->lock usage to ovpn_peer_release() now?
+
+ kfree_rcu(old, rcu);
+}
diff --git a/drivers/net/ovpn/bind.h b/drivers/net/ovpn/bind.h
new file mode 100644
index
..859213d5040deb36c416eafcf5c6ab31c4d52c7a
--- /dev/null
+++ b/drivers/net/ovpn/bind.h
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* OpenVPN data channel offload
+ *
+ * Copyright (C) 2012-2024 OpenVPN, Inc.
+ *
+ * Author:James Yonan
+ * Antonio Quartulli
+ */
+
+#ifndef _NET_OVPN_OVPNBIND_H_
+#define _NET_OVPN_OVPNBIND_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct ovpn_peer;
+
+/**
+ * union ovpn_sockaddr - basic transport layer address
Why do we need this dedicated named union? Can we merge this union into
the ovpn_bind struct as already done for the local address?
+ * @in4: IPv4 address
+ * @in6: IPv6 address
+ */
+union ovpn_sockaddr {
Family type can be putted here as a dedicated element to make address
type check simple:
unsigned short int sa_family;
+ struct sockaddr_in in4;
+ struct sockaddr_in6 in6;
+};> +
+/**
+ * struct ovpn_bind - remote peer binding
+ * @remote: the remote peer sockaddress
+ * @local: local endpoint used to talk to the peer
+ * @local.ipv4: local IPv4 used to talk to the peer
+ * @local.ipv6: local IPv6 used to talk to the peer
+ * @rcu: used to schedule RCU cleanup job
+ */
+struct ovpn_bind {
+
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
2024-10-30, 21:47:58 +0100, Antonio Quartulli wrote:
> On 30/10/2024 17:37, Sabrina Dubroca wrote:
> > 2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote:
> > > +static void ovpn_peer_release(struct ovpn_peer *peer)
> > > +{
> > > + ovpn_bind_reset(peer, NULL);
> > > +
> > > + dst_cache_destroy(&peer->dst_cache);
> >
> > Is it safe to destroy the cache at this time? In the same function, we
> > use rcu to free the peer, but AFAICT the dst_cache will be freed
> > immediately:
> >
> > void dst_cache_destroy(struct dst_cache *dst_cache)
> > {
> > [...]
> > free_percpu(dst_cache->cache);
> > }
> >
> > (probably no real issue because ovpn_udp_send_skb gets called while we
> > hold a reference to the peer?)
>
> Right.
> That was my assumption: release happens on refcnt = 0 only, therefore no
> field should be in use anymore.
> Anything that may still be in use will have its own refcounter.
My worry is that code changes over time, assumptions are forgotten,
and we end up with code that was a bit odd but safe not being safe
anymore.
> >
> > > + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
> > > + kfree_rcu(peer, rcu);
> > > +}
> >
> >
> > [...]
> > > +static int ovpn_peer_del_p2p(struct ovpn_peer *peer,
> > > + enum ovpn_del_peer_reason reason)
> > > + __must_hold(&peer->ovpn->lock)
> > > +{
> > > + struct ovpn_peer *tmp;
> > > +
> > > + tmp = rcu_dereference_protected(peer->ovpn->peer,
> > > + lockdep_is_held(&peer->ovpn->lock));
> > > + if (tmp != peer) {
> > > + DEBUG_NET_WARN_ON_ONCE(1);
> > > + if (tmp)
> > > + ovpn_peer_put(tmp);
> >
> > Does peer->ovpn->peer need to be set to NULL here as well? Or is it
> > going to survive this _put?
>
> First of all consider that this is truly something that we don't expect to
> happen (hence the WARN_ON).
> If this is happening it's because we are trying to delete a peer that is not
> the one we are connected to (unexplainable scenario in p2p mode).
>
> Still, should we hit this case (I truly can't see how), I'd say "leave
> everything as is - maybe this call was just a mistake".
Yeah, true, let's leave it. Thanks.
--
Sabrina
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
On 30/10/2024 17:37, Sabrina Dubroca wrote:
2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote:
+static void ovpn_peer_release(struct ovpn_peer *peer)
+{
+ ovpn_bind_reset(peer, NULL);
+
+ dst_cache_destroy(&peer->dst_cache);
Is it safe to destroy the cache at this time? In the same function, we
use rcu to free the peer, but AFAICT the dst_cache will be freed
immediately:
void dst_cache_destroy(struct dst_cache *dst_cache)
{
[...]
free_percpu(dst_cache->cache);
}
(probably no real issue because ovpn_udp_send_skb gets called while we
hold a reference to the peer?)
Right.
That was my assumption: release happens on refcnt = 0 only, therefore no
field should be in use anymore.
Anything that may still be in use will have its own refcounter.
+ netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
+ kfree_rcu(peer, rcu);
+}
[...]
+static int ovpn_peer_del_p2p(struct ovpn_peer *peer,
+enum ovpn_del_peer_reason reason)
+ __must_hold(&peer->ovpn->lock)
+{
+ struct ovpn_peer *tmp;
+
+ tmp = rcu_dereference_protected(peer->ovpn->peer,
+ lockdep_is_held(&peer->ovpn->lock));
+ if (tmp != peer) {
+ DEBUG_NET_WARN_ON_ONCE(1);
+ if (tmp)
+ ovpn_peer_put(tmp);
Does peer->ovpn->peer need to be set to NULL here as well? Or is it
going to survive this _put?
First of all consider that this is truly something that we don't expect
to happen (hence the WARN_ON).
If this is happening it's because we are trying to delete a peer that is
not the one we are connected to (unexplainable scenario in p2p mode).
Still, should we hit this case (I truly can't see how), I'd say "leave
everything as is - maybe this call was just a mistake".
Cheers,
+
+ return -ENOENT;
+ }
+
+ tmp->delete_reason = reason;
+ RCU_INIT_POINTER(peer->ovpn->peer, NULL);
+ ovpn_peer_put(tmp);
+
+ return 0;
+}
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 06/23] ovpn: introduce the ovpn_peer object
2024-10-29, 11:47:19 +0100, Antonio Quartulli wrote:
> +static void ovpn_peer_release(struct ovpn_peer *peer)
> +{
> + ovpn_bind_reset(peer, NULL);
> +
> + dst_cache_destroy(&peer->dst_cache);
Is it safe to destroy the cache at this time? In the same function, we
use rcu to free the peer, but AFAICT the dst_cache will be freed
immediately:
void dst_cache_destroy(struct dst_cache *dst_cache)
{
[...]
free_percpu(dst_cache->cache);
}
(probably no real issue because ovpn_udp_send_skb gets called while we
hold a reference to the peer?)
> + netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
> + kfree_rcu(peer, rcu);
> +}
[...]
> +static int ovpn_peer_del_p2p(struct ovpn_peer *peer,
> + enum ovpn_del_peer_reason reason)
> + __must_hold(&peer->ovpn->lock)
> +{
> + struct ovpn_peer *tmp;
> +
> + tmp = rcu_dereference_protected(peer->ovpn->peer,
> + lockdep_is_held(&peer->ovpn->lock));
> + if (tmp != peer) {
> + DEBUG_NET_WARN_ON_ONCE(1);
> + if (tmp)
> + ovpn_peer_put(tmp);
Does peer->ovpn->peer need to be set to NULL here as well? Or is it
going to survive this _put?
> +
> + return -ENOENT;
> + }
> +
> + tmp->delete_reason = reason;
> + RCU_INIT_POINTER(peer->ovpn->peer, NULL);
> + ovpn_peer_put(tmp);
> +
> + return 0;
> +}
--
Sabrina
