Re: [Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash

2023-03-22 Thread Antonio Quartulli 

Hi,

On 22/03/2023 08:14, Gert Doering wrote:

Hi,

On Wed, Mar 22, 2023 at 12:10:03AM +0100, Antonio Quartulli wrote:

+struct multi_instance *mi = m->instances[peer_id];
+if (!mi)
  {


This (and undoubtedly the same code in dco_linux.c) is trusting the
kernel to never return peer_id values that are outside the array
boundaries.


very good catch.



Is this what we want?


no. we should not trust an external source.

Will send v2 for this and v3 for dco-linux



I'd strongly prefer to have a check like this here

 if ((peer_id < m->max_clients) && (m->instances[peer_id]))
 {
...
 }

(which is what we do in multi_process_incoming_dco(), for example)


Note: in p2p mode, peer-id is something random, usually much bigger
than max_clients - now this *should* never be called in p2p mode, but
I still do not have a good feeling without the bounds check.

gert


--
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash

2023-03-22 Thread Gert Doering 
Hi,

On Wed, Mar 22, 2023 at 12:10:03AM +0100, Antonio Quartulli wrote:
> +struct multi_instance *mi = m->instances[peer_id];
> +if (!mi)
>  {

This (and undoubtedly the same code in dco_linux.c) is trusting the
kernel to never return peer_id values that are outside the array 
boundaries.

Is this what we want?

I'd strongly prefer to have a check like this here

if ((peer_id < m->max_clients) && (m->instances[peer_id]))
{
...
}

(which is what we do in multi_process_incoming_dco(), for example)


Note: in p2p mode, peer-id is something random, usually much bigger
than max_clients - now this *should* never be called in p2p mode, but
I still do not have a good feeling without the bounds check.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash

2023-03-21 Thread Arne Schwabe 

Am 22.03.23 um 00:10 schrieb Antonio Quartulli:

When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).

Adapt the dco-freebsd code to do so.



Acked-By: Arne Schwabe 

Arne



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash

2023-03-21 Thread Antonio Quartulli 

Hi,

On 22/03/2023 00:10, Antonio Quartulli wrote:

When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).

Adapt the dco-freebsd code to do so.

Cc: Kristof Provost 


If the patch is suitable for merging, please change the above to 
k...@freebsd.org before moving on.


Cheers,


Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli 
---
NOTE: not tested because I have no FreeBSD environment and I
can't find how to kick off the buildbot
---
  src/openvpn/dco_freebsd.c | 22 +-
  1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 225b3cf8..ae8c1380 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -674,27 +674,15 @@ dco_event_set(dco_context_t *dco, struct event_set *es, 
void *arg)
  static void
  dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t 
*nvl)
  {
-struct hash_element *he;
-struct hash_iterator hi;
-
-hash_iterator_init(m->hash, );
-
-while ((he = hash_iterator_next()))
+struct multi_instance *mi = m->instances[peer_id];
+if (!mi)
  {
-struct multi_instance *mi = (struct multi_instance *) he->value;
-
-if (mi->context.c2.tls_multi->peer_id != peerid)
-{
-continue;
-}
-
-mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
-mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
-
+msg(M_INFO, "Peer %d returned by kernel, but not found locally", 
peerid);
  return;
  }
  
-msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);

+mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
+mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
  }
  
  int


--
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash

2023-03-21 Thread Antonio Quartulli 
When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).

Adapt the dco-freebsd code to do so.

Cc: Kristof Provost 
Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli 
---
NOTE: not tested because I have no FreeBSD environment and I
can't find how to kick off the buildbot
---
 src/openvpn/dco_freebsd.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 225b3cf8..ae8c1380 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -674,27 +674,15 @@ dco_event_set(dco_context_t *dco, struct event_set *es, 
void *arg)
 static void
 dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t 
*nvl)
 {
-struct hash_element *he;
-struct hash_iterator hi;
-
-hash_iterator_init(m->hash, );
-
-while ((he = hash_iterator_next()))
+struct multi_instance *mi = m->instances[peer_id];
+if (!mi)
 {
-struct multi_instance *mi = (struct multi_instance *) he->value;
-
-if (mi->context.c2.tls_multi->peer_id != peerid)
-{
-continue;
-}
-
-mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
-mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
-
+msg(M_INFO, "Peer %d returned by kernel, but not found locally", 
peerid);
 return;
 }
 
-msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
+mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
+mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
 }
 
 int
-- 
2.39.2



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel