You can subscribe to this list here.
2002 |
Jan
|
Feb
|
Mar
|
Apr
(24) |
May
(14) |
Jun
(29) |
Jul
(33) |
Aug
(3) |
Sep
(8) |
Oct
(18) |
Nov
(1) |
Dec
(10) |
---|---|---|---|---|---|---|---|---|---|---|---|---|
2003 |
Jan
(3) |
Feb
(33) |
Mar
(7) |
Apr
(28) |
May
(30) |
Jun
(5) |
Jul
(10) |
Aug
(7) |
Sep
(32) |
Oct
(41) |
Nov
(20) |
Dec
(10) |
2004 |
Jan
(24) |
Feb
(18) |
Mar
(57) |
Apr
(40) |
May
(55) |
Jun
(48) |
Jul
(77) |
Aug
(15) |
Sep
(56) |
Oct
(80) |
Nov
(74) |
Dec
(52) |
2005 |
Jan
(38) |
Feb
(42) |
Mar
(39) |
Apr
(56) |
May
(79) |
Jun
(73) |
Jul
(16) |
Aug
(23) |
Sep
(68) |
Oct
(77) |
Nov
(52) |
Dec
(27) |
2006 |
Jan
(27) |
Feb
(18) |
Mar
(51) |
Apr
(62) |
May
(28) |
Jun
(50) |
Jul
(36) |
Aug
(33) |
Sep
(47) |
Oct
(50) |
Nov
(77) |
Dec
(13) |
2007 |
Jan
(15) |
Feb
(8) |
Mar
(14) |
Apr
(18) |
May
(25) |
Jun
(16) |
Jul
(16) |
Aug
(19) |
Sep
(32) |
Oct
(17) |
Nov
(5) |
Dec
(5) |
2008 |
Jan
(64) |
Feb
(25) |
Mar
(25) |
Apr
(6) |
May
(28) |
Jun
(20) |
Jul
(10) |
Aug
(27) |
Sep
(28) |
Oct
(59) |
Nov
(37) |
Dec
(43) |
2009 |
Jan
(40) |
Feb
(25) |
Mar
(12) |
Apr
(57) |
May
(46) |
Jun
(29) |
Jul
(39) |
Aug
(10) |
Sep
(20) |
Oct
(42) |
Nov
(50) |
Dec
(57) |
2010 |
Jan
(82) |
Feb
(165) |
Mar
(256) |
Apr
(260) |
May
(36) |
Jun
(87) |
Jul
(53) |
Aug
(89) |
Sep
(107) |
Oct
(51) |
Nov
(88) |
Dec
(117) |
2011 |
Jan
(69) |
Feb
(60) |
Mar
(113) |
Apr
(71) |
May
(67) |
Jun
(90) |
Jul
(88) |
Aug
(90) |
Sep
(48) |
Oct
(64) |
Nov
(69) |
Dec
(118) |
2012 |
Jan
(49) |
Feb
(528) |
Mar
(351) |
Apr
(190) |
May
(238) |
Jun
(193) |
Jul
(104) |
Aug
(100) |
Sep
(57) |
Oct
(41) |
Nov
(47) |
Dec
(51) |
2013 |
Jan
(94) |
Feb
(57) |
Mar
(96) |
Apr
(105) |
May
(77) |
Jun
(102) |
Jul
(27) |
Aug
(81) |
Sep
(32) |
Oct
(53) |
Nov
(127) |
Dec
(65) |
2014 |
Jan
(113) |
Feb
(59) |
Mar
(104) |
Apr
(259) |
May
(70) |
Jun
(70) |
Jul
(146) |
Aug
(45) |
Sep
(58) |
Oct
(149) |
Nov
(77) |
Dec
(83) |
2015 |
Jan
(53) |
Feb
(66) |
Mar
(86) |
Apr
(50) |
May
(135) |
Jun
(76) |
Jul
(151) |
Aug
(83) |
Sep
(97) |
Oct
(262) |
Nov
(245) |
Dec
(231) |
2016 |
Jan
(131) |
Feb
(233) |
Mar
(97) |
Apr
(138) |
May
(221) |
Jun
(254) |
Jul
(92) |
Aug
(248) |
Sep
(168) |
Oct
(275) |
Nov
(477) |
Dec
(445) |
2017 |
Jan
(218) |
Feb
(217) |
Mar
(146) |
Apr
(172) |
May
(216) |
Jun
(252) |
Jul
(164) |
Aug
(192) |
Sep
(190) |
Oct
(143) |
Nov
(255) |
Dec
(182) |
2018 |
Jan
(295) |
Feb
(164) |
Mar
(113) |
Apr
(147) |
May
(64) |
Jun
(262) |
Jul
(184) |
Aug
(90) |
Sep
(69) |
Oct
(364) |
Nov
(102) |
Dec
(101) |
2019 |
Jan
(119) |
Feb
(64) |
Mar
(64) |
Apr
(102) |
May
(57) |
Jun
(154) |
Jul
(84) |
Aug
(81) |
Sep
(76) |
Oct
(102) |
Nov
(233) |
Dec
(89) |
2020 |
Jan
(38) |
Feb
(170) |
Mar
(155) |
Apr
(172) |
May
(120) |
Jun
(223) |
Jul
(461) |
Aug
(227) |
Sep
(268) |
Oct
(113) |
Nov
(56) |
Dec
(124) |
2021 |
Jan
(121) |
Feb
(48) |
Mar
(334) |
Apr
(345) |
May
(207) |
Jun
(136) |
Jul
(71) |
Aug
(112) |
Sep
(122) |
Oct
(173) |
Nov
(184) |
Dec
(223) |
2022 |
Jan
(197) |
Feb
(206) |
Mar
(156) |
Apr
(212) |
May
(192) |
Jun
(170) |
Jul
(143) |
Aug
(380) |
Sep
(182) |
Oct
(148) |
Nov
(128) |
Dec
(269) |
2023 |
Jan
(248) |
Feb
(196) |
Mar
(264) |
Apr
(36) |
May
(123) |
Jun
(66) |
Jul
(120) |
Aug
(48) |
Sep
(157) |
Oct
(198) |
Nov
(300) |
Dec
(273) |
2024 |
Jan
(271) |
Feb
(147) |
Mar
(207) |
Apr
(78) |
May
(107) |
Jun
(168) |
Jul
(151) |
Aug
(51) |
Sep
(438) |
Oct
(221) |
Nov
(302) |
Dec
(357) |
2025 |
Jan
(451) |
Feb
(219) |
Mar
(326) |
Apr
(232) |
May
(306) |
Jun
(181) |
Jul
(339) |
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
From: ordex (C. Review) <ge...@op...> - 2025-07-26 00:08:47
|
Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1115?usp=email to look at the new patch set (#2). Change subject: multi: make some multi_*() functions static ...................................................................... multi: make some multi_*() functions static multi_process_float() and multi_print_status() are both invoked only within multi.c, which is where they is defined. For this reason we can make them static and drop their declaration from multi.h. Change-Id: Id5e06f0822a3e7e4ad1b6f93caaefdb6a8cfe547 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/multi.c M src/openvpn/multi.h 2 files changed, 10 insertions(+), 15 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/15/1115/2 diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index c5691ff..c90ed5b 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -849,7 +849,7 @@ * If status file is defined, write to file. * If status file is NULL, write to syslog. */ -void +static void multi_print_status(struct multi_context *m, struct status_output *so, const int version) { if (m->hash) @@ -3210,7 +3210,15 @@ return ret; } -void +/** + * Handles peer floating. + * + * If peer is floated to a taken address, either drops packet + * (if peer that owns address has different CN) or disconnects + * existing peer. Updates multi_instance with new address, + * updates hashtables in multi_context. + */ +static void multi_process_float(struct multi_context *m, struct multi_instance *mi, struct link_socket *sock) { diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h index 8b2704c..3c821d7 100644 --- a/src/openvpn/multi.h +++ b/src/openvpn/multi.h @@ -270,17 +270,6 @@ bool multi_process_timeout(struct multi_context *m, const unsigned int mpp_flags); -/** - * Handles peer floating. - * - * If peer is floated to a taken address, either drops packet - * (if peer that owns address has different CN) or disconnects - * existing peer. Updates multi_instance with new address, - * updates hashtables in multi_context. - */ -void multi_process_float(struct multi_context *m, struct multi_instance *mi, - struct link_socket *sock); - #define MPP_PRE_SELECT (1<<0) #define MPP_CONDITIONAL_PRE_SELECT (1<<1) #define MPP_CLOSE_ON_SIGNAL (1<<2) @@ -370,8 +359,6 @@ void multi_process_drop_outgoing_tun(struct multi_context *m, const unsigned int mpp_flags); -void multi_print_status(struct multi_context *m, struct status_output *so, const int version); - struct multi_instance *multi_get_queue(struct mbuf_set *ms); void multi_add_mbuf(struct multi_context *m, -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1115?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id5e06f0822a3e7e4ad1b6f93caaefdb6a8cfe547 Gerrit-Change-Number: 1115 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset |
From: ordex (C. Review) <ge...@op...> - 2025-07-26 00:01:34
|
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email to review the following change. Change subject: dco: drop client prefix after DCO PEER_FLOAT notification ...................................................................... dco: drop client prefix after DCO PEER_FLOAT notification multi_process_float() will possibly float a client and also generate and set its new prefix. However, after processing the PEER_FLOAT_NTF message, we were not clearing the prefix, thus effectivly making the generated prefix permanent until the next set_prefix() call. Clear the prefix right after calling multi_process_float() to avoid printing messages with the wrong prefix. Github: closes OpenVPN/openvpn#799 Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/multi.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1116/1 diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index d23aee7..32331d7 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3417,6 +3417,10 @@ &m->top.c2.from.dest, (struct sockaddr *)&dco->dco_float_peer_ss); multi_process_float(m, mi, mi->context.c2.link_sockets[0]); + /* multi_process_float() generated and set a new peer prefix, but we + * don't to keep it at this point. + */ + clear_prefix(); CLEAR(dco->dco_float_peer_ss); } #endif /* if defined(TARGET_LINUX) || defined(TARGET_WIN32) */ -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Gerrit-Change-Number: 1116 Gerrit-PatchSet: 1 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: ordex (C. Review) <ge...@op...> - 2025-07-25 23:51:16
|
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1115?usp=email to review the following change. Change subject: multi: make multi_process_float() static ...................................................................... multi: make multi_process_float() static multi_process_float() is invoked only within multi.c, which is where it is defined. For this reason we can make it static and drop its declaration in multi.h. Change-Id: Id5e06f0822a3e7e4ad1b6f93caaefdb6a8cfe547 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/multi.c M src/openvpn/multi.h 2 files changed, 9 insertions(+), 12 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/15/1115/1 diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index c5691ff..d23aee7 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3210,7 +3210,15 @@ return ret; } -void +/** + * Handles peer floating. + * + * If peer is floated to a taken address, either drops packet + * (if peer that owns address has different CN) or disconnects + * existing peer. Updates multi_instance with new address, + * updates hashtables in multi_context. + */ +static void multi_process_float(struct multi_context *m, struct multi_instance *mi, struct link_socket *sock) { diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h index 8b2704c..eb5bf86 100644 --- a/src/openvpn/multi.h +++ b/src/openvpn/multi.h @@ -270,17 +270,6 @@ bool multi_process_timeout(struct multi_context *m, const unsigned int mpp_flags); -/** - * Handles peer floating. - * - * If peer is floated to a taken address, either drops packet - * (if peer that owns address has different CN) or disconnects - * existing peer. Updates multi_instance with new address, - * updates hashtables in multi_context. - */ -void multi_process_float(struct multi_context *m, struct multi_instance *mi, - struct link_socket *sock); - #define MPP_PRE_SELECT (1<<0) #define MPP_CONDITIONAL_PRE_SELECT (1<<1) #define MPP_CLOSE_ON_SIGNAL (1<<2) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1115?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id5e06f0822a3e7e4ad1b6f93caaefdb6a8cfe547 Gerrit-Change-Number: 1115 Gerrit-PatchSet: 1 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: ordex (C. Review) <ge...@op...> - 2025-07-25 23:41:56
|
Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1114?usp=email to look at the new patch set (#3). Change subject: dco_linux: clean up PEER_GET trigger and parser ...................................................................... dco_linux: clean up PEER_GET trigger and parser This patch is intended to reduce code duplication and cleanup the DCO code around the PEER_GET command. Specifically it: * unified PEER_GET reply parser for `multi` and `non-multi` case * unified PEER_GET request trigger for `multi` and `non-multi` case * dropped struct multi_context from the argument list of dco_get_peer_stats_multi() Github: closes OpenVPN/openvpn#800 Change-Id: Icbc70225d53ca678b8c22ed437b424c16e199d66 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/dco.h M src/openvpn/dco_freebsd.c M src/openvpn/dco_freebsd.h M src/openvpn/dco_linux.c M src/openvpn/dco_win.c M src/openvpn/multi.c 6 files changed, 53 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/14/1114/3 diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 9078417..2ce0eb1 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -230,11 +230,9 @@ * Update traffic statistics for all peers * * @param dco DCO device context - * @param m the server context * @param raise_sigusr1_on_err whether to raise SIGUSR1 on error **/ -int dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err); +int dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err); /** * Update traffic statistics for single peer @@ -374,8 +372,7 @@ } static inline int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { return 0; } diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 98d8fb5..78ee9a1 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -167,6 +167,8 @@ bool ovpn_dco_init(struct context *c) { + c->c1.tuntap->dco.c = c; + if (open_fd(&c->c1.tuntap->dco) < 0) { msg(M_ERR, "Failed to open socket"); @@ -713,8 +715,7 @@ } int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { struct ifdrv drv; @@ -774,7 +775,7 @@ const nvlist_t *peer = nvpeers[i]; uint32_t peerid = nvlist_get_number(peer, "peerid"); - dco_update_peer_stat(m, peerid, nvlist_get_nvlist(peer, "bytes")); + dco_update_peer_stat(dco->c->multi, peerid, nvlist_get_nvlist(peer, "bytes")); } nvlist_destroy(nvl); diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index e1a054e..e926af5 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -57,6 +57,8 @@ int dco_del_peer_reason; uint64_t dco_read_bytes; uint64_t dco_write_bytes; + + struct context *c; } dco_context_t; #endif /* defined(ENABLE_DCO) && defined(TARGET_FREEBSD) */ diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 728fb7e..9ad3d51 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -877,53 +877,8 @@ } static int -ovpn_handle_peer_multi(dco_context_t *dco, struct nlattr *attrs[]) -{ - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - - /* this function assumes openvpn is running in multipeer mode as - * it accesses c->multi - */ - if (dco->ifmode != OVPN_MODE_MP) - { - msg(M_WARN, "%s: can't parse 'multi-peer' message on P2P instance", __func__); - return NL_SKIP; - } - - if (!attrs[OVPN_A_PEER]) - { - return NL_SKIP; - } - - struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); - - if (!tb_peer[OVPN_A_PEER_ID]) - { - msg(M_WARN, "ovpn-dco: no peer-id provided in (MULTI) PEER_GET reply"); - return NL_SKIP; - } - - struct multi_context *m = dco->c->multi; - uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); - - if (peer_id >= m->max_clients || !m->instances[peer_id]) - { - msg(M_WARN, "%s: cannot store DCO stats for peer %u", __func__, - peer_id); - return NL_SKIP; - } - - dco_update_peer_stat(&m->instances[peer_id]->context.c2, tb_peer, peer_id); - - return NL_OK; -} - -static int ovpn_handle_peer(dco_context_t *dco, struct nlattr *attrs[]) { - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - if (!attrs[OVPN_A_PEER]) { msg(D_DCO_DEBUG, "%s: malformed reply", __func__); @@ -942,12 +897,25 @@ uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); struct context_2 *c2; + msg(D_DCO_DEBUG, "%s: parsing message for peer %u...", __func__, peer_id); + if (dco->ifmode == OVPN_MODE_P2P) { c2 = &dco->c->c2; + if (c2->tls_multi->dco_peer_id != peer_id) + { + return NL_SKIP; + } } else { + if (peer_id >= dco->c->multi->max_clients) + { + msg(M_WARN, "%s: received out of bound peer_id %u (max=%u)", __func__, peer_id, + dco->c->multi->max_clients); + return NL_SKIP; + } + struct multi_instance *mi = dco->c->multi->instances[peer_id]; if (!mi) { @@ -958,14 +926,6 @@ c2 = &mi->context.c2; } - /* at this point this check should never fail for MP mode, - * but it's still fully valid for P2P mode - */ - if (c2->tls_multi->dco_peer_id != peer_id) - { - return NL_SKIP; - } - dco_update_peer_stat(c2, tb_peer, peer_id); return NL_OK; @@ -1176,17 +1136,7 @@ { case OVPN_CMD_PEER_GET: { - /* this message is part of a peer list dump, hence triggered - * by a MP/server instance - */ - if (nlh->nlmsg_flags & NLM_F_MULTI) - { - return ovpn_handle_peer_multi(dco, attrs); - } - else - { - return ovpn_handle_peer(dco, attrs); - } + return ovpn_handle_peer(dco, attrs); } case OVPN_CMD_PEER_DEL_NTF: @@ -1221,52 +1171,32 @@ return ovpn_nl_recvmsgs(dco, __func__); } -int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +static int +dco_get_peer(dco_context_t *dco, int peer_id, const bool raise_sigusr1_on_err) { - msg(D_DCO_DEBUG, "%s", __func__); - - struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET); - - nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; - - int ret = ovpn_nl_msg_send(dco, nl_msg, __func__); - - nlmsg_free(nl_msg); - - if (raise_sigusr1_on_err && ret < 0) - { - msg(M_WARN, "Error retrieving DCO peer stats: the underlying DCO peer" - "may have been deleted from the kernel without notifying " - "userspace. Restarting the session"); - register_signal(m->top.sig, SIGUSR1, "dco peer stats error"); - } - return ret; -} - -int -dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err) -{ - int peer_id = c->c2.tls_multi->dco_peer_id; - if (peer_id == -1) + /* peer_id == -1 means "dump all peers", but this is allowed in MP mode only. + * If it happens in P2P mode it means that the DCO peer was deleted and we + * can simply bail out + */ + if (peer_id == -1 && dco->ifmode == OVPN_MODE_P2P) { return 0; } msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id); - if (!c->c1.tuntap) - { - return 0; - } - - dco_context_t *dco = &c->c1.tuntap->dco; struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET); struct nlattr *attr = nla_nest_start(nl_msg, OVPN_A_PEER); int ret = -EMSGSIZE; - NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); + if (peer_id != -1) + { + NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); + } + else + { + nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; + } nla_nest_end(nl_msg, attr); ret = ovpn_nl_msg_send(dco, nl_msg, __func__); @@ -1279,11 +1209,23 @@ msg(M_WARN, "Error retrieving DCO peer stats: the underlying DCO peer" "may have been deleted from the kernel without notifying " "userspace. Restarting the session"); - register_signal(c->sig, SIGUSR1, "dco peer stats error"); + register_signal(dco->c->sig, SIGUSR1, "dco peer stats error"); } return ret; } +int +dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err) +{ + return dco_get_peer(&c->c1.tuntap->dco, c->c2.tls_multi->dco_peer_id, raise_sigusr1_on_err); +} + +int +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) +{ + return dco_get_peer(dco, -1, raise_sigusr1_on_err); +} + bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index e5a33a0..995b121 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -715,8 +715,7 @@ } int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { /* Not implemented. */ return 0; diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index a62c57a..c5691ff 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -551,7 +551,7 @@ { if (dco_enabled(&m->top.options)) { - if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m, false) < 0) + if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, false) < 0) { return; } @@ -862,7 +862,7 @@ if (dco_enabled(&m->top.options)) { - if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m, true) < 0) + if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, true) < 0) { return; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1114?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icbc70225d53ca678b8c22ed437b424c16e199d66 Gerrit-Change-Number: 1114 Gerrit-PatchSet: 3 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset |
From: ordex (C. Review) <ge...@op...> - 2025-07-25 23:31:11
|
Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1114?usp=email to look at the new patch set (#2). Change subject: dco_linux: clean up PEER_GET trigger and parser ...................................................................... dco_linux: clean up PEER_GET trigger and parser This patch is intended to reduce code duplication and cleanup the DCO code around the PEER_GET command. Specifically it: * unified PEER_GET reply parser for `multi` and `non-multi` case * unified PEER_GET request trigger for `multi` and `non-multi` case * dropped struct multi_context from the argument list of dco_get_peer_stats_multi() Github: closes OpenVPN/openvpn#800 Change-Id: Icbc70225d53ca678b8c22ed437b424c16e199d66 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/dco.h M src/openvpn/dco_freebsd.c M src/openvpn/dco_linux.c M src/openvpn/dco_win.c M src/openvpn/multi.c 5 files changed, 49 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/14/1114/2 diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 9078417..2ce0eb1 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -230,11 +230,9 @@ * Update traffic statistics for all peers * * @param dco DCO device context - * @param m the server context * @param raise_sigusr1_on_err whether to raise SIGUSR1 on error **/ -int dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err); +int dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err); /** * Update traffic statistics for single peer @@ -374,8 +372,7 @@ } static inline int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { return 0; } diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 98d8fb5..e55e58d 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -713,8 +713,7 @@ } int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { struct ifdrv drv; @@ -774,7 +773,7 @@ const nvlist_t *peer = nvpeers[i]; uint32_t peerid = nvlist_get_number(peer, "peerid"); - dco_update_peer_stat(m, peerid, nvlist_get_nvlist(peer, "bytes")); + dco_update_peer_stat(dco->c->multi, peerid, nvlist_get_nvlist(peer, "bytes")); } nvlist_destroy(nvl); diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 728fb7e..9ad3d51 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -877,53 +877,8 @@ } static int -ovpn_handle_peer_multi(dco_context_t *dco, struct nlattr *attrs[]) -{ - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - - /* this function assumes openvpn is running in multipeer mode as - * it accesses c->multi - */ - if (dco->ifmode != OVPN_MODE_MP) - { - msg(M_WARN, "%s: can't parse 'multi-peer' message on P2P instance", __func__); - return NL_SKIP; - } - - if (!attrs[OVPN_A_PEER]) - { - return NL_SKIP; - } - - struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); - - if (!tb_peer[OVPN_A_PEER_ID]) - { - msg(M_WARN, "ovpn-dco: no peer-id provided in (MULTI) PEER_GET reply"); - return NL_SKIP; - } - - struct multi_context *m = dco->c->multi; - uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); - - if (peer_id >= m->max_clients || !m->instances[peer_id]) - { - msg(M_WARN, "%s: cannot store DCO stats for peer %u", __func__, - peer_id); - return NL_SKIP; - } - - dco_update_peer_stat(&m->instances[peer_id]->context.c2, tb_peer, peer_id); - - return NL_OK; -} - -static int ovpn_handle_peer(dco_context_t *dco, struct nlattr *attrs[]) { - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - if (!attrs[OVPN_A_PEER]) { msg(D_DCO_DEBUG, "%s: malformed reply", __func__); @@ -942,12 +897,25 @@ uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); struct context_2 *c2; + msg(D_DCO_DEBUG, "%s: parsing message for peer %u...", __func__, peer_id); + if (dco->ifmode == OVPN_MODE_P2P) { c2 = &dco->c->c2; + if (c2->tls_multi->dco_peer_id != peer_id) + { + return NL_SKIP; + } } else { + if (peer_id >= dco->c->multi->max_clients) + { + msg(M_WARN, "%s: received out of bound peer_id %u (max=%u)", __func__, peer_id, + dco->c->multi->max_clients); + return NL_SKIP; + } + struct multi_instance *mi = dco->c->multi->instances[peer_id]; if (!mi) { @@ -958,14 +926,6 @@ c2 = &mi->context.c2; } - /* at this point this check should never fail for MP mode, - * but it's still fully valid for P2P mode - */ - if (c2->tls_multi->dco_peer_id != peer_id) - { - return NL_SKIP; - } - dco_update_peer_stat(c2, tb_peer, peer_id); return NL_OK; @@ -1176,17 +1136,7 @@ { case OVPN_CMD_PEER_GET: { - /* this message is part of a peer list dump, hence triggered - * by a MP/server instance - */ - if (nlh->nlmsg_flags & NLM_F_MULTI) - { - return ovpn_handle_peer_multi(dco, attrs); - } - else - { - return ovpn_handle_peer(dco, attrs); - } + return ovpn_handle_peer(dco, attrs); } case OVPN_CMD_PEER_DEL_NTF: @@ -1221,52 +1171,32 @@ return ovpn_nl_recvmsgs(dco, __func__); } -int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +static int +dco_get_peer(dco_context_t *dco, int peer_id, const bool raise_sigusr1_on_err) { - msg(D_DCO_DEBUG, "%s", __func__); - - struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET); - - nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; - - int ret = ovpn_nl_msg_send(dco, nl_msg, __func__); - - nlmsg_free(nl_msg); - - if (raise_sigusr1_on_err && ret < 0) - { - msg(M_WARN, "Error retrieving DCO peer stats: the underlying DCO peer" - "may have been deleted from the kernel without notifying " - "userspace. Restarting the session"); - register_signal(m->top.sig, SIGUSR1, "dco peer stats error"); - } - return ret; -} - -int -dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err) -{ - int peer_id = c->c2.tls_multi->dco_peer_id; - if (peer_id == -1) + /* peer_id == -1 means "dump all peers", but this is allowed in MP mode only. + * If it happens in P2P mode it means that the DCO peer was deleted and we + * can simply bail out + */ + if (peer_id == -1 && dco->ifmode == OVPN_MODE_P2P) { return 0; } msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id); - if (!c->c1.tuntap) - { - return 0; - } - - dco_context_t *dco = &c->c1.tuntap->dco; struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET); struct nlattr *attr = nla_nest_start(nl_msg, OVPN_A_PEER); int ret = -EMSGSIZE; - NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); + if (peer_id != -1) + { + NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); + } + else + { + nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; + } nla_nest_end(nl_msg, attr); ret = ovpn_nl_msg_send(dco, nl_msg, __func__); @@ -1279,11 +1209,23 @@ msg(M_WARN, "Error retrieving DCO peer stats: the underlying DCO peer" "may have been deleted from the kernel without notifying " "userspace. Restarting the session"); - register_signal(c->sig, SIGUSR1, "dco peer stats error"); + register_signal(dco->c->sig, SIGUSR1, "dco peer stats error"); } return ret; } +int +dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err) +{ + return dco_get_peer(&c->c1.tuntap->dco, c->c2.tls_multi->dco_peer_id, raise_sigusr1_on_err); +} + +int +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) +{ + return dco_get_peer(dco, -1, raise_sigusr1_on_err); +} + bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index e5a33a0..995b121 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -715,8 +715,7 @@ } int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { /* Not implemented. */ return 0; diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index a62c57a..c5691ff 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -551,7 +551,7 @@ { if (dco_enabled(&m->top.options)) { - if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m, false) < 0) + if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, false) < 0) { return; } @@ -862,7 +862,7 @@ if (dco_enabled(&m->top.options)) { - if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m, true) < 0) + if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, true) < 0) { return; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1114?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icbc70225d53ca678b8c22ed437b424c16e199d66 Gerrit-Change-Number: 1114 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset |
From: ordex (C. Review) <ge...@op...> - 2025-07-25 23:29:53
|
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1114?usp=email to review the following change. Change subject: dco_linux: clean up PEER_GET trigger and parser ...................................................................... dco_linux: clean up PEER_GET trigger and parser This patch is intended to reduce code duplication and cleanup the DCO code around the PEER_GET command. Specifically it: * unified PEER_GET reply parser for `multi` and `non-multi` case * unified PEER_GET request trigger for `multi` and `non-multi` case * dropped struct multi_context from the argument list of dco_get_peer_stats_multi() Change-Id: Icbc70225d53ca678b8c22ed437b424c16e199d66 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/dco.h M src/openvpn/dco_freebsd.c M src/openvpn/dco_linux.c M src/openvpn/dco_win.c M src/openvpn/multi.c 5 files changed, 49 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/14/1114/1 diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 9078417..2ce0eb1 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -230,11 +230,9 @@ * Update traffic statistics for all peers * * @param dco DCO device context - * @param m the server context * @param raise_sigusr1_on_err whether to raise SIGUSR1 on error **/ -int dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err); +int dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err); /** * Update traffic statistics for single peer @@ -374,8 +372,7 @@ } static inline int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { return 0; } diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 98d8fb5..e55e58d 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -713,8 +713,7 @@ } int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { struct ifdrv drv; @@ -774,7 +773,7 @@ const nvlist_t *peer = nvpeers[i]; uint32_t peerid = nvlist_get_number(peer, "peerid"); - dco_update_peer_stat(m, peerid, nvlist_get_nvlist(peer, "bytes")); + dco_update_peer_stat(dco->c->multi, peerid, nvlist_get_nvlist(peer, "bytes")); } nvlist_destroy(nvl); diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 728fb7e..9ad3d51 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -877,53 +877,8 @@ } static int -ovpn_handle_peer_multi(dco_context_t *dco, struct nlattr *attrs[]) -{ - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - - /* this function assumes openvpn is running in multipeer mode as - * it accesses c->multi - */ - if (dco->ifmode != OVPN_MODE_MP) - { - msg(M_WARN, "%s: can't parse 'multi-peer' message on P2P instance", __func__); - return NL_SKIP; - } - - if (!attrs[OVPN_A_PEER]) - { - return NL_SKIP; - } - - struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); - - if (!tb_peer[OVPN_A_PEER_ID]) - { - msg(M_WARN, "ovpn-dco: no peer-id provided in (MULTI) PEER_GET reply"); - return NL_SKIP; - } - - struct multi_context *m = dco->c->multi; - uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); - - if (peer_id >= m->max_clients || !m->instances[peer_id]) - { - msg(M_WARN, "%s: cannot store DCO stats for peer %u", __func__, - peer_id); - return NL_SKIP; - } - - dco_update_peer_stat(&m->instances[peer_id]->context.c2, tb_peer, peer_id); - - return NL_OK; -} - -static int ovpn_handle_peer(dco_context_t *dco, struct nlattr *attrs[]) { - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - if (!attrs[OVPN_A_PEER]) { msg(D_DCO_DEBUG, "%s: malformed reply", __func__); @@ -942,12 +897,25 @@ uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); struct context_2 *c2; + msg(D_DCO_DEBUG, "%s: parsing message for peer %u...", __func__, peer_id); + if (dco->ifmode == OVPN_MODE_P2P) { c2 = &dco->c->c2; + if (c2->tls_multi->dco_peer_id != peer_id) + { + return NL_SKIP; + } } else { + if (peer_id >= dco->c->multi->max_clients) + { + msg(M_WARN, "%s: received out of bound peer_id %u (max=%u)", __func__, peer_id, + dco->c->multi->max_clients); + return NL_SKIP; + } + struct multi_instance *mi = dco->c->multi->instances[peer_id]; if (!mi) { @@ -958,14 +926,6 @@ c2 = &mi->context.c2; } - /* at this point this check should never fail for MP mode, - * but it's still fully valid for P2P mode - */ - if (c2->tls_multi->dco_peer_id != peer_id) - { - return NL_SKIP; - } - dco_update_peer_stat(c2, tb_peer, peer_id); return NL_OK; @@ -1176,17 +1136,7 @@ { case OVPN_CMD_PEER_GET: { - /* this message is part of a peer list dump, hence triggered - * by a MP/server instance - */ - if (nlh->nlmsg_flags & NLM_F_MULTI) - { - return ovpn_handle_peer_multi(dco, attrs); - } - else - { - return ovpn_handle_peer(dco, attrs); - } + return ovpn_handle_peer(dco, attrs); } case OVPN_CMD_PEER_DEL_NTF: @@ -1221,52 +1171,32 @@ return ovpn_nl_recvmsgs(dco, __func__); } -int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +static int +dco_get_peer(dco_context_t *dco, int peer_id, const bool raise_sigusr1_on_err) { - msg(D_DCO_DEBUG, "%s", __func__); - - struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET); - - nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; - - int ret = ovpn_nl_msg_send(dco, nl_msg, __func__); - - nlmsg_free(nl_msg); - - if (raise_sigusr1_on_err && ret < 0) - { - msg(M_WARN, "Error retrieving DCO peer stats: the underlying DCO peer" - "may have been deleted from the kernel without notifying " - "userspace. Restarting the session"); - register_signal(m->top.sig, SIGUSR1, "dco peer stats error"); - } - return ret; -} - -int -dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err) -{ - int peer_id = c->c2.tls_multi->dco_peer_id; - if (peer_id == -1) + /* peer_id == -1 means "dump all peers", but this is allowed in MP mode only. + * If it happens in P2P mode it means that the DCO peer was deleted and we + * can simply bail out + */ + if (peer_id == -1 && dco->ifmode == OVPN_MODE_P2P) { return 0; } msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id); - if (!c->c1.tuntap) - { - return 0; - } - - dco_context_t *dco = &c->c1.tuntap->dco; struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET); struct nlattr *attr = nla_nest_start(nl_msg, OVPN_A_PEER); int ret = -EMSGSIZE; - NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); + if (peer_id != -1) + { + NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); + } + else + { + nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; + } nla_nest_end(nl_msg, attr); ret = ovpn_nl_msg_send(dco, nl_msg, __func__); @@ -1279,11 +1209,23 @@ msg(M_WARN, "Error retrieving DCO peer stats: the underlying DCO peer" "may have been deleted from the kernel without notifying " "userspace. Restarting the session"); - register_signal(c->sig, SIGUSR1, "dco peer stats error"); + register_signal(dco->c->sig, SIGUSR1, "dco peer stats error"); } return ret; } +int +dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err) +{ + return dco_get_peer(&c->c1.tuntap->dco, c->c2.tls_multi->dco_peer_id, raise_sigusr1_on_err); +} + +int +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) +{ + return dco_get_peer(dco, -1, raise_sigusr1_on_err); +} + bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index e5a33a0..995b121 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -715,8 +715,7 @@ } int -dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m, - const bool raise_sigusr1_on_err) +dco_get_peer_stats_multi(dco_context_t *dco, const bool raise_sigusr1_on_err) { /* Not implemented. */ return 0; diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index a62c57a..c5691ff 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -551,7 +551,7 @@ { if (dco_enabled(&m->top.options)) { - if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m, false) < 0) + if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, false) < 0) { return; } @@ -862,7 +862,7 @@ if (dco_enabled(&m->top.options)) { - if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m, true) < 0) + if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, true) < 0) { return; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1114?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icbc70225d53ca678b8c22ed437b424c16e199d66 Gerrit-Change-Number: 1114 Gerrit-PatchSet: 1 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 19:52:32
|
cron2 has uploaded a new patch set (#3) to the change originally created by ralf_lici. ( http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: add flag to print addresses in a consistent format during float ...................................................................... add flag to print addresses in a consistent format during float Introduce the MAPF_SHOW_FAMILY flag to prepend the address family to the address when printing an mroute_addr object, similar to print_sockaddr_ex(). This ensures that when logging a float operation, both the old and new addresses are printed in the same format: $proto:[$family]$address:$port. Note: when using this flag with an IPv4-mapped IPv6 address, the output will appear as: [AF_INET6]a.b.c.d Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395 Signed-off-by: Ralf Lici <ra...@ma...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32345.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/mroute.c M src/openvpn/mroute.h M src/openvpn/multi.c 3 files changed, 11 insertions(+), 2 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1092/3 diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c index a617b33..e3b1e9b 100644 --- a/src/openvpn/mroute.c +++ b/src/openvpn/mroute.c @@ -415,6 +415,10 @@ { buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET, false)); } + if (flags & MAPF_SHOW_FAMILY) + { + buf_printf(&out, "[AF_INET]"); + } buf_printf(&out, "%s", print_in_addr_t(ntohl(maddr.v4.addr), (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc)); if (maddr.type & MR_WITH_NETBITS) @@ -442,6 +446,10 @@ { buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET6, false)); } + if (flags & MAPF_SHOW_FAMILY) + { + buf_printf(&out, "[AF_INET6]"); + } if (IN6_IS_ADDR_V4MAPPED( &maddr.v6.addr ) ) { buf_printf(&out, "%s", print_in_addr_t(maddr.v4mappedv6.addr, diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h index c359fd2..4f9fc03 100644 --- a/src/openvpn/mroute.h +++ b/src/openvpn/mroute.h @@ -150,6 +150,7 @@ #define MAPF_SUBNET (1<<0) #define MAPF_IA_EMPTY_IF_UNDEF (1<<1) #define MAPF_SHOW_ARP (1<<2) +#define MAPF_SHOW_FAMILY (1<<3) const char *mroute_addr_print_ex(const struct mroute_addr *ma, const unsigned int flags, struct gc_arena *gc); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index ec260a2..a62c57a 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3275,8 +3275,8 @@ msg(D_MULTI_MEDIUM, "peer %" PRIu32 " (%s) floated from %s to %s", mi->context.c2.tls_multi->peer_id, tls_common_name(mi->context.c2.tls_multi, false), - mroute_addr_print(&mi->real, &gc), - print_link_socket_actual(&m->top.c2.from, &gc)); + mroute_addr_print_ex(&mi->real, MAPF_SHOW_FAMILY, &gc), + mroute_addr_print_ex(&real, MAPF_SHOW_FAMILY, &gc)); /* remove old address from hash table before changing address */ ASSERT(hash_remove(m->hash, &mi->real)); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395 Gerrit-Change-Number: 1092 Gerrit-PatchSet: 3 Gerrit-Owner: ralf_lici <ra...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 19:52:32
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email ) Change subject: add flag to print addresses in a consistent format during float ...................................................................... add flag to print addresses in a consistent format during float Introduce the MAPF_SHOW_FAMILY flag to prepend the address family to the address when printing an mroute_addr object, similar to print_sockaddr_ex(). This ensures that when logging a float operation, both the old and new addresses are printed in the same format: $proto:[$family]$address:$port. Note: when using this flag with an IPv4-mapped IPv6 address, the output will appear as: [AF_INET6]a.b.c.d Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395 Signed-off-by: Ralf Lici <ra...@ma...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32345.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/mroute.c M src/openvpn/mroute.h M src/openvpn/multi.c 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c index a617b33..e3b1e9b 100644 --- a/src/openvpn/mroute.c +++ b/src/openvpn/mroute.c @@ -415,6 +415,10 @@ { buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET, false)); } + if (flags & MAPF_SHOW_FAMILY) + { + buf_printf(&out, "[AF_INET]"); + } buf_printf(&out, "%s", print_in_addr_t(ntohl(maddr.v4.addr), (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc)); if (maddr.type & MR_WITH_NETBITS) @@ -442,6 +446,10 @@ { buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET6, false)); } + if (flags & MAPF_SHOW_FAMILY) + { + buf_printf(&out, "[AF_INET6]"); + } if (IN6_IS_ADDR_V4MAPPED( &maddr.v6.addr ) ) { buf_printf(&out, "%s", print_in_addr_t(maddr.v4mappedv6.addr, diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h index c359fd2..4f9fc03 100644 --- a/src/openvpn/mroute.h +++ b/src/openvpn/mroute.h @@ -150,6 +150,7 @@ #define MAPF_SUBNET (1<<0) #define MAPF_IA_EMPTY_IF_UNDEF (1<<1) #define MAPF_SHOW_ARP (1<<2) +#define MAPF_SHOW_FAMILY (1<<3) const char *mroute_addr_print_ex(const struct mroute_addr *ma, const unsigned int flags, struct gc_arena *gc); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index ec260a2..a62c57a 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3275,8 +3275,8 @@ msg(D_MULTI_MEDIUM, "peer %" PRIu32 " (%s) floated from %s to %s", mi->context.c2.tls_multi->peer_id, tls_common_name(mi->context.c2.tls_multi, false), - mroute_addr_print(&mi->real, &gc), - print_link_socket_actual(&m->top.c2.from, &gc)); + mroute_addr_print_ex(&mi->real, MAPF_SHOW_FAMILY, &gc), + mroute_addr_print_ex(&real, MAPF_SHOW_FAMILY, &gc)); /* remove old address from hash table before changing address */ ASSERT(hash_remove(m->hash, &mi->real)); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395 Gerrit-Change-Number: 1092 Gerrit-PatchSet: 3 Gerrit-Owner: ralf_lici <ra...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
From: Gert D. <ge...@gr...> - 2025-07-25 19:52:09
|
"Because it makes sense" - as agreed, this still distinguishes between "v4 on a v6 socket" and "v4 on a v4 socket", because for troubleshooting it can make a difference. But having both old and new address printed by the same function in the same format is definitely an improvement :-) Tested v4 on a v4-only socket, v4+v6 on a dual-stack v6 socket, all looks nice and consistent now. Thanks. Your patch has been applied to the master branch. commit 7d5ec053f0f30c6cd27b60ed76859a09f6dbf5e4 Author: Ralf Lici Date: Fri Jul 25 21:41:39 2025 +0200 add flag to print addresses in a consistent format during float Signed-off-by: Ralf Lici <ra...@ma...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32345.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
From: Gert D. <ge...@gr...> - 2025-07-25 19:41:56
|
From: Ralf Lici <ra...@ma...> Introduce the MAPF_SHOW_FAMILY flag to prepend the address family to the address when printing an mroute_addr object, similar to print_sockaddr_ex(). This ensures that when logging a float operation, both the old and new addresses are printed in the same format: $proto:[$family]$address:$port. Note: when using this flag with an IPv4-mapped IPv6 address, the output will appear as: [AF_INET6]a.b.c.d Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395 Signed-off-by: Ralf Lici <ra...@ma...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1092 This mail reflects revision 2 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c index a617b33..e3b1e9b 100644 --- a/src/openvpn/mroute.c +++ b/src/openvpn/mroute.c @@ -415,6 +415,10 @@ { buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET, false)); } + if (flags & MAPF_SHOW_FAMILY) + { + buf_printf(&out, "[AF_INET]"); + } buf_printf(&out, "%s", print_in_addr_t(ntohl(maddr.v4.addr), (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc)); if (maddr.type & MR_WITH_NETBITS) @@ -442,6 +446,10 @@ { buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET6, false)); } + if (flags & MAPF_SHOW_FAMILY) + { + buf_printf(&out, "[AF_INET6]"); + } if (IN6_IS_ADDR_V4MAPPED( &maddr.v6.addr ) ) { buf_printf(&out, "%s", print_in_addr_t(maddr.v4mappedv6.addr, diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h index c359fd2..4f9fc03 100644 --- a/src/openvpn/mroute.h +++ b/src/openvpn/mroute.h @@ -150,6 +150,7 @@ #define MAPF_SUBNET (1<<0) #define MAPF_IA_EMPTY_IF_UNDEF (1<<1) #define MAPF_SHOW_ARP (1<<2) +#define MAPF_SHOW_FAMILY (1<<3) const char *mroute_addr_print_ex(const struct mroute_addr *ma, const unsigned int flags, struct gc_arena *gc); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 4696686..69ec2da 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3274,8 +3274,8 @@ msg(D_MULTI_MEDIUM, "peer %" PRIu32 " (%s) floated from %s to %s", mi->context.c2.tls_multi->peer_id, tls_common_name(mi->context.c2.tls_multi, false), - mroute_addr_print(&mi->real, &gc), - print_link_socket_actual(&m->top.c2.from, &gc)); + mroute_addr_print_ex(&mi->real, MAPF_SHOW_FAMILY, &gc), + mroute_addr_print_ex(&real, MAPF_SHOW_FAMILY, &gc)); /* remove old address from hash table before changing address */ ASSERT(hash_remove(m->hash, &mi->real)); |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 19:41:36
|
Attention is currently required from: flichtenheld, plaisthos, ralf_lici. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email ) Change subject: add flag to print addresses in a consistent format during float ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395 Gerrit-Change-Number: 1092 Gerrit-PatchSet: 2 Gerrit-Owner: ralf_lici <ra...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ralf_lici <ra...@ma...> Gerrit-Comment-Date: Fri, 25 Jul 2025 19:41:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 18:19:45
|
Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email ) Change subject: options: Clean up function setenv_foreign_option ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I'm not convinced we couldn't do something more radical, like, getting rid of the loop and `first` and all that, since all callers call `setenv_foreign_option()` with `len == 3`... (unless I misread something) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1112?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I7efc77f63734501dfa8a8f5bed17b1a1b4e9e201 Gerrit-Change-Number: 1112 Gerrit-PatchSet: 1 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 25 Jul 2025 18:19:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 18:06:40
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email ) Change subject: dco_linux: fix async message reception ...................................................................... dco_linux: fix async message reception Currently whenever we send a PEER_GET request to ovpn, we also set the CB that is supposed to parse the reply. However, due to the async nature of netlink messages, we could get an unrelated notification, sent by ovpn (kernel) upon some event, after userland has set the CB, but before parsing the awaited reply. When this happens, the notification is then parsed with the configured CB instead of the notification parser, thus effectively rejecting the notification and losing the event. To fix this inconsistency, make ovpn_handle_msg() the default and only netlink parser CB. It is configured upon DCO initialization and is never removed. ovpn_handle_msg() will check the message type and will call the corresponding handler. This way, no matter what message we get at what time, we'll always parse it correctly. As a bonus we can also simplify the nl_sendmsg() API as we don't need to pass the cb and its argument anymore. The ID of the NLCTRL family is now also stored in the DCO context as we need it to check when we receive a mcast ID lookup message. Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Github: closes OpenVPN/openvpn#793 Signed-off-by: Antonio Quartulli <an...@ma...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32339.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/dco_linux.c M src/openvpn/dco_linux.h 2 files changed, 110 insertions(+), 67 deletions(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 7c639d9..728fb7e 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -167,23 +167,19 @@ } /** - * Send a prepared netlink message and registers cb as callback if non-null. + * Send a prepared netlink message. * * The method will also free nl_msg * @param dco The dco context to use * @param nl_msg the message to use - * @param cb An optional callback if the caller expects an answer - * @param cb_arg An optional param to pass to the callback * @param prefix A prefix to report in the error message to give the user context * @return status of sending the message */ static int -ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - void *cb_arg, const char *prefix) +ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, const char *prefix) { dco->status = 1; - nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -285,7 +281,7 @@ } nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -385,6 +381,29 @@ } static void +ovpn_dco_register(dco_context_t *dco) +{ + msg(D_DCO_DEBUG, __func__); + ovpn_get_mcast_id(dco); + + if (dco->ovpn_dco_mcast_id < 0) + { + msg(M_FATAL, "cannot get mcast group: %s", nl_geterror(dco->ovpn_dco_mcast_id)); + } + + /* Register for ovpn-dco specific multicast messages that the kernel may + * send + */ + int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); + if (ret) + { + msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); + } +} + +static int ovpn_handle_msg(struct nl_msg *msg, void *arg); + +static void ovpn_dco_init_netlink(dco_context_t *dco) { dco->ovpn_dco_id = resolve_ovpn_netlink_id(M_FATAL); @@ -420,11 +439,15 @@ nl_socket_set_cb(dco->nl_sock, dco->nl_cb); + dco->dco_message_peer_id = -1; nl_cb_err(dco->nl_cb, NL_CB_CUSTOM, ovpn_nl_cb_error, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_FINISH, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_ACK, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); + nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); + + ovpn_dco_register(dco); /* The async PACKET messages confuse libnl and it will drop them with * wrong sequence numbers (NLE_SEQ_MISMATCH), so disable libnl's sequence @@ -476,27 +499,6 @@ CLEAR(dco); } -static void -ovpn_dco_register(dco_context_t *dco) -{ - msg(D_DCO_DEBUG, __func__); - ovpn_get_mcast_id(dco); - - if (dco->ovpn_dco_mcast_id < 0) - { - msg(M_FATAL, "cannot get mcast group: %s", nl_geterror(dco->ovpn_dco_mcast_id)); - } - - /* Register for ovpn-dco specific multicast messages that the kernel may - * send - */ - int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); - if (ret) - { - msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); - } -} - int open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev) { @@ -516,10 +518,6 @@ msg(M_FATAL, "DCO: cannot retrieve ifindex for interface %s", dev); } - tt->dco.dco_message_peer_id = -1; - - ovpn_dco_register(&tt->dco); - return 0; } @@ -548,7 +546,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_KEYCONF_PEER_ID, peerid); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -572,7 +570,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peerid); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -598,7 +596,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_KEYCONF_SLOT, slot); nla_nest_end(nl_msg, keyconf); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -657,7 +655,7 @@ nla_nest_end(nl_msg, key_conf); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -686,7 +684,7 @@ keepalive_timeout); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -754,7 +752,7 @@ /* Even though 'nlctrl' is a constant, there seem to be no library * provided define for it */ - int ctrlid = genl_ctrl_resolve(dco->nl_sock, "nlctrl"); + dco->ctrlid = genl_ctrl_resolve(dco->nl_sock, "nlctrl"); struct nl_msg *nl_msg = nlmsg_alloc(); if (!nl_msg) @@ -762,12 +760,12 @@ return -ENOMEM; } - genlmsg_put(nl_msg, 0, 0, ctrlid, 0, 0, CTRL_CMD_GETFAMILY, 0); + genlmsg_put(nl_msg, 0, 0, dco->ctrlid, 0, 0, CTRL_CMD_GETFAMILY, 0); int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_FAMILY_NAME); - ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -879,31 +877,34 @@ } static int -dco_parse_peer_multi(struct nl_msg *msg, void *arg) +ovpn_handle_peer_multi(dco_context_t *dco, struct nlattr *attrs[]) { - struct nlattr *tb[OVPN_A_MAX + 1]; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - nla_parse(tb, OVPN_A_MAX, genlmsg_attrdata(gnlh, 0), - genlmsg_attrlen(gnlh, 0), NULL); + /* this function assumes openvpn is running in multipeer mode as + * it accesses c->multi + */ + if (dco->ifmode != OVPN_MODE_MP) + { + msg(M_WARN, "%s: can't parse 'multi-peer' message on P2P instance", __func__); + return NL_SKIP; + } - if (!tb[OVPN_A_PEER]) + if (!attrs[OVPN_A_PEER]) { return NL_SKIP; } struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, tb[OVPN_A_PEER], NULL); + nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); if (!tb_peer[OVPN_A_PEER_ID]) { - msg(M_WARN, "%s: no peer-id provided in reply", __func__); + msg(M_WARN, "ovpn-dco: no peer-id provided in (MULTI) PEER_GET reply"); return NL_SKIP; } - struct multi_context *m = arg; + struct multi_context *m = dco->c->multi; uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); if (peer_id >= m->max_clients || !m->instances[peer_id]) @@ -919,39 +920,53 @@ } static int -dco_parse_peer(struct nl_msg *msg, void *arg) +ovpn_handle_peer(dco_context_t *dco, struct nlattr *attrs[]) { - struct context *c = arg; - struct nlattr *tb[OVPN_A_MAX + 1]; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - nla_parse(tb, OVPN_A_MAX, genlmsg_attrdata(gnlh, 0), - genlmsg_attrlen(gnlh, 0), NULL); - - if (!tb[OVPN_A_PEER]) + if (!attrs[OVPN_A_PEER]) { msg(D_DCO_DEBUG, "%s: malformed reply", __func__); return NL_SKIP; } struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, tb[OVPN_A_PEER], NULL); + nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); if (!tb_peer[OVPN_A_PEER_ID]) { - msg(M_WARN, "%s: no peer-id provided in reply", __func__); + msg(M_WARN, "ovpn-dco: no peer-id provided in PEER_GET reply"); return NL_SKIP; } uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); - if (c->c2.tls_multi->dco_peer_id != peer_id) + struct context_2 *c2; + + if (dco->ifmode == OVPN_MODE_P2P) + { + c2 = &dco->c->c2; + } + else + { + struct multi_instance *mi = dco->c->multi->instances[peer_id]; + if (!mi) + { + msg(M_WARN, "%s: received data for a non-existing peer %u", __func__, peer_id); + return NL_SKIP; + } + + c2 = &mi->context.c2; + } + + /* at this point this check should never fail for MP mode, + * but it's still fully valid for P2P mode + */ + if (c2->tls_multi->dco_peer_id != peer_id) { return NL_SKIP; } - dco_update_peer_stat(&c->c2, tb_peer, peer_id); + dco_update_peer_stat(c2, tb_peer, peer_id); return NL_OK; } @@ -1120,9 +1135,22 @@ { dco_context_t *dco = arg; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); struct nlattr *attrs[OVPN_A_MAX + 1]; struct nlmsghdr *nlh = nlmsg_hdr(msg); + struct genlmsghdr *gnlh = genlmsg_hdr(nlh); + + msg(D_DCO_DEBUG, "ovpn-dco: received netlink message type=%u cmd=%u flags=%#.4x", + nlh->nlmsg_type, gnlh->cmd, nlh->nlmsg_flags); + + /* if we get a message from the NLCTRL family, it means + * this is the reply to the mcast ID resolution request + * and we parse it accordingly. + */ + if (nlh->nlmsg_type == dco->ctrlid) + { + msg(D_DCO_DEBUG, "ovpn-dco: received CTRLID message"); + return mcast_family_handler(msg, dco); + } if (!genlmsg_valid_hdr(nlh, 0)) { @@ -1146,6 +1174,21 @@ */ switch (gnlh->cmd) { + case OVPN_CMD_PEER_GET: + { + /* this message is part of a peer list dump, hence triggered + * by a MP/server instance + */ + if (nlh->nlmsg_flags & NLM_F_MULTI) + { + return ovpn_handle_peer_multi(dco, attrs); + } + else + { + return ovpn_handle_peer(dco, attrs); + } + } + case OVPN_CMD_PEER_DEL_NTF: { return ovpn_handle_peer_del_ntf(dco, attrs); @@ -1174,7 +1217,6 @@ dco_do_read(dco_context_t *dco) { msg(D_DCO_DEBUG, __func__); - nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); return ovpn_nl_recvmsgs(dco, __func__); } @@ -1189,7 +1231,7 @@ nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; - int ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer_multi, m, __func__); + int ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nlmsg_free(nl_msg); @@ -1227,7 +1269,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer, c, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h index 5e61cf1..cc14f45 100644 --- a/src/openvpn/dco_linux.h +++ b/src/openvpn/dco_linux.h @@ -66,6 +66,7 @@ int status; struct context *c; + int ctrlid; enum ovpn_mode ifmode; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Gerrit-Change-Number: 1100 Gerrit-PatchSet: 6 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 18:06:33
|
cron2 has uploaded a new patch set (#6) to the change originally created by ordex. ( http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: dco_linux: fix async message reception ...................................................................... dco_linux: fix async message reception Currently whenever we send a PEER_GET request to ovpn, we also set the CB that is supposed to parse the reply. However, due to the async nature of netlink messages, we could get an unrelated notification, sent by ovpn (kernel) upon some event, after userland has set the CB, but before parsing the awaited reply. When this happens, the notification is then parsed with the configured CB instead of the notification parser, thus effectively rejecting the notification and losing the event. To fix this inconsistency, make ovpn_handle_msg() the default and only netlink parser CB. It is configured upon DCO initialization and is never removed. ovpn_handle_msg() will check the message type and will call the corresponding handler. This way, no matter what message we get at what time, we'll always parse it correctly. As a bonus we can also simplify the nl_sendmsg() API as we don't need to pass the cb and its argument anymore. The ID of the NLCTRL family is now also stored in the DCO context as we need it to check when we receive a mcast ID lookup message. Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Github: closes OpenVPN/openvpn#793 Signed-off-by: Antonio Quartulli <an...@ma...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32339.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/dco_linux.c M src/openvpn/dco_linux.h 2 files changed, 110 insertions(+), 67 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/00/1100/6 diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 7c639d9..728fb7e 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -167,23 +167,19 @@ } /** - * Send a prepared netlink message and registers cb as callback if non-null. + * Send a prepared netlink message. * * The method will also free nl_msg * @param dco The dco context to use * @param nl_msg the message to use - * @param cb An optional callback if the caller expects an answer - * @param cb_arg An optional param to pass to the callback * @param prefix A prefix to report in the error message to give the user context * @return status of sending the message */ static int -ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - void *cb_arg, const char *prefix) +ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, const char *prefix) { dco->status = 1; - nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -285,7 +281,7 @@ } nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -385,6 +381,29 @@ } static void +ovpn_dco_register(dco_context_t *dco) +{ + msg(D_DCO_DEBUG, __func__); + ovpn_get_mcast_id(dco); + + if (dco->ovpn_dco_mcast_id < 0) + { + msg(M_FATAL, "cannot get mcast group: %s", nl_geterror(dco->ovpn_dco_mcast_id)); + } + + /* Register for ovpn-dco specific multicast messages that the kernel may + * send + */ + int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); + if (ret) + { + msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); + } +} + +static int ovpn_handle_msg(struct nl_msg *msg, void *arg); + +static void ovpn_dco_init_netlink(dco_context_t *dco) { dco->ovpn_dco_id = resolve_ovpn_netlink_id(M_FATAL); @@ -420,11 +439,15 @@ nl_socket_set_cb(dco->nl_sock, dco->nl_cb); + dco->dco_message_peer_id = -1; nl_cb_err(dco->nl_cb, NL_CB_CUSTOM, ovpn_nl_cb_error, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_FINISH, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_ACK, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); + nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); + + ovpn_dco_register(dco); /* The async PACKET messages confuse libnl and it will drop them with * wrong sequence numbers (NLE_SEQ_MISMATCH), so disable libnl's sequence @@ -476,27 +499,6 @@ CLEAR(dco); } -static void -ovpn_dco_register(dco_context_t *dco) -{ - msg(D_DCO_DEBUG, __func__); - ovpn_get_mcast_id(dco); - - if (dco->ovpn_dco_mcast_id < 0) - { - msg(M_FATAL, "cannot get mcast group: %s", nl_geterror(dco->ovpn_dco_mcast_id)); - } - - /* Register for ovpn-dco specific multicast messages that the kernel may - * send - */ - int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); - if (ret) - { - msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); - } -} - int open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev) { @@ -516,10 +518,6 @@ msg(M_FATAL, "DCO: cannot retrieve ifindex for interface %s", dev); } - tt->dco.dco_message_peer_id = -1; - - ovpn_dco_register(&tt->dco); - return 0; } @@ -548,7 +546,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_KEYCONF_PEER_ID, peerid); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -572,7 +570,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peerid); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -598,7 +596,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_KEYCONF_SLOT, slot); nla_nest_end(nl_msg, keyconf); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -657,7 +655,7 @@ nla_nest_end(nl_msg, key_conf); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -686,7 +684,7 @@ keepalive_timeout); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -754,7 +752,7 @@ /* Even though 'nlctrl' is a constant, there seem to be no library * provided define for it */ - int ctrlid = genl_ctrl_resolve(dco->nl_sock, "nlctrl"); + dco->ctrlid = genl_ctrl_resolve(dco->nl_sock, "nlctrl"); struct nl_msg *nl_msg = nlmsg_alloc(); if (!nl_msg) @@ -762,12 +760,12 @@ return -ENOMEM; } - genlmsg_put(nl_msg, 0, 0, ctrlid, 0, 0, CTRL_CMD_GETFAMILY, 0); + genlmsg_put(nl_msg, 0, 0, dco->ctrlid, 0, 0, CTRL_CMD_GETFAMILY, 0); int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_FAMILY_NAME); - ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -879,31 +877,34 @@ } static int -dco_parse_peer_multi(struct nl_msg *msg, void *arg) +ovpn_handle_peer_multi(dco_context_t *dco, struct nlattr *attrs[]) { - struct nlattr *tb[OVPN_A_MAX + 1]; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - nla_parse(tb, OVPN_A_MAX, genlmsg_attrdata(gnlh, 0), - genlmsg_attrlen(gnlh, 0), NULL); + /* this function assumes openvpn is running in multipeer mode as + * it accesses c->multi + */ + if (dco->ifmode != OVPN_MODE_MP) + { + msg(M_WARN, "%s: can't parse 'multi-peer' message on P2P instance", __func__); + return NL_SKIP; + } - if (!tb[OVPN_A_PEER]) + if (!attrs[OVPN_A_PEER]) { return NL_SKIP; } struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, tb[OVPN_A_PEER], NULL); + nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); if (!tb_peer[OVPN_A_PEER_ID]) { - msg(M_WARN, "%s: no peer-id provided in reply", __func__); + msg(M_WARN, "ovpn-dco: no peer-id provided in (MULTI) PEER_GET reply"); return NL_SKIP; } - struct multi_context *m = arg; + struct multi_context *m = dco->c->multi; uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); if (peer_id >= m->max_clients || !m->instances[peer_id]) @@ -919,39 +920,53 @@ } static int -dco_parse_peer(struct nl_msg *msg, void *arg) +ovpn_handle_peer(dco_context_t *dco, struct nlattr *attrs[]) { - struct context *c = arg; - struct nlattr *tb[OVPN_A_MAX + 1]; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - nla_parse(tb, OVPN_A_MAX, genlmsg_attrdata(gnlh, 0), - genlmsg_attrlen(gnlh, 0), NULL); - - if (!tb[OVPN_A_PEER]) + if (!attrs[OVPN_A_PEER]) { msg(D_DCO_DEBUG, "%s: malformed reply", __func__); return NL_SKIP; } struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, tb[OVPN_A_PEER], NULL); + nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); if (!tb_peer[OVPN_A_PEER_ID]) { - msg(M_WARN, "%s: no peer-id provided in reply", __func__); + msg(M_WARN, "ovpn-dco: no peer-id provided in PEER_GET reply"); return NL_SKIP; } uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); - if (c->c2.tls_multi->dco_peer_id != peer_id) + struct context_2 *c2; + + if (dco->ifmode == OVPN_MODE_P2P) + { + c2 = &dco->c->c2; + } + else + { + struct multi_instance *mi = dco->c->multi->instances[peer_id]; + if (!mi) + { + msg(M_WARN, "%s: received data for a non-existing peer %u", __func__, peer_id); + return NL_SKIP; + } + + c2 = &mi->context.c2; + } + + /* at this point this check should never fail for MP mode, + * but it's still fully valid for P2P mode + */ + if (c2->tls_multi->dco_peer_id != peer_id) { return NL_SKIP; } - dco_update_peer_stat(&c->c2, tb_peer, peer_id); + dco_update_peer_stat(c2, tb_peer, peer_id); return NL_OK; } @@ -1120,9 +1135,22 @@ { dco_context_t *dco = arg; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); struct nlattr *attrs[OVPN_A_MAX + 1]; struct nlmsghdr *nlh = nlmsg_hdr(msg); + struct genlmsghdr *gnlh = genlmsg_hdr(nlh); + + msg(D_DCO_DEBUG, "ovpn-dco: received netlink message type=%u cmd=%u flags=%#.4x", + nlh->nlmsg_type, gnlh->cmd, nlh->nlmsg_flags); + + /* if we get a message from the NLCTRL family, it means + * this is the reply to the mcast ID resolution request + * and we parse it accordingly. + */ + if (nlh->nlmsg_type == dco->ctrlid) + { + msg(D_DCO_DEBUG, "ovpn-dco: received CTRLID message"); + return mcast_family_handler(msg, dco); + } if (!genlmsg_valid_hdr(nlh, 0)) { @@ -1146,6 +1174,21 @@ */ switch (gnlh->cmd) { + case OVPN_CMD_PEER_GET: + { + /* this message is part of a peer list dump, hence triggered + * by a MP/server instance + */ + if (nlh->nlmsg_flags & NLM_F_MULTI) + { + return ovpn_handle_peer_multi(dco, attrs); + } + else + { + return ovpn_handle_peer(dco, attrs); + } + } + case OVPN_CMD_PEER_DEL_NTF: { return ovpn_handle_peer_del_ntf(dco, attrs); @@ -1174,7 +1217,6 @@ dco_do_read(dco_context_t *dco) { msg(D_DCO_DEBUG, __func__); - nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); return ovpn_nl_recvmsgs(dco, __func__); } @@ -1189,7 +1231,7 @@ nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; - int ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer_multi, m, __func__); + int ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nlmsg_free(nl_msg); @@ -1227,7 +1269,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer, c, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h index 5e61cf1..cc14f45 100644 --- a/src/openvpn/dco_linux.h +++ b/src/openvpn/dco_linux.h @@ -66,6 +66,7 @@ int status; struct context *c; + int ctrlid; enum ovpn_mode ifmode; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Gerrit-Change-Number: 1100 Gerrit-PatchSet: 6 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
From: Gert D. <ge...@gr...> - 2025-07-25 18:06:13
|
Thanks for that work. I tested all the "easy" notifications (DEL_PEER and FLOAT), and can confirm that the race/mixup we observed in gerrit#1086 (issue #793) is now fixed for good - with sufficient patience it's still possible to inject a FLOAT message between "ask for peer stats" and "receive peer stats", but the new dispatching logic handles things just fine, we're no longer losing FLOAT messages, and life is good. I did not test KEY_SWAP_NTF as that's not easy to trigger (without kernel changes to lower the threshold) but as that code is not affected at all by this commit I do not expect any surprises there. Besides the manual test, this handled the DCO client/server test runs just fine. There's some suggestions to the code I still have, but "it works, ship it" (so we get more exposure to the code we considered fixed) - the cleanup can be a separate patch. Your patch has been applied to the master branch. commit f353b7100c859a02e70723c998594c3efd83c419 Author: Antonio Quartulli Date: Fri Jul 25 19:27:02 2025 +0200 dco_linux: fix async message reception Signed-off-by: Antonio Quartulli <an...@ma...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32339.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
From: Gert D. <ge...@gr...> - 2025-07-25 17:27:22
|
From: Antonio Quartulli <an...@ma...> Currently whenever we send a PEER_GET request to ovpn, we also set the CB that is supposed to parse the reply. However, due to the async nature of netlink messages, we could get an unrelated notification, sent by ovpn upon some event, after having set the CB, but before parsing the awaited reply. When this happens, the notification is then parsed with the configured CB instead of the notification parser, thus effectively rejecting the notification and losing the event. To fix this inconsistency, make ovpn_handle_msg() the default and only netlink parser CB. It is configured upon DCO initialization and is never removed. ovpn_handle_msg() will check the message type and will call the according parser. This way, no matter what message we get at what time, but we'll always parse it correctly. As a bonus we can also simplify the nl_sendmsg() API as we don't need to pass the cb and its argument anymore. The ID of the NLCTRL family is now also stored in the DCO context as we need it to check when we receive a mcast ID lookup message. Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Github: OpenVPN/openvpn#793 Signed-off-by: Antonio Quartulli <an...@ma...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1100 This mail reflects revision 5 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 7c639d9..728fb7e 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -167,23 +167,19 @@ } /** - * Send a prepared netlink message and registers cb as callback if non-null. + * Send a prepared netlink message. * * The method will also free nl_msg * @param dco The dco context to use * @param nl_msg the message to use - * @param cb An optional callback if the caller expects an answer - * @param cb_arg An optional param to pass to the callback * @param prefix A prefix to report in the error message to give the user context * @return status of sending the message */ static int -ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - void *cb_arg, const char *prefix) +ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, const char *prefix) { dco->status = 1; - nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -285,7 +281,7 @@ } nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -385,6 +381,29 @@ } static void +ovpn_dco_register(dco_context_t *dco) +{ + msg(D_DCO_DEBUG, __func__); + ovpn_get_mcast_id(dco); + + if (dco->ovpn_dco_mcast_id < 0) + { + msg(M_FATAL, "cannot get mcast group: %s", nl_geterror(dco->ovpn_dco_mcast_id)); + } + + /* Register for ovpn-dco specific multicast messages that the kernel may + * send + */ + int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); + if (ret) + { + msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); + } +} + +static int ovpn_handle_msg(struct nl_msg *msg, void *arg); + +static void ovpn_dco_init_netlink(dco_context_t *dco) { dco->ovpn_dco_id = resolve_ovpn_netlink_id(M_FATAL); @@ -420,11 +439,15 @@ nl_socket_set_cb(dco->nl_sock, dco->nl_cb); + dco->dco_message_peer_id = -1; nl_cb_err(dco->nl_cb, NL_CB_CUSTOM, ovpn_nl_cb_error, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_FINISH, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_ACK, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); + nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); + + ovpn_dco_register(dco); /* The async PACKET messages confuse libnl and it will drop them with * wrong sequence numbers (NLE_SEQ_MISMATCH), so disable libnl's sequence @@ -476,27 +499,6 @@ CLEAR(dco); } -static void -ovpn_dco_register(dco_context_t *dco) -{ - msg(D_DCO_DEBUG, __func__); - ovpn_get_mcast_id(dco); - - if (dco->ovpn_dco_mcast_id < 0) - { - msg(M_FATAL, "cannot get mcast group: %s", nl_geterror(dco->ovpn_dco_mcast_id)); - } - - /* Register for ovpn-dco specific multicast messages that the kernel may - * send - */ - int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); - if (ret) - { - msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); - } -} - int open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev) { @@ -516,10 +518,6 @@ msg(M_FATAL, "DCO: cannot retrieve ifindex for interface %s", dev); } - tt->dco.dco_message_peer_id = -1; - - ovpn_dco_register(&tt->dco); - return 0; } @@ -548,7 +546,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_KEYCONF_PEER_ID, peerid); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -572,7 +570,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peerid); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -598,7 +596,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_KEYCONF_SLOT, slot); nla_nest_end(nl_msg, keyconf); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -657,7 +655,7 @@ nla_nest_end(nl_msg, key_conf); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -686,7 +684,7 @@ keepalive_timeout); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -754,7 +752,7 @@ /* Even though 'nlctrl' is a constant, there seem to be no library * provided define for it */ - int ctrlid = genl_ctrl_resolve(dco->nl_sock, "nlctrl"); + dco->ctrlid = genl_ctrl_resolve(dco->nl_sock, "nlctrl"); struct nl_msg *nl_msg = nlmsg_alloc(); if (!nl_msg) @@ -762,12 +760,12 @@ return -ENOMEM; } - genlmsg_put(nl_msg, 0, 0, ctrlid, 0, 0, CTRL_CMD_GETFAMILY, 0); + genlmsg_put(nl_msg, 0, 0, dco->ctrlid, 0, 0, CTRL_CMD_GETFAMILY, 0); int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_FAMILY_NAME); - ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -879,31 +877,34 @@ } static int -dco_parse_peer_multi(struct nl_msg *msg, void *arg) +ovpn_handle_peer_multi(dco_context_t *dco, struct nlattr *attrs[]) { - struct nlattr *tb[OVPN_A_MAX + 1]; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - nla_parse(tb, OVPN_A_MAX, genlmsg_attrdata(gnlh, 0), - genlmsg_attrlen(gnlh, 0), NULL); + /* this function assumes openvpn is running in multipeer mode as + * it accesses c->multi + */ + if (dco->ifmode != OVPN_MODE_MP) + { + msg(M_WARN, "%s: can't parse 'multi-peer' message on P2P instance", __func__); + return NL_SKIP; + } - if (!tb[OVPN_A_PEER]) + if (!attrs[OVPN_A_PEER]) { return NL_SKIP; } struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, tb[OVPN_A_PEER], NULL); + nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); if (!tb_peer[OVPN_A_PEER_ID]) { - msg(M_WARN, "%s: no peer-id provided in reply", __func__); + msg(M_WARN, "ovpn-dco: no peer-id provided in (MULTI) PEER_GET reply"); return NL_SKIP; } - struct multi_context *m = arg; + struct multi_context *m = dco->c->multi; uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); if (peer_id >= m->max_clients || !m->instances[peer_id]) @@ -919,39 +920,53 @@ } static int -dco_parse_peer(struct nl_msg *msg, void *arg) +ovpn_handle_peer(dco_context_t *dco, struct nlattr *attrs[]) { - struct context *c = arg; - struct nlattr *tb[OVPN_A_MAX + 1]; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); - msg(D_DCO_DEBUG, "%s: parsing message...", __func__); - nla_parse(tb, OVPN_A_MAX, genlmsg_attrdata(gnlh, 0), - genlmsg_attrlen(gnlh, 0), NULL); - - if (!tb[OVPN_A_PEER]) + if (!attrs[OVPN_A_PEER]) { msg(D_DCO_DEBUG, "%s: malformed reply", __func__); return NL_SKIP; } struct nlattr *tb_peer[OVPN_A_PEER_MAX + 1]; - nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, tb[OVPN_A_PEER], NULL); + nla_parse_nested(tb_peer, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL); if (!tb_peer[OVPN_A_PEER_ID]) { - msg(M_WARN, "%s: no peer-id provided in reply", __func__); + msg(M_WARN, "ovpn-dco: no peer-id provided in PEER_GET reply"); return NL_SKIP; } uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]); - if (c->c2.tls_multi->dco_peer_id != peer_id) + struct context_2 *c2; + + if (dco->ifmode == OVPN_MODE_P2P) + { + c2 = &dco->c->c2; + } + else + { + struct multi_instance *mi = dco->c->multi->instances[peer_id]; + if (!mi) + { + msg(M_WARN, "%s: received data for a non-existing peer %u", __func__, peer_id); + return NL_SKIP; + } + + c2 = &mi->context.c2; + } + + /* at this point this check should never fail for MP mode, + * but it's still fully valid for P2P mode + */ + if (c2->tls_multi->dco_peer_id != peer_id) { return NL_SKIP; } - dco_update_peer_stat(&c->c2, tb_peer, peer_id); + dco_update_peer_stat(c2, tb_peer, peer_id); return NL_OK; } @@ -1120,9 +1135,22 @@ { dco_context_t *dco = arg; - struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg)); struct nlattr *attrs[OVPN_A_MAX + 1]; struct nlmsghdr *nlh = nlmsg_hdr(msg); + struct genlmsghdr *gnlh = genlmsg_hdr(nlh); + + msg(D_DCO_DEBUG, "ovpn-dco: received netlink message type=%u cmd=%u flags=%#.4x", + nlh->nlmsg_type, gnlh->cmd, nlh->nlmsg_flags); + + /* if we get a message from the NLCTRL family, it means + * this is the reply to the mcast ID resolution request + * and we parse it accordingly. + */ + if (nlh->nlmsg_type == dco->ctrlid) + { + msg(D_DCO_DEBUG, "ovpn-dco: received CTRLID message"); + return mcast_family_handler(msg, dco); + } if (!genlmsg_valid_hdr(nlh, 0)) { @@ -1146,6 +1174,21 @@ */ switch (gnlh->cmd) { + case OVPN_CMD_PEER_GET: + { + /* this message is part of a peer list dump, hence triggered + * by a MP/server instance + */ + if (nlh->nlmsg_flags & NLM_F_MULTI) + { + return ovpn_handle_peer_multi(dco, attrs); + } + else + { + return ovpn_handle_peer(dco, attrs); + } + } + case OVPN_CMD_PEER_DEL_NTF: { return ovpn_handle_peer_del_ntf(dco, attrs); @@ -1174,7 +1217,6 @@ dco_do_read(dco_context_t *dco) { msg(D_DCO_DEBUG, __func__); - nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); return ovpn_nl_recvmsgs(dco, __func__); } @@ -1189,7 +1231,7 @@ nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; - int ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer_multi, m, __func__); + int ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nlmsg_free(nl_msg); @@ -1227,7 +1269,7 @@ NLA_PUT_U32(nl_msg, OVPN_A_PEER_ID, peer_id); nla_nest_end(nl_msg, attr); - ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer, c, __func__); + ret = ovpn_nl_msg_send(dco, nl_msg, __func__); nla_put_failure: nlmsg_free(nl_msg); diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h index 5e61cf1..cc14f45 100644 --- a/src/openvpn/dco_linux.h +++ b/src/openvpn/dco_linux.h @@ -66,6 +66,7 @@ int status; struct context *c; + int ctrlid; enum ovpn_mode ifmode; |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 17:20:41
|
Attention is currently required from: flichtenheld, ordex, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email ) Change subject: dco_linux: fix async message reception ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Patchset: PS5: Hah, gotcha ``` Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_get_peer_stats_multi Jul 25 19:18:56 ubuntu2004 tun-udp-p2p-tls-sha256[186220]: dco_do_read Jul 25 19:18:56 ubuntu2004 tun-udp-p2p-tls-sha256[186220]: ovpn-dco: received netlink message type=31 cmd=11 flags=0000 Jul 25 19:18:56 ubuntu2004 tun-udp-p2p-tls-sha256[186220]: ovpn-dco: ignoring message for foreign ifindex 982 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: ovpn-dco: received netlink message type=31 cmd=11 flags=0000 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: ovpn-dco: received CMD_PEER_FLOAT_NTF, ifindex: 982, peer-id 2, address: [AF_INET]193.149.48.173:59979 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: ovpn-dco: received netlink message type=31 cmd=3 flags=0x0002 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: ovpn_handle_peer_multi: parsing message... Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / dco_read_bytes(0): 615704 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / dco_write_bytes(0): 625840 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / tun_read_bytes(0): 562992 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / tun_write_bytes(0): 568872 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: ovpn-dco: received netlink message type=31 cmd=3 flags=0x0002 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: ovpn_handle_peer_multi: parsing message... Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / dco_read_bytes(2): 11732 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / dco_write_bytes(2): 11392 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / tun_read_bytes(2): 9256 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_update_peer_stat / tun_write_bytes(2): 9256 Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_do_read Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: dco_do_read: netlink reports blocking read - aborting wait Jul 25 19:18:56 ubuntu2004 tun-udp-p2mp[186147]: peer 2 (cron2-freebsd-tc-amd64) floated from udp6:193.149.48.172:59979 to [AF_INET6]::ffff:193.149.48.173:59979 Jul 25 19:18:57 ubuntu2004 kernel: [294871.134548] tun1: peer 2 floated to 193.149.48.173:59979 ``` this is precisely the situation that makes the pre-1100 code lose the "FLOAT!" message, because it arrives *after* `dco_get_peer_stats_multi()` starts running (or just before, but we do not look at DCO events) and before the actual cmd=3 reply comes in. Good work! -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Gerrit-Change-Number: 1100 Gerrit-PatchSet: 5 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <an...@ma...> Gerrit-Comment-Date: Fri, 25 Jul 2025 17:20:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 17:05:03
|
Attention is currently required from: flichtenheld, ordex, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email ) Change subject: dco_linux: fix async message reception ...................................................................... Patch Set 5: (1 comment) File src/openvpn/dco_linux.c: http://gerrit.openvpn.net/c/openvpn/+/1100/comment/acf86e82_8c8c0854 : PS4, Line 1189: } > as mentioned above, MODE_MP may still need to call ovpn_handle_peer() Now you got me started... if we need to have one function handle both P2P and P2MP modes anyway, why do we need two functions? They look similar enough that using `ovpn_handle_peer()` for all `OVPN_CMD_PEER_GET` replies might just work (possibly moving the peer-id check inside the P2P clause to avoid having it in the MP path where it's more like an CANTHAPPEN...)? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1100?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I23ad79e14844aefde9ece34dadef0b75ff267201 Gerrit-Change-Number: 1100 Gerrit-PatchSet: 5 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <an...@ma...> Gerrit-Comment-Date: Fri, 25 Jul 2025 17:04:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Comment-In-Reply-To: ordex <an...@ma...> Gerrit-MessageType: comment |
From: ralf_lici (C. Review) <ge...@op...> - 2025-07-25 16:46:10
|
Attention is currently required from: flichtenheld, plaisthos. ralf_lici has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1113?usp=email ) Change subject: management: resync timer on bytecount interval change ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > please rebase on current master to fix build issue Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1113?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ic0035d52e0ea123398318870d2f4d21af927a602 Gerrit-Change-Number: 1113 Gerrit-PatchSet: 2 Gerrit-Owner: ralf_lici <ra...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 25 Jul 2025 16:46:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> Gerrit-MessageType: comment |
From: flichtenheld (C. Review) <ge...@op...> - 2025-07-25 16:41:04
|
Attention is currently required from: plaisthos, ralf_lici. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1113?usp=email ) Change subject: management: resync timer on bytecount interval change ...................................................................... Patch Set 1: Code-Review-1 (1 comment) Patchset: PS1: please rebase on current master to fix build issue -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1113?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ic0035d52e0ea123398318870d2f4d21af927a602 Gerrit-Change-Number: 1113 Gerrit-PatchSet: 1 Gerrit-Owner: ralf_lici <ra...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: ralf_lici <ra...@ma...> Gerrit-Comment-Date: Fri, 25 Jul 2025 16:40:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 15:40:21
|
cron2 has uploaded a new patch set (#2) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/1111?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: plugins: Clean up -Wconversion warnings ...................................................................... plugins: Clean up -Wconversion warnings Most of the are actually the same ones copied to every single plugin. Some drive-by fixes of other warnings and some conversion cleanups that had no warnings because they were suppressed by casts. Change-Id: Id61df43bd79fc794a55e107daa0218c8441c2b2c Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32330.html Signed-off-by: Gert Doering <ge...@gr...> --- M sample/sample-plugins/client-connect/sample-client-connect.c M sample/sample-plugins/defer/multi-auth.c M sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c M sample/sample-plugins/log/log.c M sample/sample-plugins/log/log_v3.c M sample/sample-plugins/simple/base64.c M sample/sample-plugins/simple/simple.c M src/plugins/auth-pam/auth-pam.c M src/plugins/auth-pam/utils.c M src/plugins/down-root/down-root.c 10 files changed, 42 insertions(+), 54 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/11/1111/2 diff --git a/sample/sample-plugins/client-connect/sample-client-connect.c b/sample/sample-plugins/client-connect/sample-client-connect.c index 18c2c6f..5194270 100644 --- a/sample/sample-plugins/client-connect/sample-client-connect.c +++ b/sample/sample-plugins/client-connect/sample-client-connect.c @@ -86,9 +86,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -309,7 +308,7 @@ /* do mighty complicated work that will really take time here... */ plugin_log(PLOG_NOTE, MODULE, "in async/deferred handler, sleep(%d)", seconds); - sleep(seconds); + sleep((unsigned int)seconds); /* write config options to openvpn */ int ret = write_cc_options_file(name, envp); diff --git a/sample/sample-plugins/defer/multi-auth.c b/sample/sample-plugins/defer/multi-auth.c index 38db07f..c458346 100644 --- a/sample/sample-plugins/defer/multi-auth.c +++ b/sample/sample-plugins/defer/multi-auth.c @@ -124,9 +124,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -170,7 +169,7 @@ /* Require a minimum OpenVPN Plugin API */ OPENVPN_EXPORT int -openvpn_plugin_min_version_required_v1() +openvpn_plugin_min_version_required_v1(void) { return OPENVPN_PLUGIN_VERSION_MIN; } @@ -349,9 +348,9 @@ */ /* do mighty complicated work that will really take time here... */ - plog(context, PLOG_NOTE, "in async/deferred handler, usleep(%d)", - context->test_deferred_auth*1000); - usleep(context->test_deferred_auth*1000); + useconds_t wait_time = (useconds_t)context->test_deferred_auth*1000; + plog(context, PLOG_NOTE, "in async/deferred handler, usleep(%u)", wait_time); + usleep(wait_time); /* now signal success state to openvpn */ int fd = open(auth_control_file, O_WRONLY); diff --git a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c index cc256dd..f1f8868 100644 --- a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c +++ b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c @@ -69,9 +69,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/log/log.c b/sample/sample-plugins/log/log.c index 82595cf..b304f16 100644 --- a/sample/sample-plugins/log/log.c +++ b/sample/sample-plugins/log/log.c @@ -52,9 +52,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/log/log_v3.c b/sample/sample-plugins/log/log_v3.c index c90cc3d..a024027 100644 --- a/sample/sample-plugins/log/log_v3.c +++ b/sample/sample-plugins/log/log_v3.c @@ -55,9 +55,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/simple/base64.c b/sample/sample-plugins/simple/base64.c index 6855966..e1cc791 100644 --- a/sample/sample-plugins/simple/base64.c +++ b/sample/sample-plugins/simple/base64.c @@ -59,9 +59,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -175,7 +174,7 @@ /* test the BASE64 encode function */ char *buf = NULL; - int r = ovpn_base64_encode(clcert_cn, strlen(clcert_cn), &buf); + int r = ovpn_base64_encode(clcert_cn, (int)strlen(clcert_cn), &buf); ovpn_log(PLOG_NOTE, PLUGIN_NAME, "BASE64 encoded '%s' (return value %i): '%s'", clcert_cn, r, buf); diff --git a/sample/sample-plugins/simple/simple.c b/sample/sample-plugins/simple/simple.c index e17f3fa..2c096e2 100644 --- a/sample/sample-plugins/simple/simple.c +++ b/sample/sample-plugins/simple/simple.c @@ -54,9 +54,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c index 8692806..7264f95 100644 --- a/src/plugins/auth-pam/auth-pam.c +++ b/src/plugins/auth-pam/auth-pam.c @@ -165,31 +165,30 @@ } } -static int -recv_string(int fd, char *buffer, int len) +static ssize_t +recv_string(int fd, char *buffer, size_t len) { if (len > 0) { - ssize_t size; memset(buffer, 0, len); - size = read(fd, buffer, len); + ssize_t size = read(fd, buffer, len); buffer[len-1] = 0; if (size >= 1) { - return (int)size; + return size; } } return -1; } -static int +static ssize_t send_string(int fd, const char *string) { - const int len = strlen(string) + 1; + const size_t len = strlen(string) + 1; const ssize_t size = write(fd, string, len); if (size == len) { - return (int) size; + return size; } else { @@ -645,27 +644,26 @@ * PAM conversation function */ static int -my_conv(int n, const struct pam_message **msg_array, +my_conv(int num_msg, const struct pam_message **msg_array, struct pam_response **response_array, void *appdata_ptr) { const struct user_pass *up = ( const struct user_pass *) appdata_ptr; struct pam_response *aresp; - int i; int ret = PAM_SUCCESS; *response_array = NULL; - if (n <= 0 || n > PAM_MAX_NUM_MSG) + if (num_msg <= 0 || num_msg > PAM_MAX_NUM_MSG) { return (PAM_CONV_ERR); } - if ((aresp = calloc(n, sizeof *aresp)) == NULL) + if ((aresp = calloc((size_t)num_msg, sizeof *aresp)) == NULL) { return (PAM_BUF_ERR); } /* loop through each PAM-module query */ - for (i = 0; i < n; ++i) + for (int i = 0; i < num_msg; ++i) { const struct pam_message *msg = msg_array[i]; aresp[i].resp_retcode = 0; @@ -683,9 +681,9 @@ { /* use name/value list match method */ const struct name_value_list *list = up->name_value_list; - int j; /* loop through name/value pairs */ + int j; /* checked after loop */ for (j = 0; j < list->len; ++j) { const char *match_name = list->data[j].name; diff --git a/src/plugins/auth-pam/utils.c b/src/plugins/auth-pam/utils.c index a6ccfbf..ab5e17e 100644 --- a/src/plugins/auth-pam/utils.c +++ b/src/plugins/auth-pam/utils.c @@ -79,7 +79,7 @@ while (scratch) { - strncat(temp, searching, scratch-searching); + strncat(temp, searching, (size_t)(scratch-searching)); strcat(temp, replacewith); searching = scratch+strlen(searchfor); @@ -93,9 +93,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/src/plugins/down-root/down-root.c b/src/plugins/down-root/down-root.c index 253adcd..2c0faf4 100644 --- a/src/plugins/down-root/down-root.c +++ b/src/plugins/down-root/down-root.c @@ -88,9 +88,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -108,10 +107,10 @@ /* * Return the length of a string array */ -static int +static size_t string_array_len(const char *array[]) { - int i = 0; + size_t i = 0; if (array) { while (array[i]) @@ -141,14 +140,14 @@ } } -static int +static ssize_t send_control(int fd, int code) { unsigned char c = (unsigned char) code; const ssize_t size = write(fd, &c, sizeof(c)); if (size == sizeof(c)) { - return (int) size; + return size; } else { @@ -281,7 +280,6 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[]) { struct down_root_context *context; - int i = 0; /* * Allocate our context @@ -320,7 +318,7 @@ } /* Ignore argv[0], as it contains just the plug-in file name */ - for (i = 1; i < string_array_len(argv); i++) + for (int i = 1; i < string_array_len(argv); i++) { context->command[i-1] = (char *) argv[i]; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1111?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id61df43bd79fc794a55e107daa0218c8441c2b2c Gerrit-Change-Number: 1111 Gerrit-PatchSet: 2 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
From: cron2 (C. Review) <ge...@op...> - 2025-07-25 15:40:17
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1111?usp=email ) Change subject: plugins: Clean up -Wconversion warnings ...................................................................... plugins: Clean up -Wconversion warnings Most of the are actually the same ones copied to every single plugin. Some drive-by fixes of other warnings and some conversion cleanups that had no warnings because they were suppressed by casts. Change-Id: Id61df43bd79fc794a55e107daa0218c8441c2b2c Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32330.html Signed-off-by: Gert Doering <ge...@gr...> --- M sample/sample-plugins/client-connect/sample-client-connect.c M sample/sample-plugins/defer/multi-auth.c M sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c M sample/sample-plugins/log/log.c M sample/sample-plugins/log/log_v3.c M sample/sample-plugins/simple/base64.c M sample/sample-plugins/simple/simple.c M src/plugins/auth-pam/auth-pam.c M src/plugins/auth-pam/utils.c M src/plugins/down-root/down-root.c 10 files changed, 42 insertions(+), 54 deletions(-) diff --git a/sample/sample-plugins/client-connect/sample-client-connect.c b/sample/sample-plugins/client-connect/sample-client-connect.c index 18c2c6f..5194270 100644 --- a/sample/sample-plugins/client-connect/sample-client-connect.c +++ b/sample/sample-plugins/client-connect/sample-client-connect.c @@ -86,9 +86,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -309,7 +308,7 @@ /* do mighty complicated work that will really take time here... */ plugin_log(PLOG_NOTE, MODULE, "in async/deferred handler, sleep(%d)", seconds); - sleep(seconds); + sleep((unsigned int)seconds); /* write config options to openvpn */ int ret = write_cc_options_file(name, envp); diff --git a/sample/sample-plugins/defer/multi-auth.c b/sample/sample-plugins/defer/multi-auth.c index 38db07f..c458346 100644 --- a/sample/sample-plugins/defer/multi-auth.c +++ b/sample/sample-plugins/defer/multi-auth.c @@ -124,9 +124,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -170,7 +169,7 @@ /* Require a minimum OpenVPN Plugin API */ OPENVPN_EXPORT int -openvpn_plugin_min_version_required_v1() +openvpn_plugin_min_version_required_v1(void) { return OPENVPN_PLUGIN_VERSION_MIN; } @@ -349,9 +348,9 @@ */ /* do mighty complicated work that will really take time here... */ - plog(context, PLOG_NOTE, "in async/deferred handler, usleep(%d)", - context->test_deferred_auth*1000); - usleep(context->test_deferred_auth*1000); + useconds_t wait_time = (useconds_t)context->test_deferred_auth*1000; + plog(context, PLOG_NOTE, "in async/deferred handler, usleep(%u)", wait_time); + usleep(wait_time); /* now signal success state to openvpn */ int fd = open(auth_control_file, O_WRONLY); diff --git a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c index cc256dd..f1f8868 100644 --- a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c +++ b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c @@ -69,9 +69,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/log/log.c b/sample/sample-plugins/log/log.c index 82595cf..b304f16 100644 --- a/sample/sample-plugins/log/log.c +++ b/sample/sample-plugins/log/log.c @@ -52,9 +52,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/log/log_v3.c b/sample/sample-plugins/log/log_v3.c index c90cc3d..a024027 100644 --- a/sample/sample-plugins/log/log_v3.c +++ b/sample/sample-plugins/log/log_v3.c @@ -55,9 +55,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/simple/base64.c b/sample/sample-plugins/simple/base64.c index 6855966..e1cc791 100644 --- a/sample/sample-plugins/simple/base64.c +++ b/sample/sample-plugins/simple/base64.c @@ -59,9 +59,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -175,7 +174,7 @@ /* test the BASE64 encode function */ char *buf = NULL; - int r = ovpn_base64_encode(clcert_cn, strlen(clcert_cn), &buf); + int r = ovpn_base64_encode(clcert_cn, (int)strlen(clcert_cn), &buf); ovpn_log(PLOG_NOTE, PLUGIN_NAME, "BASE64 encoded '%s' (return value %i): '%s'", clcert_cn, r, buf); diff --git a/sample/sample-plugins/simple/simple.c b/sample/sample-plugins/simple/simple.c index e17f3fa..2c096e2 100644 --- a/sample/sample-plugins/simple/simple.c +++ b/sample/sample-plugins/simple/simple.c @@ -54,9 +54,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c index 8692806..7264f95 100644 --- a/src/plugins/auth-pam/auth-pam.c +++ b/src/plugins/auth-pam/auth-pam.c @@ -165,31 +165,30 @@ } } -static int -recv_string(int fd, char *buffer, int len) +static ssize_t +recv_string(int fd, char *buffer, size_t len) { if (len > 0) { - ssize_t size; memset(buffer, 0, len); - size = read(fd, buffer, len); + ssize_t size = read(fd, buffer, len); buffer[len-1] = 0; if (size >= 1) { - return (int)size; + return size; } } return -1; } -static int +static ssize_t send_string(int fd, const char *string) { - const int len = strlen(string) + 1; + const size_t len = strlen(string) + 1; const ssize_t size = write(fd, string, len); if (size == len) { - return (int) size; + return size; } else { @@ -645,27 +644,26 @@ * PAM conversation function */ static int -my_conv(int n, const struct pam_message **msg_array, +my_conv(int num_msg, const struct pam_message **msg_array, struct pam_response **response_array, void *appdata_ptr) { const struct user_pass *up = ( const struct user_pass *) appdata_ptr; struct pam_response *aresp; - int i; int ret = PAM_SUCCESS; *response_array = NULL; - if (n <= 0 || n > PAM_MAX_NUM_MSG) + if (num_msg <= 0 || num_msg > PAM_MAX_NUM_MSG) { return (PAM_CONV_ERR); } - if ((aresp = calloc(n, sizeof *aresp)) == NULL) + if ((aresp = calloc((size_t)num_msg, sizeof *aresp)) == NULL) { return (PAM_BUF_ERR); } /* loop through each PAM-module query */ - for (i = 0; i < n; ++i) + for (int i = 0; i < num_msg; ++i) { const struct pam_message *msg = msg_array[i]; aresp[i].resp_retcode = 0; @@ -683,9 +681,9 @@ { /* use name/value list match method */ const struct name_value_list *list = up->name_value_list; - int j; /* loop through name/value pairs */ + int j; /* checked after loop */ for (j = 0; j < list->len; ++j) { const char *match_name = list->data[j].name; diff --git a/src/plugins/auth-pam/utils.c b/src/plugins/auth-pam/utils.c index a6ccfbf..ab5e17e 100644 --- a/src/plugins/auth-pam/utils.c +++ b/src/plugins/auth-pam/utils.c @@ -79,7 +79,7 @@ while (scratch) { - strncat(temp, searching, scratch-searching); + strncat(temp, searching, (size_t)(scratch-searching)); strcat(temp, replacewith); searching = scratch+strlen(searchfor); @@ -93,9 +93,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/src/plugins/down-root/down-root.c b/src/plugins/down-root/down-root.c index 253adcd..2c0faf4 100644 --- a/src/plugins/down-root/down-root.c +++ b/src/plugins/down-root/down-root.c @@ -88,9 +88,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -108,10 +107,10 @@ /* * Return the length of a string array */ -static int +static size_t string_array_len(const char *array[]) { - int i = 0; + size_t i = 0; if (array) { while (array[i]) @@ -141,14 +140,14 @@ } } -static int +static ssize_t send_control(int fd, int code) { unsigned char c = (unsigned char) code; const ssize_t size = write(fd, &c, sizeof(c)); if (size == sizeof(c)) { - return (int) size; + return size; } else { @@ -281,7 +280,6 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[]) { struct down_root_context *context; - int i = 0; /* * Allocate our context @@ -320,7 +318,7 @@ } /* Ignore argv[0], as it contains just the plug-in file name */ - for (i = 1; i < string_array_len(argv); i++) + for (int i = 1; i < string_array_len(argv); i++) { context->command[i-1] = (char *) argv[i]; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1111?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id61df43bd79fc794a55e107daa0218c8441c2b2c Gerrit-Change-Number: 1111 Gerrit-PatchSet: 2 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
From: Gert D. <ge...@gr...> - 2025-07-25 15:38:08
|
Stared long and hard at the changes, some of which clearly look like I and others have been busy copy-pasting sample code around :-) I haven't actually tested this beyond a test-compile ("make all" in the sample-plugins directory, the BBs test-compiling the other plugins) - easy and straightforward enough. I took note there is an "useconds_t" now, but I guess that all platforms having usleep() will also have this... Your patch has been applied to the master branch. commit df3ac551259865a0826d4571a3ae48bb1bdf38e3 Author: Frank Lichtenheld Date: Fri Jul 25 14:44:09 2025 +0200 plugins: Clean up -Wconversion warnings Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg32330.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
From: ralf_lici (C. Review) <ge...@op...> - 2025-07-25 13:15:35
|
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1113?usp=email to review the following change. Change subject: management: resync timer on bytecount interval change ...................................................................... management: resync timer on bytecount interval change coarse_timer_wakeup tracks when the next timer-driven task will occur. When a user issues `bytecount n` on the management interface but the existing wakeup is more than n seconds ahead, bandwidth logging won’t run until that original timer fires, delaying logs. Introduce a flag to detect when the bytecount interval changes and, when set, recalculate coarse_timer_wakeup so logging fires exactly n seconds after the command. This guarantees bytecount adheres to the user-specified interval. Change-Id: Ic0035d52e0ea123398318870d2f4d21af927a602 Signed-off-by: Ralf Lici <ra...@ma...> --- M src/openvpn/forward.c M src/openvpn/manage.c M src/openvpn/manage.h 3 files changed, 15 insertions(+), 0 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/13/1113/1 diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index a4f260a..192fff4 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -838,6 +838,19 @@ static void check_coarse_timers(struct context *c) { +#ifdef ENABLE_MANAGEMENT + /* The 'bytecount' command starts a timer at runtime, but it would not be + * processed if coarse_timer_wakeup was previously set to a higher value. + * Therefore, if the command has arrived, we reset coarse_timer_wakeup in + * to order to update it accordingly. + */ + if (management && management->connection.bytecount_interval_changed) + { + reset_coarse_timers(c); + management->connection.bytecount_interval_changed = false; + } +#endif /* ENABLE_MANAGEMENT */ + if (now < c->c2.coarse_timer_wakeup) { context_reschedule_sec(c, c->c2.coarse_timer_wakeup - now); diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 8836e79..0df78ee 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -503,6 +503,7 @@ man->connection.bytecount_update_seconds = 0; event_timeout_clear(&man->connection.bytecount_update_interval); } + man->connection.bytecount_interval_changed = true; msg(M_CLIENT, "SUCCESS: bytecount interval changed"); } diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index eb19a4e..00e3931 100644 --- a/src/openvpn/manage.h +++ b/src/openvpn/manage.h @@ -318,6 +318,7 @@ bool state_realtime; bool log_realtime; bool echo_realtime; + bool bytecount_interval_changed; int bytecount_update_seconds; struct event_timeout bytecount_update_interval; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1113?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ic0035d52e0ea123398318870d2f4d21af927a602 Gerrit-Change-Number: 1113 Gerrit-PatchSet: 1 Gerrit-Owner: ralf_lici <ra...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: Gert D. <ge...@gr...> - 2025-07-25 12:44:29
|
From: Frank Lichtenheld <fr...@li...> Most of the are actually the same ones copied to every single plugin. Some drive-by fixes of other warnings and some conversion cleanups that had no warnings because they were suppressed by casts. Change-Id: Id61df43bd79fc794a55e107daa0218c8441c2b2c Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1111 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/sample/sample-plugins/client-connect/sample-client-connect.c b/sample/sample-plugins/client-connect/sample-client-connect.c index 18c2c6f..5194270 100644 --- a/sample/sample-plugins/client-connect/sample-client-connect.c +++ b/sample/sample-plugins/client-connect/sample-client-connect.c @@ -86,9 +86,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -309,7 +308,7 @@ /* do mighty complicated work that will really take time here... */ plugin_log(PLOG_NOTE, MODULE, "in async/deferred handler, sleep(%d)", seconds); - sleep(seconds); + sleep((unsigned int)seconds); /* write config options to openvpn */ int ret = write_cc_options_file(name, envp); diff --git a/sample/sample-plugins/defer/multi-auth.c b/sample/sample-plugins/defer/multi-auth.c index 38db07f..c458346 100644 --- a/sample/sample-plugins/defer/multi-auth.c +++ b/sample/sample-plugins/defer/multi-auth.c @@ -124,9 +124,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -170,7 +169,7 @@ /* Require a minimum OpenVPN Plugin API */ OPENVPN_EXPORT int -openvpn_plugin_min_version_required_v1() +openvpn_plugin_min_version_required_v1(void) { return OPENVPN_PLUGIN_VERSION_MIN; } @@ -349,9 +348,9 @@ */ /* do mighty complicated work that will really take time here... */ - plog(context, PLOG_NOTE, "in async/deferred handler, usleep(%d)", - context->test_deferred_auth*1000); - usleep(context->test_deferred_auth*1000); + useconds_t wait_time = (useconds_t)context->test_deferred_auth*1000; + plog(context, PLOG_NOTE, "in async/deferred handler, usleep(%u)", wait_time); + usleep(wait_time); /* now signal success state to openvpn */ int fd = open(auth_control_file, O_WRONLY); diff --git a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c index cc256dd..f1f8868 100644 --- a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c +++ b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c @@ -69,9 +69,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/log/log.c b/sample/sample-plugins/log/log.c index 82595cf..b304f16 100644 --- a/sample/sample-plugins/log/log.c +++ b/sample/sample-plugins/log/log.c @@ -52,9 +52,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/log/log_v3.c b/sample/sample-plugins/log/log_v3.c index c90cc3d..a024027 100644 --- a/sample/sample-plugins/log/log_v3.c +++ b/sample/sample-plugins/log/log_v3.c @@ -55,9 +55,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/sample/sample-plugins/simple/base64.c b/sample/sample-plugins/simple/base64.c index 6855966..e1cc791 100644 --- a/sample/sample-plugins/simple/base64.c +++ b/sample/sample-plugins/simple/base64.c @@ -59,9 +59,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -175,7 +174,7 @@ /* test the BASE64 encode function */ char *buf = NULL; - int r = ovpn_base64_encode(clcert_cn, strlen(clcert_cn), &buf); + int r = ovpn_base64_encode(clcert_cn, (int)strlen(clcert_cn), &buf); ovpn_log(PLOG_NOTE, PLUGIN_NAME, "BASE64 encoded '%s' (return value %i): '%s'", clcert_cn, r, buf); diff --git a/sample/sample-plugins/simple/simple.c b/sample/sample-plugins/simple/simple.c index e17f3fa..2c096e2 100644 --- a/sample/sample-plugins/simple/simple.c +++ b/sample/sample-plugins/simple/simple.c @@ -54,9 +54,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c index 8692806..7264f95 100644 --- a/src/plugins/auth-pam/auth-pam.c +++ b/src/plugins/auth-pam/auth-pam.c @@ -165,31 +165,30 @@ } } -static int -recv_string(int fd, char *buffer, int len) +static ssize_t +recv_string(int fd, char *buffer, size_t len) { if (len > 0) { - ssize_t size; memset(buffer, 0, len); - size = read(fd, buffer, len); + ssize_t size = read(fd, buffer, len); buffer[len-1] = 0; if (size >= 1) { - return (int)size; + return size; } } return -1; } -static int +static ssize_t send_string(int fd, const char *string) { - const int len = strlen(string) + 1; + const size_t len = strlen(string) + 1; const ssize_t size = write(fd, string, len); if (size == len) { - return (int) size; + return size; } else { @@ -645,27 +644,26 @@ * PAM conversation function */ static int -my_conv(int n, const struct pam_message **msg_array, +my_conv(int num_msg, const struct pam_message **msg_array, struct pam_response **response_array, void *appdata_ptr) { const struct user_pass *up = ( const struct user_pass *) appdata_ptr; struct pam_response *aresp; - int i; int ret = PAM_SUCCESS; *response_array = NULL; - if (n <= 0 || n > PAM_MAX_NUM_MSG) + if (num_msg <= 0 || num_msg > PAM_MAX_NUM_MSG) { return (PAM_CONV_ERR); } - if ((aresp = calloc(n, sizeof *aresp)) == NULL) + if ((aresp = calloc((size_t)num_msg, sizeof *aresp)) == NULL) { return (PAM_BUF_ERR); } /* loop through each PAM-module query */ - for (i = 0; i < n; ++i) + for (int i = 0; i < num_msg; ++i) { const struct pam_message *msg = msg_array[i]; aresp[i].resp_retcode = 0; @@ -683,9 +681,9 @@ { /* use name/value list match method */ const struct name_value_list *list = up->name_value_list; - int j; /* loop through name/value pairs */ + int j; /* checked after loop */ for (j = 0; j < list->len; ++j) { const char *match_name = list->data[j].name; diff --git a/src/plugins/auth-pam/utils.c b/src/plugins/auth-pam/utils.c index a6ccfbf..ab5e17e 100644 --- a/src/plugins/auth-pam/utils.c +++ b/src/plugins/auth-pam/utils.c @@ -79,7 +79,7 @@ while (scratch) { - strncat(temp, searching, scratch-searching); + strncat(temp, searching, (size_t)(scratch-searching)); strcat(temp, replacewith); searching = scratch+strlen(searchfor); @@ -93,9 +93,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { diff --git a/src/plugins/down-root/down-root.c b/src/plugins/down-root/down-root.c index 253adcd..2c0faf4 100644 --- a/src/plugins/down-root/down-root.c +++ b/src/plugins/down-root/down-root.c @@ -88,9 +88,8 @@ { if (envp) { - int i; - const int namelen = strlen(name); - for (i = 0; envp[i]; ++i) + const size_t namelen = strlen(name); + for (int i = 0; envp[i]; ++i) { if (!strncmp(envp[i], name, namelen)) { @@ -108,10 +107,10 @@ /* * Return the length of a string array */ -static int +static size_t string_array_len(const char *array[]) { - int i = 0; + size_t i = 0; if (array) { while (array[i]) @@ -141,14 +140,14 @@ } } -static int +static ssize_t send_control(int fd, int code) { unsigned char c = (unsigned char) code; const ssize_t size = write(fd, &c, sizeof(c)); if (size == sizeof(c)) { - return (int) size; + return size; } else { @@ -281,7 +280,6 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[]) { struct down_root_context *context; - int i = 0; /* * Allocate our context @@ -320,7 +318,7 @@ } /* Ignore argv[0], as it contains just the plug-in file name */ - for (i = 1; i < string_array_len(argv); i++) + for (int i = 1; i < string_array_len(argv); i++) { context->command[i-1] = (char *) argv[i]; } |