diff mbox series

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

Message ID 20230321231003.3582-1-a@unstable.cc
State Changes Requested
Headers
Series [Openvpn-devel] dco_freebsd: use m->instances[] instead of m->hash |

Commit Message

Antonio Quartulli March 21, 2023, 11:10 p.m. UTC 
  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 <kp@netgate.com>
Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
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(-)

Comments

Antonio Quartulli March 21, 2023, 11:35 p.m. UTC | #1 
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 <kp@netgate.com>

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

Cheers,

> Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
> 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, &hi);
> -
> -    while ((he = hash_iterator_next(&hi)))
> +    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
Arne Schwabe March 21, 2023, 11:58 p.m. UTC | #2 
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@rfc2549.org>

Arne
Gert Doering March 22, 2023, 7:14 a.m. UTC | #3 
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
Antonio Quartulli March 22, 2023, 8:35 a.m. UTC | #4 
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
diff mbox series

Patch

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, &hi);
-
-    while ((he = hash_iterator_next(&hi)))
+    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