[ovs-dev] userspace: Enable non-bridge port as tunnel endpoint.
diff mbox series

Message ID 1563299708-17693-1-git-send-email-pkusunyifeng@gmail.com
State New
Headers show
Series
  • [ovs-dev] userspace: Enable non-bridge port as tunnel endpoint.
Related show

Commit Message

Yifeng Sun July 16, 2019, 5:55 p.m. UTC
For userspace datapath, currently only the bridge itself, the LOCAL port,
can be the tunnel endpoint to encap/decap tunnel packets.  This patch
enables non-bridge port as tunnel endpoint.  One use case is for users to
create a bridge and a vtep port as tap, and configure underlay IP at vtep
port as the tunnel endpoint.

Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: William Tu <u9012063@gmail.com>
---
 ofproto/ofproto-dpif-xlate.c | 56 ++++++++++++++++++++++++++++++++++++++------
 tests/tunnel-push-pop.at     | 55 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 7 deletions(-)

Comments

0-day Robot July 16, 2019, 6:59 p.m. UTC | #1
Bleep bloop.  Greetings Yifeng Sun, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: William Tu <u9012063@gmail.com>
Lines checked: 184, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Ben Pfaff July 16, 2019, 9:13 p.m. UTC | #2
On Tue, Jul 16, 2019 at 02:59:55PM -0400, 0-day Robot wrote:
> checkpatch:
> WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: William Tu <u9012063@gmail.com>
> Lines checked: 184, Warnings: 1, Errors: 0

I agree with the robot in this case.  Is William a co-author?
William Tu July 16, 2019, 9:32 p.m. UTC | #3
On Tue, Jul 16, 2019 at 2:13 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Tue, Jul 16, 2019 at 02:59:55PM -0400, 0-day Robot wrote:
> > checkpatch:
> > WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: William Tu <u9012063@gmail.com>
> > Lines checked: 184, Warnings: 1, Errors: 0
>
> I agree with the robot in this case.  Is William a co-author?
>
Yes, thanks.
Co-authored-by: William Tu <u9012063@gmail.com>
Ben Pfaff July 16, 2019, 10:04 p.m. UTC | #4
On Tue, Jul 16, 2019 at 10:55:08AM -0700, Yifeng Sun wrote:
> For userspace datapath, currently only the bridge itself, the LOCAL port,
> can be the tunnel endpoint to encap/decap tunnel packets.  This patch
> enables non-bridge port as tunnel endpoint.  One use case is for users to
> create a bridge and a vtep port as tap, and configure underlay IP at vtep
> port as the tunnel endpoint.
> 
> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> Signed-off-by: William Tu <u9012063@gmail.com>

Thanks for working on this!

I think that there might be a conceptual error in
is_neighbor_reply_correct() regarding IPv6.  This code adds an "if"
statement that tests for ARP, but nested inside it is code for matching
an IPv6 address.  This seems wrong.  Is it a mistake, or is there
something deeper going on here?

Thanks,

Ben.
Yifeng Sun July 16, 2019, 10:44 p.m. UTC | #5
Thanks Ben for pointing this out. There surely is a mistake that we need to fix.
We will come up a new version.

Thanks,
Yifeng

On Tue, Jul 16, 2019 at 3:04 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Tue, Jul 16, 2019 at 10:55:08AM -0700, Yifeng Sun wrote:
> > For userspace datapath, currently only the bridge itself, the LOCAL port,
> > can be the tunnel endpoint to encap/decap tunnel packets.  This patch
> > enables non-bridge port as tunnel endpoint.  One use case is for users to
> > create a bridge and a vtep port as tap, and configure underlay IP at vtep
> > port as the tunnel endpoint.
> >
> > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > Signed-off-by: William Tu <u9012063@gmail.com>
>
> Thanks for working on this!
>
> I think that there might be a conceptual error in
> is_neighbor_reply_correct() regarding IPv6.  This code adds an "if"
> statement that tests for ARP, but nested inside it is code for matching
> an IPv6 address.  This seems wrong.  Is it a mistake, or is there
> something deeper going on here?
>
> Thanks,
>
> Ben.

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 73966a4e83ca..6fd9539c894b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3403,6 +3403,19 @@  tnl_route_lookup_flow(const struct xlate_ctx *ctx,
             }
         }
     }
+
+    /* If tunnel IP isn't configured on bridges, then we search all ports. */
+    HMAP_FOR_EACH (xbridge, hmap_node, &ctx->xcfg->xbridges) {
+        struct xport *port;
+
+        HMAP_FOR_EACH (port, ofp_node, &xbridge->xports) {
+            if (!strncmp(netdev_get_name(port->netdev),
+                         out_dev, IFNAMSIZ)) {
+                *out_port = port;
+                return 0;
+            }
+        }
+    }
     return -ENOENT;
 }
 
@@ -3975,10 +3988,11 @@  is_neighbor_reply_correct(const struct xlate_ctx *ctx, const struct flow *flow)
     bool ret = false;
     int i;
     struct xbridge_addr *xbridge_addr = xbridge_addr_ref(ctx->xbridge->addr);
