[Openvpn-devel,1/2] Fix IPv4 default gateway with multiple route tables

Message ID 20210413122006.3960-1-themiron@yandex-team.ru
State Changes Requested
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel,1/2] Fix IPv4 default gateway with multiple route tables
Related show

Commit Message

Vladislav Grishenko April 13, 2021, 12:20 p.m.
Current default gateway selection for zero destignation address just
dumps and parses all the routing tables. If any of non-main table
with default route comes first, wrong default gateway can be picked.
Since adding/removing routes currently handles only main table,
let's stick to RT_TABLE_MAIN while selecting default route too.

Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
---
 src/openvpn/networking_sitnl.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli April 14, 2021, 9:23 p.m. | #1
Hi,

On 13/04/2021 14:20, Vladislav Grishenko wrote:
> Current default gateway selection for zero destignation address just

little typ0: destiGnation

> dumps and parses all the routing tables. If any of non-main table
> with default route comes first, wrong default gateway can be picked.
> Since adding/removing routes currently handles only main table,
> let's stick to RT_TABLE_MAIN while selecting default route too.
> 
> Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
> ---
>  src/openvpn/networking_sitnl.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
> index 2bc70a50..56543648 100644
> --- a/src/openvpn/networking_sitnl.c
> +++ b/src/openvpn/networking_sitnl.c
> @@ -426,6 +426,7 @@ typedef struct {
>      inet_address_t gw;
>      char iface[IFNAMSIZ];
>      bool default_only;
> +    unsigned int table;
>  } route_res_t;
>  
>  static int
> @@ -435,7 +436,7 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
>      struct rtmsg *r = NLMSG_DATA(n);
>      struct rtattr *rta = RTM_RTA(r);
>      int len = n->nlmsg_len - NLMSG_LENGTH(sizeof(*r));
> -    unsigned int ifindex = 0;
> +    unsigned int table, ifindex = 0;
>  
>      /* filter-out non-zero dst prefixes */
>      if (res->default_only && r->rtm_dst_len != 0)
> @@ -443,6 +444,9 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
>          return 1;
>      }
>  
> +    /* route table, ignored with RTA_TABLE */
> +    table = r->rtm_table;
> +
>      while (RTA_OK(rta, len))
>      {
>          switch (rta->rta_type)
> @@ -460,11 +464,22 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
>              case RTA_GATEWAY:
>                  memcpy(&res->gw, RTA_DATA(rta), res->addr_size);
>                  break;
> +
> +            /* route table */
> +            case RTA_TABLE:
> +                table = *(unsigned int *)RTA_DATA(rta);
> +                break;
>          }
>  
>          rta = RTA_NEXT(rta, len);
>      }
>  
> +    /* filter-out zero dns prefixes from other tables */

This comments seems off?
I'd suggest something like:

/* filter out any route not coming from the selected table */

> +    if (res->table && res->table != table)
> +    {
> +        return 1;

Am I wrong or at this point we have already copied the GW to res->gw
thus, the caller will believe we have a result?

IMHO we should save the GW pointer in a temporary variable and copy it
only after this check has passed.

As a side notw, could you please resend this patch based on release/2.5?
We should start by fixing the bug there and then porting this patch to
master.

Regards,

> +    }
> +
>      if (!if_indextoname(ifindex, res->iface))
>      {
>          msg(M_WARN | M_ERRNO, "%s: rtnl: can't get ifname for index %d",
> @@ -507,6 +522,7 @@ sitnl_route_best_gw(sa_family_t af_family, const inet_address_t *dst,
>              {
>                  req.n.nlmsg_flags |= NLM_F_DUMP;
>                  res.default_only = true;
> +                res.table = RT_TABLE_MAIN;
>              }
>              else
>              {
>

Patch

diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
index 2bc70a50..56543648 100644
--- a/src/openvpn/networking_sitnl.c
+++ b/src/openvpn/networking_sitnl.c
@@ -426,6 +426,7 @@  typedef struct {
     inet_address_t gw;
     char iface[IFNAMSIZ];
     bool default_only;
+    unsigned int table;
 } route_res_t;
 
 static int
@@ -435,7 +436,7 @@  sitnl_route_save(struct nlmsghdr *n, void *arg)
     struct rtmsg *r = NLMSG_DATA(n);
     struct rtattr *rta = RTM_RTA(r);
     int len = n->nlmsg_len - NLMSG_LENGTH(sizeof(*r));
-    unsigned int ifindex = 0;
+    unsigned int table, ifindex = 0;
 
     /* filter-out non-zero dst prefixes */
     if (res->default_only && r->rtm_dst_len != 0)
@@ -443,6 +444,9 @@  sitnl_route_save(struct nlmsghdr *n, void *arg)
         return 1;
     }
 
+    /* route table, ignored with RTA_TABLE */
+    table = r->rtm_table;
+
     while (RTA_OK(rta, len))
     {
         switch (rta->rta_type)
@@ -460,11 +464,22 @@  sitnl_route_save(struct nlmsghdr *n, void *arg)
             case RTA_GATEWAY:
                 memcpy(&res->gw, RTA_DATA(rta), res->addr_size);
                 break;
+
+            /* route table */
+            case RTA_TABLE:
+                table = *(unsigned int *)RTA_DATA(rta);
+                break;
         }
 
         rta = RTA_NEXT(rta, len);
     }
 
+    /* filter-out zero dns prefixes from other tables */
+    if (res->table && res->table != table)
+    {
+        return 1;
+    }
+
     if (!if_indextoname(ifindex, res->iface))
     {
         msg(M_WARN | M_ERRNO, "%s: rtnl: can't get ifname for index %d",
@@ -507,6 +522,7 @@  sitnl_route_best_gw(sa_family_t af_family, const inet_address_t *dst,
             {
                 req.n.nlmsg_flags |= NLM_F_DUMP;
                 res.default_only = true;
+                res.table = RT_TABLE_MAIN;
             }
             else
             {