| Message ID | 20230111160848.22906-1-gert@greenie.muc.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Repair special-casing of EEXIST for Linux/SITNL route install | expand |
Hi, Netlink is antonio's realm, but fwiw, I gave it a whirl: On Wed, Jan 11, 2023 at 11:38 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 route_add(), 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) > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/networking_sitnl.c | 6 +----- > src/openvpn/route.c | 10 ++++++++-- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/src/openvpn/networking_sitnl.c > b/src/openvpn/networking_sitnl.c > index fe124616..92f30044 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 99f948ba..2db127bb 100644 > --- a/src/openvpn/route.c > +++ b/src/openvpn/route.c > @@ -1594,8 +1594,14 @@ add_route(struct route_ipv4 *r, > } > > status = 1; > - 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 = 2; > + } > + else if (ret < 0) > { > msg(M_WARN, "ERROR: Linux route add command failed"); > status = 0; > Looks good and works as expected. Got CONNECTED,ROUTE_ERROR on real errors, not if the route exists. Also, an existing route no longer gets removed. net_route_v4_add is also used in dco.c but there the return value is not checked and should not be affected by the change as far as I can tell.. A couple of things: - call to net_route_v6_add() in route_add_ipv6() also could benefit from the same treatment - net_route_v?_add() using iproute2 always returns success (0). We can't distinguish between "exists" and other errors in that case, but still it should not return unconditional success. BTW, the same functions in networking_freebsd.c returns 0 on success, +1 on error, not < 0. Not used in route.c and return value not checked in dco.c so could be cleaned up in 2.7. I notice Antonio prefers -ve for error, but that doesn't always work in cross-platform code. On Windows, some error codes are > 2^31. One thing to note: when a route exists here we log with D_ROUTE and real errors with M_WARN. While this should be preferred, we do not do the same in other route methods like IPAPI or service where such a distinction is possible. Cases where existing route cannot be reliably detected, we have no option. Making all route methods log consistently could be a part of the planned re-write/fixup in 2.7. Selva
Hi, for the netlink/sitnl bits: this makes sense to me. I agree with Selva that the v6 variant could benefit from the same treatment. However, this patch can also be hacked on its own Acked-by: Antonio Quartulli <a@unstable.cc> On 11/01/2023 17:08, Gert Doering 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 route_add(), 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) > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/networking_sitnl.c | 6 +----- > src/openvpn/route.c | 10 ++++++++-- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c > index fe124616..92f30044 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 99f948ba..2db127bb 100644 > --- a/src/openvpn/route.c > +++ b/src/openvpn/route.c > @@ -1594,8 +1594,14 @@ add_route(struct route_ipv4 *r, > } > > status = 1; > - 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 = 2; > + } > + else if (ret < 0) > { > msg(M_WARN, "ERROR: Linux route add command failed"); > status = 0;
On Wed, Jan 11, 2023 at 7:30 PM Antonio Quartulli <a@unstable.cc> wrote: > Hi, > > for the netlink/sitnl bits: this makes sense to me. > I agree with Selva that the v6 variant could benefit from the same > treatment. > > However, this patch can also be hacked on its own > > Acked-by: Antonio Quartulli <a@unstable.cc> > Not so fast. As it stands the patch introduces a bug that was not there: existing v6 routes can cause CONENCTED,ROUTE_ERROR instead of CONNECTED,SUCCESS. That's why I did not ACK it. Not sure I understand "can also be hacked alone". Selva
Hi,
On Thu, Jan 12, 2023 at 02:50:27AM -0500, Selva Nair wrote:
> Not sure I understand "can also be hacked alone".
I think that was intended to be "acked" alone :-)
Anyway, thanks for spotting this, and shame on me for only testing v4
("the v6 stuff is new and shiny and has none of these problems") - but
of course, the v6 netlink stuff was written together with the v4 netlink
stuff, and shares traits.
I'll send a v2 with v6 fixed and tested.
gert
On Wed, Jan 11, 2023 at 11:38 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. > Found one more case of route addition flag not correctly recorded -- RL_DID_LOCAL used for the direct route to the server. I will wait for the final shape and form of this one before sending a patch. Selva
diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index fe124616..92f30044 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 99f948ba..2db127bb 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1594,8 +1594,14 @@ add_route(struct route_ipv4 *r, } status = 1; - 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 = 2; + } + else if (ret < 0) { msg(M_WARN, "ERROR: Linux route add command failed"); status = 0;
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 route_add(), 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) Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/networking_sitnl.c | 6 +----- src/openvpn/route.c | 10 ++++++++-- 2 files changed, 9 insertions(+), 7 deletions(-)