+    struct in6_addr *ip_addr, *mask;
 
     /* Verify if 'nw_dst' of ARP or 'ipv6_dst' of ICMPV6 is in the list. */
     for (i = 0; xbridge_addr && i < xbridge_addr->n_addr; i++) {
-        struct in6_addr *ip_addr = &xbridge_addr->addr[i];
+        ip_addr = &xbridge_addr->addr[i];
         if ((IN6_IS_ADDR_V4MAPPED(ip_addr) &&
              flow->dl_type == htons(ETH_TYPE_ARP) &&
              in6_addr_get_mapped_ipv4(ip_addr) == flow->nw_dst) ||
@@ -3991,20 +4005,48 @@  is_neighbor_reply_correct(const struct xlate_ctx *ctx, const struct flow *flow)
     }
 
     xbridge_addr_unref(xbridge_addr);
+
+    /* If not found in bridge's IPs, search in its ports. */
+    if (flow->dl_type == htons(ETH_TYPE_ARP) && !ret) {
+        struct xport *port;
+        int error, n_in6;
+        HMAP_FOR_EACH (port, ofp_node, &ctx->xbridge->xports) {
+            error = netdev_get_addr_list(port->netdev, &ip_addr,
+                                         &mask, &n_in6);
+            if (error) {
+                continue;
+            }
+            if (IN6_IS_ADDR_V4MAPPED(ip_addr)) {
+                ovs_be32 ip4_mask = in6_addr_get_mapped_ipv4(mask);
+                if ((flow->nw_dst & ip4_mask) ==
+                    (in6_addr_get_mapped_ipv4(ip_addr) & ip4_mask)) {
+                    ret = true;
+                    break;
+                }
+            } else {
+                struct in6_addr masked_dst, masked_addr;
+                masked_dst = ipv6_addr_bitand(&flow->ipv6_dst, mask);
+                masked_addr = ipv6_addr_bitand(ip_addr, mask);
+                if (ipv6_addr_equals(&masked_dst, &masked_addr)) {
+                    ret = true;
+                    break;
+                }
+            }
+        }
+
+    }
     return ret;
 }
 
 static bool
-terminate_native_tunnel(struct xlate_ctx *ctx, ofp_port_t ofp_port,
-                        struct flow *flow, struct flow_wildcards *wc,
-                        odp_port_t *tnl_port)
+terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
+                        struct flow_wildcards *wc, odp_port_t *tnl_port)
 {
     *tnl_port = ODPP_NONE;
 
     /* XXX: Write better Filter for tunnel port. We can use in_port
      * in tunnel-port flow to avoid these checks completely. */
-    if (ofp_port == OFPP_LOCAL &&
-        ovs_native_tunneling_is_on(ctx->xbridge->ofproto)) {
+    if (ovs_native_tunneling_is_on(ctx->xbridge->ofproto)) {
         *tnl_port = tnl_port_map_lookup(flow, wc);
 
         /* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
@@ -4148,7 +4190,7 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
             native_tunnel_output(ctx, xport, flow, odp_port, truncate);
             flow->tunnel = flow_tnl; /* Restore tunnel metadata */
 
-        } else if (terminate_native_tunnel(ctx, ofp_port, flow, wc,
+        } else if (terminate_native_tunnel(ctx, flow, wc,
                                            &odp_tnl_port)) {
             /* Intercept packet to be received on native tunnel port. */
             nl_msg_put_odp_port(ctx->odp_actions, OVS_ACTION_ATTR_TUNNEL_POP,
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index f7172433ee63..5056b971256a 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -601,3 +601,58 @@  NXST_FLOW reply:
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([tunnel_push_pop - use non-local port as tunnel endpoint])
+
+OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
+AT_CHECK([ovs-vsctl add-port br0 vtep0 -- set int vtep0 type=dummy], [0])
+AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy], [0])
+AT_CHECK([ovs-vsctl add-port int-br t1 -- set Interface t1 type=gre \
+                    options:remote_ip=1.1.2.92 ofport_request=3], [0])
+
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
+dummy@ovs-dummy: hit:0 missed:0
+  br0:
+    br0 65534/100: (dummy-internal)
+    p0 1/1: (dummy)
+    vtep0 2/2: (dummy)
+  int-br:
+    int-br 65534/3: (dummy-internal)
+    t1 3/4: (gre: remote_ip=1.1.2.92)
+])
+
+AT_CHECK([ovs-appctl netdev-dummy/ip4addr vtep0 1.1.2.88/24], [0], [OK
+])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 vtep0], [0], [OK
+])
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+AT_CHECK([ovs-ofctl add-flow int-br action=normal])
+
+dnl Use arp request and reply to achieve tunnel next hop mac binding
+dnl By default, vtep0's MAC address is aa:55:aa:55:00:03
+AT_CHECK([ovs-appctl netdev-dummy/receive vtep0 'recirc_id(0),in_port(2),eth(dst=ff:ff:ff:ff:ff:ff,src=aa:55:aa:55:00:03),eth_type(0x0806),arp(tip=1.1.2.92,sip=1.1.2.88,op=1,sha=aa:55:aa:55:00:03,tha=00:00:00:00:00:00)'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:03),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=aa:55:aa:55:00:03)'])
+
+AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl
+1.1.2.92                                      f8:bc:12:44:34:b6   br0
+])
+
+AT_CHECK([ovs-appctl ovs/route/show | tail -n+2 | sort], [0], [dnl
+User: 1.1.2.0/24 dev vtep0 SRC 1.1.2.88
+])
+
+dnl Check GRE tunnel pop
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:03),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout])
+
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: tnl_pop(4)
+])
+
+dnl Check GRE tunnel push
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(3),eth(dst=f9:bc:12:44:34:b6,src=af:55:aa:55:00:03),eth_type(0x0800),ipv4(src=1.1.3.88,dst=1.1.3.92,proto=1,tos=0,ttl=64,frag=no)'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: clone(tnl_push(tnl_port(4),header(size=38,type=3,eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:03,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x6558))),out_port(2)),1)
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP