diff mbox series

[ovs-dev] ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses

Message ID 1523864157-23094-1-git-send-email-wenxu@ucloud.cn
State Changes Requested
Headers show
Series [ovs-dev] ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses | expand

Commit Message

wenxu April 16, 2018, 7:35 a.m. UTC 
From: wenxu <wenxu@ucloud.cn>

It makes OVS native tunneling honor tunnel-specified source addresses,
in the same way that Linux kernel tunneling honors them.

This patch made valid tun_src specified by flow-action can be used for
tunnel_src of packet. add a "local" property for a route entry.
Like the kernel space when lookup the route, if there are tun_src specified
by flow-action or port options. Check the tun_src wheather is a local
address, then lookup the route.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 lib/ovs-router.c             | 40 +++++++++++++++++++++++++++++++++-------
 lib/ovs-router.h             |  2 +-
 lib/route-table.c            | 11 ++++++++++-
 ofproto/ofproto-dpif-sflow.c |  2 +-
 ofproto/ofproto-dpif-xlate.c |  4 ++++
 5 files changed, 49 insertions(+), 10 deletions(-)

Comments

Ben Pfaff April 16, 2018, 8:40 p.m. UTC | #1 
On Mon, Apr 16, 2018 at 03:35:57PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> It makes OVS native tunneling honor tunnel-specified source addresses,
> in the same way that Linux kernel tunneling honors them.
> 
> This patch made valid tun_src specified by flow-action can be used for
> tunnel_src of packet. add a "local" property for a route entry.
> Like the kernel space when lookup the route, if there are tun_src specified
> by flow-action or port options. Check the tun_src wheather is a local
> address, then lookup the route.
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>

Thanks for v2.  It resolves the portability issue.  I hope I may ask
some more questions, because there are some pieces that I do not
understand yet.

I think that there is a redundant assignment to change->rd.rtm_dst_len
in route_table_parse().  I think that the first assignment here may be
removed:

        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
        if (rtm->rtm_type == RTN_LOCAL) {
            dst_len_off += 64;
            change->rd.local = true;
        } else {
            change->rd.local = false;
        }
        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);

I do not understand dst_len_off yet.  It is normally 96, but for local
addresses it increases by 64, to 160.  Then, only for IPv4, it gets
added to the prefix length.  I don't understand how a prefix length
greater than 128 is to be interpreted, since an IPv6 address is only 128
bits long.  (Without the addition of 64, I understand: it is because
32-bit IPv4 addresses are mapped into 128-bit IPv6 addresses and 96 =
128 - 32.)  rd.rtm_dst_len eventually gets passed to ovs_router_insert()
as plen, then to rt_init_match() as plen, and then that is used to
create a IPv6 address mask, so it seems like the value should be 128 or
less, not 160 or more.

Thanks,

Ben.
wenxu April 17, 2018, 2:54 a.m. UTC | #2 
At 2018-04-17 04:40:26, "Ben Pfaff" <blp@ovn.org> wrote:
>On Mon, Apr 16, 2018 at 03:35:57PM +0800, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>> 
>> It makes OVS native tunneling honor tunnel-specified source addresses,
>> in the same way that Linux kernel tunneling honors them.
>> 
>> This patch made valid tun_src specified by flow-action can be used for
>> tunnel_src of packet. add a "local" property for a route entry.
>> Like the kernel space when lookup the route, if there are tun_src specified
>> by flow-action or port options. Check the tun_src wheather is a local
>> address, then lookup the route.
>> 
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>
>Thanks for v2.  It resolves the portability issue.  I hope I may ask
>some more questions, because there are some pieces that I do not
>understand yet.
>
>I think that there is a redundant assignment to change->rd.rtm_dst_len
>in route_table_parse().  I think that the first assignment here may be
>removed:
>
>        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
>        if (rtm->rtm_type == RTN_LOCAL) {
>            dst_len_off += 64;
>            change->rd.local = true;
>        } else {
>            change->rd.local = false;
>        }
>        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);
>
>I do not understand dst_len_off yet.  It is normally 96, but for local
>addresses it increases by 64, to 160.  Then, only for IPv4, it gets
>added to the prefix length.  I don't understand how a prefix length
>greater than 128 is to be interpreted, since an IPv6 address is only 128
>bits long.  (Without the addition of 64, I understand: it is because
>32-bit IPv4 addresses are mapped into 128-bit IPv6 addresses and 96 =
>128 - 32.)  rd.rtm_dst_len eventually gets passed to ovs_router_insert()
>as plen, then to rt_init_match() as plen, and then that is used to
>create a IPv6 address mask, so it seems like the value should be 128 or
>less, not 160 or more.

I want to improve the priority of the local route higher than userdef route
in the ovs_router_add : the priority of userdef route  is alway higheer than cached route
ovs_router_insert__(mark, plen + 32, &ip6, plen, argv[2], &gw6);


I think I should only enhance the priority of the local route is fine, but not dst_len


