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

  Current default gateway selection for zero destination 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.

v2: keep gateway address unchanged on lookup error
v3: reduce ammout of gateway address copying

Reported-by: Donald Sharp <donaldsharp72@gmail.com>
Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
---
 src/openvpn/networking_sitnl.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
  

Hi,

On 16/04/2021 14:07, Vladislav Grishenko wrote:
> Current default gateway selection for zero destination 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.
> 
> v2: keep gateway address unchanged on lookup error
> v3: reduce ammout of gateway address copying
> 
> Reported-by: Donald Sharp <donaldsharp72@gmail.com>
> Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
> ---
>  src/openvpn/networking_sitnl.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
> index 2bc70a50..ea1621ed 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,8 @@ 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;
> +    void *gw = NULL;
>  
>      /* filter-out non-zero dst prefixes */
>      if (res->default_only && r->rtm_dst_len != 0)
> @@ -443,6 +445,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)
> @@ -458,13 +463,24 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
>  
>              /* GW for the route */
>              case RTA_GATEWAY:
> -                memcpy(&res->gw, RTA_DATA(rta), res->addr_size);
> +                gw = RTA_DATA(rta);
> +                break;
> +
> +            /* route table */
> +            case RTA_TABLE:
> +                table = *(unsigned int *)RTA_DATA(rta);
>                  break;
>          }
>  
>          rta = RTA_NEXT(rta, len);
>      }
>  
> +    /* filter out any route not coming from the selected table */
> +    if (res->table && res->table != table)

Today I was wondering if 0 could be a meaningful table ID, since this
code uses 0 as "no table was selected for filtering" (thus creating a
false negative).

After checking rtnetlink.h, I could see that 0 is indeed the UNSPEC
value (RT_TABLE_UNSPEC=0), therefore your patch is correct.

However, to prevent the next casual reader from asking the same
question, wouldn't it make sense to change this comparison with:

if (res->table != RT_TABLE_UNSPEC && 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",
> @@ -472,6 +488,11 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
>          return -1;
>      }
>  
> +    if (gw)
> +    {
> +        memcpy(&res->gw, gw, res->addr_size);
> +    }
> +
>      return 0;
>  }
>  
> @@ -507,6 +528,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;

Another question: why restricting the research to the MAIN table only
for IPv4 and only when searching a default route?

Like you said in the commit message: OpenVPN only operates on the MAIN
routing table for the time being, therefore, shouldn't we just set
"res.table = RT_TABLE_MAIN" by default *all the time* for now?

Otherwise we are fixing this problem only for the ipv4/default search,
but the function remains "buggy" for the other cases.


>              }
>              else
>              {
> 


Regards,
  

Hi,

> However, to prevent the next casual reader from asking the same question,
> wouldn't it make sense to change this comparison with:
> 
> if (res->table != RT_TABLE_UNSPEC && res->table != table)

I don’t think it's necessary for following reasons:
1. Readability. "if (res->table)" reads as "if filter table is set ". Changing it to "if (res->table != RT_TABLE_UNSPEC)" gives the same reading imho.
2. RT_TABLE_UNSPEC was and will be always zero, because zillion of software incl. iproute2 depends on zeroed structs with 0 == UNSPEC
3. Comparing with RT_TABLE_UNSPEC here will require additional code to explicitely init res.table with RT_TABLE_UNSPEC, which in fact is will be noop after CLEAR(res) and have little sense, since there's no chance to have RT_TABLE_UNSPEC != 0.
Anyway, if you still thinks it's required, I can do change.

> Another question: why restricting the research to the MAIN table only for IPv4
> and only when searching a default route?

For real dst, incl ::/128 we do the right thing - perform "ip route get" that will get real gateway for specified dst no matter what table or anything else is.
And we performs "ip route show" dump only to get gateway for "0.0.0.0" destination by a reason.
Getting default gateway for "0.0.0.0/0" destination is not logically possible at all, for example what is default gateway for this case:
	0.0.0.0/1 via a.a.a.a
	128.0.0.0/1 via b.b.b.b
Therefore for 0.0.0.0/0 case heuristic is used, if there's  a default route, if it's in main table and so on, that’s why only dumping needs it.
Any other - don't.
 
> Otherwise we are fixing this problem only for the ipv4/default search, but the
> function remains "buggy" for the other cases.

Buggy here is searching default gateway for 0.0.0.0/0 itself. Other cases are right from the scratch :)

--
Best Regards, Vladislav Grishenko

