| 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 | expand |
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 > { >
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 {
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(-)