| Message ID | 20230321231003.3582-1-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] dco_freebsd: use m->instances[] instead of m->hash | expand |
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
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
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
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 --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
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(-)