> -----Original Message-----
> From: Antonio Quartulli <a@unstable.cc>
> Sent: Friday, April 16, 2021 7:01 PM
> To: Vladislav Grishenko <themiron@yandex-team.ru>; openvpn-
> devel@lists.sourceforge.net
> Subject: Re: [Openvpn-devel] [PATCH v3 1/2] Fix IPv4 default gateway with
> multiple route tables
> 
> Hi,
> 
> On 16/04/2021 14:07, Vladislav Grishenko wrote:
> > Current default gateway selection for zero destination 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.
> >
> > v2: keep gateway address unchanged on lookup error
> > v3: reduce ammout of gateway address copying
> >
> > Reported-by: Donald Sharp <donaldsharp72@gmail.com>
> > Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
> > ---
> >  src/openvpn/networking_sitnl.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/openvpn/networking_sitnl.c
> > b/src/openvpn/networking_sitnl.c index 2bc70a50..ea1621ed 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,8 @@ 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;
> > +    void *gw = NULL;
> >
> >      /* filter-out non-zero dst prefixes */
> >      if (res->default_only && r->rtm_dst_len != 0) @@ -443,6 +445,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)
> > @@ -458,13 +463,24 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
> >
> >              /* GW for the route */
> >              case RTA_GATEWAY:
> > -                memcpy(&res->gw, RTA_DATA(rta), res->addr_size);
> > +                gw = RTA_DATA(rta);
> > +                break;
> > +
> > +            /* route table */
> > +            case RTA_TABLE:
> > +                table = *(unsigned int *)RTA_DATA(rta);
> >                  break;
> >          }
> >
> >          rta = RTA_NEXT(rta, len);
> >      }
> >
> > +    /* filter out any route not coming from the selected table */
> > +    if (res->table && res->table != table)
> 
> Today I was wondering if 0 could be a meaningful table ID, since this code uses 0
> as "no table was selected for filtering" (thus creating a false negative).
> 
> After checking rtnetlink.h, I could see that 0 is indeed the UNSPEC value
> (RT_TABLE_UNSPEC=0), therefore your patch is correct.
> 
> However, to prevent the next casual reader from asking the same question,
> wouldn't it make sense to change this comparison with:
> 
> if (res->table != RT_TABLE_UNSPEC && 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", @@ -472,6 +488,11 @@ sitnl_route_save(struct nlmsghdr *n, void *arg)
> >          return -1;
> >      }
> >
> > +    if (gw)
> > +    {
> > +        memcpy(&res->gw, gw, res->addr_size);
> > +    }
> > +
> >      return 0;
> >  }
> >
> > @@ -507,6 +528,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;
> 
> Another question: why restricting the research to the MAIN table only for IPv4
> and only when searching a default route?
> 
> Like you said in the commit message: OpenVPN only operates on the MAIN
> routing table for the time being, therefore, shouldn't we just set "res.table =
> RT_TABLE_MAIN" by default *all the time* for now?
> 
> Otherwise we are fixing this problem only for the ipv4/default search, but the
> function remains "buggy" for the other cases.
> 
> 
> >              }
> >              else
> >              {
> >
> 
> 
> Regards,
> 
> --
> Antonio Quartulli
  

Hi,

On 16/04/2021 16:43, Vladislav Grishenko wrote:
> Hi,
> 
>> However, to prevent the next casual reader from asking the same question,
>> wouldn't it make sense to change this comparison with:
>>
>> if (res->table != RT_TABLE_UNSPEC && res->table != table)
> 
> I don’t think it's necessary for following reasons:
> 1. Readability. "if (res->table)" reads as "if filter table is set ". Changing it to "if (res->table != RT_TABLE_UNSPEC)" gives the same reading imho.
> 2. RT_TABLE_UNSPEC was and will be always zero, because zillion of software incl. iproute2 depends on zeroed structs with 0 == UNSPEC
> 3. Comparing with RT_TABLE_UNSPEC here will require additional code to explicitely init res.table with RT_TABLE_UNSPEC, which in fact is will be noop after CLEAR(res) and have little sense, since there's no chance to have RT_TABLE_UNSPEC != 0.
> Anyway, if you still thinks it's required, I can do change.

No, it's ok. You are right that 0 is just assumed to be the UNSPEC
filter (i.e. don't filter). Let's keep it as it is.

> 
>> Another question: why restricting the research to the MAIN table only for IPv4
>> and only when searching a default route?
> 
> For real dst, incl ::/128 we do the right thing - perform "ip route get" that will get real gateway for specified dst no matter what table or anything else is.
> And we performs "ip route show" dump only to get gateway for "0.0.0.0" destination by a reason.
> Getting default gateway for "0.0.0.0/0" destination is not logically possible at all, for example what is default gateway for this case:
> 	0.0.0.0/1 via a.a.a.a
> 	128.0.0.0/1 via b.b.b.b
> Therefore for 0.0.0.0/0 case heuristic is used, if there's  a default route, if it's in main table and so on, that’s why only dumping needs it.
> Any other - don't.
>  
>> Otherwise we are fixing this problem only for the ipv4/default search, but the
>> function remains "buggy" for the other cases.
> 
> Buggy here is searching default gateway for 0.0.0.0/0 itself. Other cases are right from the scratch :)
> 

Right. The problem I was seeing is simply that we are not providing all
info to the kernel as we should.
This requires further improvement later, but not as part of this fix :-)

So the patch is fine as it is, thanks!

Acked-by: Antonio Quartulli <antonio@openvpn.net>

@Gert: this should go to 2.5 too. Thanks!


Regards,
  

Code looks reasonable (though I won't claim to understand the nuances
of Netlink :-) ) - took me a bit to remember what is done here with
the callback function for matching, but yeah, seems to do what it says
on the lid.  Did some very basic testing on a system with just a single
route table and nothing breaks :-)

Your patch has been applied to the master and release/2.5 branch (bugfix).

commit c7f95891a4a0aabb64e7d4f3200525c1a2fcf433 (master)
commit 48ba0b059138dec7a94d2368375344d39c4e05e9 (release/2.5)
Author: Vladislav Grishenko
Date:   Fri Apr 16 17:07:07 2021 +0500

     Fix IPv4 default gateway with multiple route tables

     Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210416120708.1532-1-themiron@yandex-team.ru>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22130.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering