Hi,
Netlink is antonio's realm, but fwiw, I gave it a whirl:
On Wed, Jan 11, 2023 at 11:38 AM Gert Doering <g...@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 <g...@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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
