diff mbox series

[Openvpn-devel,v2] Repair special-casing of EEXIST for Linux/SITNL route install

Message ID 20230118074633.27586-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel,v2] Repair special-casing of EEXIST for Linux/SITNL route install |

Commit Message

Gert Doering Jan. 18, 2023, 7:46 a.m. UTC 
  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(-)

Comments

Selva Nair Jan. 18, 2023, 6:14 p.m. UTC | #1 
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>
Gert Doering Jan. 19, 2023, 7:37 a.m. UTC | #2 
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 mbox series

Patch

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;
     }