>Thanks,
>
>Ben.
Ben Pfaff April 17, 2018, 3:04 p.m. UTC | #3 
On Tue, Apr 17, 2018 at 10:54:29AM +0800, wenxu wrote:
> 
> 
> 
> 
> 
> 
> 
> 
> 
> At 2018-04-17 04:40:26, "Ben Pfaff" <blp@ovn.org> wrote:
> >On Mon, Apr 16, 2018 at 03:35:57PM +0800, wenxu@ucloud.cn wrote:
> >> From: wenxu <wenxu@ucloud.cn>
> >> 
> >> It makes OVS native tunneling honor tunnel-specified source addresses,
> >> in the same way that Linux kernel tunneling honors them.
> >> 
> >> This patch made valid tun_src specified by flow-action can be used for
> >> tunnel_src of packet. add a "local" property for a route entry.
> >> Like the kernel space when lookup the route, if there are tun_src specified
> >> by flow-action or port options. Check the tun_src wheather is a local
> >> address, then lookup the route.
> >> 
> >> Signed-off-by: wenxu <wenxu@ucloud.cn>
> >
> >Thanks for v2.  It resolves the portability issue.  I hope I may ask
> >some more questions, because there are some pieces that I do not
> >understand yet.
> >
> >I think that there is a redundant assignment to change->rd.rtm_dst_len
> >in route_table_parse().  I think that the first assignment here may be
> >removed:
> >
> >        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
> >        if (rtm->rtm_type == RTN_LOCAL) {
> >            dst_len_off += 64;
> >            change->rd.local = true;
> >        } else {
> >            change->rd.local = false;
> >        }
> >        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);
> >
> >I do not understand dst_len_off yet.  It is normally 96, but for local
> >addresses it increases by 64, to 160.  Then, only for IPv4, it gets
> >added to the prefix length.  I don't understand how a prefix length
> >greater than 128 is to be interpreted, since an IPv6 address is only 128
> >bits long.  (Without the addition of 64, I understand: it is because
> >32-bit IPv4 addresses are mapped into 128-bit IPv6 addresses and 96 =
> >128 - 32.)  rd.rtm_dst_len eventually gets passed to ovs_router_insert()
> >as plen, then to rt_init_match() as plen, and then that is used to
> >create a IPv6 address mask, so it seems like the value should be 128 or
> >less, not 160 or more.
> 
> I want to improve the priority of the local route higher than userdef route
> in the ovs_router_add : the priority of userdef route  is alway higheer than cached route
> ovs_router_insert__(mark, plen + 32, &ip6, plen, argv[2], &gw6);
> 
> 
> I think I should only enhance the priority of the local route is fine, but not dst_len

That sounds right to me.  Thanks!
diff mbox series

Patch

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index ad8bd62..3129359 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -66,6 +66,7 @@  struct ovs_router_entry {
     struct in6_addr src_addr;
     uint8_t plen;
     uint8_t priority;
+    bool local;
     uint32_t mark;
 };
 
@@ -110,13 +111,28 @@  ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst,
     const struct cls_rule *cr;
     struct flow flow = {.ipv6_dst = *ip6_dst, .pkt_mark = mark};
 
+    if (src && ipv6_addr_is_set(src)) {
+        const struct cls_rule *cr_src;
+        struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
+
+        cr_src = classifier_lookup(&cls, OVS_VERSION_MAX, &flow_src, NULL);
+        if (cr_src) {
+            struct ovs_router_entry *p_src = ovs_router_entry_cast(cr_src);
+            if (p_src->local != true) {
+                return false;
+            }
+        } else {
+            return false;
+        }
+    }
+
     cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL);
     if (cr) {
         struct ovs_router_entry *p = ovs_router_entry_cast(cr);
 
         ovs_strlcpy(output_bridge, p->output_bridge, IFNAMSIZ);
         *gw = p->gw;
-        if (src) {
+        if (src && !ipv6_addr_is_set(src)) {
             *src = p->src_addr;
         }
         return true;
@@ -197,7 +213,7 @@  out:
 }
 
 static int
-ovs_router_insert__(uint32_t mark, uint8_t priority,
+ovs_router_insert__(uint32_t mark, uint8_t priority, bool local,
                     const struct in6_addr *ip6_dst,
                     uint8_t plen, const char output_bridge[],
                     const struct in6_addr *gw)
@@ -217,6 +233,7 @@  ovs_router_insert__(uint32_t mark, uint8_t priority,
     p->mark = mark;
     p->nw_addr = match.flow.ipv6_dst;
     p->plen = plen;
+    p->local = local;
     p->priority = priority;
     err = get_src_addr(ip6_dst, output_bridge, &p->src_addr);
     if (err && ipv6_addr_is_set(gw)) {
@@ -249,10 +266,11 @@  ovs_router_insert__(uint32_t mark, uint8_t priority,
 
 void
 ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
-                  const char output_bridge[], const struct in6_addr *gw)
+                  bool local, const char output_bridge[], 
+                  const struct in6_addr *gw)
 {
     if (use_system_routing_table) {
-        ovs_router_insert__(mark, plen, ip_dst, plen, output_bridge, gw);
+        ovs_router_insert__(mark, plen, local, ip_dst, plen, output_bridge, gw);
     }
 }
 
@@ -360,7 +378,7 @@  ovs_router_add(struct unixctl_conn *conn, int argc,
         }
     }
 
-    err = ovs_router_insert__(mark, plen + 32, &ip6, plen, argv[2], &gw6);
+    err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], &gw6);
     if (err) {
         unixctl_command_reply_error(conn, "Error while inserting route.");
     } else {
@@ -417,7 +435,12 @@  ovs_router_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
         ipv6_format_mapped(&rt->nw_addr, &ds);
         plen = rt->plen;
         if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) {
-            plen -= 96;
+            uint8_t plen_off = 96;
+
+            if (rt->local == true) {
+                plen_off += 64;
+            }
+            plen -= plen_off;
         }
         ds_put_format(&ds, "/%"PRIu8, plen);
         if (rt->mark) {
@@ -431,6 +454,9 @@  ovs_router_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
         }
         ds_put_format(&ds, " SRC ");
         ipv6_format_mapped(&rt->src_addr, &ds);
+        if (rt->local) {
+            ds_put_format(&ds, " local");
+        }
         ds_put_format(&ds, "\n");
     }
     unixctl_command_reply(conn, ds_cstr(&ds));
@@ -441,7 +467,7 @@  static void
 ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc,
                       const char *argv[], void *aux OVS_UNUSED)
 {
-    struct in6_addr gw, src;
+    struct in6_addr gw, src = in6addr_any;
     char iface[IFNAMSIZ];
     struct in6_addr ip6;
     unsigned int plen;
diff --git a/lib/ovs-router.h b/lib/ovs-router.h
index f65d652..34ea163 100644
--- a/lib/ovs-router.h
+++ b/lib/ovs-router.h
@@ -31,7 +31,7 @@  bool ovs_router_lookup(uint32_t mark, const struct in6_addr *ip_dst,
                        struct in6_addr *src, struct in6_addr *gw);
 void ovs_router_init(void);
 void ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst,
-                       uint8_t plen,
+                       uint8_t plen, bool local,
                        const char output_bridge[], const struct in6_addr *gw);
 void ovs_router_flush(void);
 
diff --git a/lib/route-table.c b/lib/route-table.c
index 2ee45e5..5c789fd 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -47,6 +47,7 @@  VLOG_DEFINE_THIS_MODULE(route_table);
 struct route_data {
     /* Copied from struct rtmsg. */
     unsigned char rtm_dst_len;
+    bool local;
 
     /* Extracted from Netlink attributes. */
     struct in6_addr rta_dst; /* 0 if missing. */
@@ -229,6 +230,7 @@  route_table_parse(struct ofpbuf *buf, struct route_table_msg *change)
     if (parsed) {
         const struct nlmsghdr *nlmsg;
         int rta_oif;      /* Output interface index. */
+        uint8_t dst_len_off = 96;
 
         nlmsg = buf->data;
 
@@ -245,6 +247,13 @@  route_table_parse(struct ofpbuf *buf, struct route_table_msg *change)
         }
         change->nlmsg_type     = nlmsg->nlmsg_type;
         change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
+        if (rtm->rtm_type == RTN_LOCAL) {
+            dst_len_off += 64;
+            change->rd.local = true;
+        } else {
+            change->rd.local = false;
+        }
+        change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);
         if (attrs[RTA_OIF]) {
             rta_oif = nl_attr_get_u32(attrs[RTA_OIF]);
 
@@ -307,7 +316,7 @@  route_table_handle_msg(const struct route_table_msg *change)
         const struct route_data *rd = &change->rd;
 
         ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len,
-                          rd->ifname, &rd->rta_gw);
+                          rd->local, rd->ifname, &rd->rta_gw);
     }
 }
 
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 5d8c0e1..dbf2c02 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -468,7 +468,7 @@  sflow_choose_agent_address(const char *agent_device,
 
         if (inet_parse_active(target, SFL_DEFAULT_COLLECTOR_PORT, &sa.ss)
             && sa.ss.ss_family == AF_INET) {
-            struct in6_addr addr6, src, gw;
+            struct in6_addr addr6, gw, src = in6addr_any;;
 
             in6_addr_set_mapped_ipv4(&addr6, sa.sin.sin_addr.s_addr);
             /* sFlow only supports target in default routing table with
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c8baba1..4f8ffbb 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3320,6 +3320,10 @@  native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
     /* Backup flow & base_flow data. */
     memcpy(&old_base_flow, &ctx->base_flow, sizeof old_base_flow);
     memcpy(&old_flow, &ctx->xin->flow, sizeof old_flow);
+	
+    if (flow->tunnel.ip_src) {
+        in6_addr_set_mapped_ipv4(&s_ip6, flow->tunnel.ip_src);
+    }
 
     err = tnl_route_lookup_flow(ctx, flow, &d_ip6, &s_ip6, &out_dev);
     if (err) {