| Message ID | 20230118074633.27586-1-gert@greenie.muc.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Repair special-casing of EEXIST for Linux/SITNL route install | expand |
Hi, On Wed, Jan 18, 2023 at 2:47 AM Gert Doering <gert@greenie.muc.de> wrote: > The code in sitnl_route_set() used to treat "route can not be installed > because it already exists" (EEXIST) as "not an error". > > This is arguably a reasonable approach, but needs to handled higher > up - if the low level add_route() function say "no error", we will try > to remove that route later on in delete_route(), possibly removing > someone else's "already existing" route then. > > So: > - remove special case in sitnl_route_set() > - do not pass NLM_F_REPLACE flag to sitnl_route_set() call - this would > cause netlink to just replace existing routes, never return EEXIST > (see "man netlink(7)") > - add detailed return code handling to add_route(), assign "2" on > "-EEXIST" > (and log appropriate message). > > (Note: sitnl_route_set() is a common function for sitnl route add and > delete, but EEXIST can not happen on delete - so this change has no > impact for the "delete" case) > > v2: use RTA_ macros, also adjust add_route_ipv6() > Looks good now :) > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/networking_sitnl.c | 6 +----- > src/openvpn/route.c | 24 ++++++++++++++++++------ > 2 files changed, 19 insertions(+), 11 deletions(-) > > diff --git a/src/openvpn/networking_sitnl.c > b/src/openvpn/networking_sitnl.c > index dece83c2..cb9a47c0 100644 > --- a/src/openvpn/networking_sitnl.c > +++ b/src/openvpn/networking_sitnl.c > @@ -944,10 +944,6 @@ sitnl_route_set(int cmd, uint32_t flags, int ifindex, > sa_family_t af_family, > } > > ret = sitnl_send(&req.n, 0, 0, NULL, NULL); > - if (ret == -EEXIST) > - { > - ret = 0; > - } > err: > return ret; > } > @@ -1177,7 +1173,7 @@ sitnl_route_add(const char *iface, sa_family_t > af_family, const void *dst, > scope = RT_SCOPE_LINK; > } > > - return sitnl_route_set(RTM_NEWROUTE, NLM_F_CREATE | NLM_F_REPLACE, > ifindex, > + return sitnl_route_set(RTM_NEWROUTE, NLM_F_CREATE, ifindex, > af_family, dst, prefixlen, gw, table, metric, > scope, > RTPROT_BOOT, RTN_UNICAST); > } > diff --git a/src/openvpn/route.c b/src/openvpn/route.c > index 61a40ff1..f1257b00 100644 > --- a/src/openvpn/route.c > +++ b/src/openvpn/route.c > @@ -1599,8 +1599,14 @@ add_route(struct route_ipv4 *r, > } > > status = RTA_SUCCESS; > - if (net_route_v4_add(ctx, &r->network, > netmask_to_netbits2(r->netmask), > - &r->gateway, iface, 0, metric) < 0) > + int ret = net_route_v4_add(ctx, &r->network, > netmask_to_netbits2(r->netmask), > + &r->gateway, iface, 0, metric); > + if (ret == -EEXIST) > + { > + msg(D_ROUTE, "NOTE: Linux route add command failed because route > exists"); > + status = RTA_EEXIST; > + } > + else if (ret < 0) > { > msg(M_WARN, "ERROR: Linux route add command failed"); > status = RTA_ERROR; > @@ -1963,11 +1969,17 @@ add_route_ipv6(struct route_ipv6 *r6, const struct > tuntap *tt, > } > > status = RTA_SUCCESS; > - if (net_route_v6_add(ctx, &r6->network, r6->netbits, > - gateway_needed ? &r6->gateway : NULL, device, 0, > - metric) < 0) > + int ret = net_route_v6_add(ctx, &r6->network, r6->netbits, > + gateway_needed ? &r6->gateway : NULL, > + device, 0, metric); > + if (ret == -EEXIST) > { > - msg(M_WARN, "ERROR: Linux IPv6 route can't be added"); > + msg(D_ROUTE, "NOTE: Linux route add command failed because route > exists"); > Even the word "Linux" is redundant here and elsewhere, but we need more changes to make logging consistent and avoid duplicates. Something for 2.7. > + status = RTA_EEXIST; > + } > + else if (ret < 0) > + { > + msg(M_WARN, "ERROR: Linux route add command failed"); > status = RTA_ERROR; > } > Acked-by: Selva Nair <selva.nair@gmail.com>
For good measure, ran through all the client & server side tests on
Linux (other platforms not affected). Since this is where I developed
and tested the patch, no surprises :-)
Thanks to having RTA_SUCCESS/RTA_EEXIST now, doing a double-review
again ("did I get all the numbers right?") was also very straightforward.
Patch has been applied to the master and release/2.6 branch.
commit adc54f483b210484ff1488e01c8aee1b2b0ea477 (master)
commit eb85961e72c927256ce9267a43f3519d3ab4edcb (release/2.6)
Author: Gert Doering
Date: Wed Jan 18 08:46:33 2023 +0100
Repair special-casing of EEXIST for Linux/SITNL route install
Signed-off-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20230118074633.27586-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26046.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index dece83c2..cb9a47c0 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -944,10 +944,6 @@ sitnl_route_set(int cmd, uint32_t flags, int ifindex, sa_family_t af_family, } ret = sitnl_send(&req.n, 0, 0, NULL, NULL); - if (ret == -EEXIST) - { - ret = 0; - } err: return ret; } @@ -1177,7 +1173,7 @@ sitnl_route_add(const char *iface, sa_family_t af_family, const void *dst, scope = RT_SCOPE_LINK; } - return sitnl_route_set(RTM_NEWROUTE, NLM_F_CREATE | NLM_F_REPLACE, ifindex, + return sitnl_route_set(RTM_NEWROUTE, NLM_F_CREATE, ifindex, af_family, dst, prefixlen, gw, table, metric, scope, RTPROT_BOOT, RTN_UNICAST); } diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 61a40ff1..f1257b00 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1599,8 +1599,14 @@ add_route(struct route_ipv4 *r, } status = RTA_SUCCESS; - if (net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), - &r->gateway, iface, 0, metric) < 0) + int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), + &r->gateway, iface, 0, metric); + if (ret == -EEXIST) + { + msg(D_ROUTE, "NOTE: Linux route add command failed because route exists"); + status = RTA_EEXIST; + } + else if (ret < 0) { msg(M_WARN, "ERROR: Linux route add command failed"); status = RTA_ERROR; @@ -1963,11 +1969,17 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, } status = RTA_SUCCESS; - if (net_route_v6_add(ctx, &r6->network, r6->netbits, - gateway_needed ? &r6->gateway : NULL, device, 0, - metric) < 0) + int ret = net_route_v6_add(ctx, &r6->network, r6->netbits, + gateway_needed ? &r6->gateway : NULL, + device, 0, metric); + if (ret == -EEXIST) { - msg(M_WARN, "ERROR: Linux IPv6 route can't be added"); + msg(D_ROUTE, "NOTE: Linux route add command failed because route exists"); + status = RTA_EEXIST; + } + else if (ret < 0) + { + msg(M_WARN, "ERROR: Linux route add command failed"); status = RTA_ERROR; }
The code in sitnl_route_set() used to treat "route can not be installed because it already exists" (EEXIST) as "not an error". This is arguably a reasonable approach, but needs to handled higher up - if the low level add_route() function say "no error", we will try to remove that route later on in delete_route(), possibly removing someone else's "already existing" route then. So: - remove special case in sitnl_route_set() - do not pass NLM_F_REPLACE flag to sitnl_route_set() call - this would cause netlink to just replace existing routes, never return EEXIST (see "man netlink(7)") - add detailed return code handling to add_route(), assign "2" on "-EEXIST" (and log appropriate message). (Note: sitnl_route_set() is a common function for sitnl route add and delete, but EEXIST can not happen on delete - so this change has no impact for the "delete" case) v2: use RTA_ macros, also adjust add_route_ipv6() Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/networking_sitnl.c | 6 +----- src/openvpn/route.c | 24 ++++++++++++++++++------ 2 files changed, 19 insertions(+), 11 deletions(-)