diff mbox series

[ovs-dev,3/3] ofproto-dpif-xlate: Fix ignoring IPv6 local_ip for native tunnels.

Message ID 20240220223547.2368878-4-i.maximets@ovn.org
State Accepted
Commit 166ee41d282c506d100bc2185d60af277121b55b
Headers show
Series Fix ignoring of IPv6 'local_ip' for native tunnels. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets Feb. 20, 2024, 10:35 p.m. UTC 
Local IP is taken into account only in case of IPv4 address, IPv6
source is not checked.  That leads to source being ignored during the
route lookup and ultimately packets encapsulated with a source IP
found during a route lookup, which is likely the wrong one.

Even worse, after encapsulation we have a difference between the
tunnel metadata that contains a correct source IP and the generated
actions that used a wrong source IP.  This means that if there are
OpenFlow rules in a bridge where packet goes after encapsulation,
we may match on rules that do not correspond to the actual packet
we have.

Add the check for IPv6 source address before the route lookup.

Tests added to check that we're actually using the configured local_ip
as a source address in the packet.  Also adding the same test for IPv4,
since apparently we don't have any tests covering this functionality
for userspace tunnels.

This issue also affects the case where source address is set via
OpenFlow, e.g. 'set_filed:2001:beef::88->tun_ipv6_src', but it's just
a different way of populating the tunnel metadata that doesn't depend
on a tunnel to be native or kernel one.  So, not adding extra tests
for this case for now.

Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses")
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052938.html
Reported-by: Derrick Lim <derrick.lim@rakuten.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 AUTHORS.rst                   |  1 +
 ofproto/ofproto-dpif-xlate.c  |  2 +
 tests/tunnel-push-pop-ipv6.at | 84 +++++++++++++++++++++++++++++++++++
 tests/tunnel-push-pop.at      | 82 ++++++++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+)

Comments

Eelco Chaudron Feb. 21, 2024, 3:31 p.m. UTC | #1 
On 20 Feb 2024, at 23:35, Ilya Maximets wrote:

> Local IP is taken into account only in case of IPv4 address, IPv6
> source is not checked.  That leads to source being ignored during the
> route lookup and ultimately packets encapsulated with a source IP
> found during a route lookup, which is likely the wrong one.
>
> Even worse, after encapsulation we have a difference between the
> tunnel metadata that contains a correct source IP and the generated
> actions that used a wrong source IP.  This means that if there are
> OpenFlow rules in a bridge where packet goes after encapsulation,
> we may match on rules that do not correspond to the actual packet
> we have.
>
> Add the check for IPv6 source address before the route lookup.
>
> Tests added to check that we're actually using the configured local_ip
> as a source address in the packet.  Also adding the same test for IPv4,
> since apparently we don't have any tests covering this functionality
> for userspace tunnels.
>
> This issue also affects the case where source address is set via
> OpenFlow, e.g. 'set_filed:2001:beef::88->tun_ipv6_src', but it's just
> a different way of populating the tunnel metadata that doesn't depend
> on a tunnel to be native or kernel one.  So, not adding extra tests
> for this case for now.
>
> Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses")
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052938.html
> Reported-by: Derrick Lim <derrick.lim@rakuten.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Thanks for the patch, the changes look good to me and the tests are passing.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
diff mbox series

Patch

diff --git a/AUTHORS.rst b/AUTHORS.rst
index fc08f3bbf..f99df385b 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -589,6 +589,7 @@  David Evans                     davidjoshuaevans@gmail.com
 David Palma                     palma@onesource.pt
 David van Moolenbroek           dvmoolenbroek@aimvalley.nl
 Derek Cormier                   derek.cormier@lab.ntt.co.jp
+Derrick Lim                     derrick.lim@rakuten.com
 Dhaval Badiani                  dbadiani@vmware.com
 DK Moon
 Ding Zhi                        zhi.ding@6wind.com
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 1cf4d5f7c..89f183182 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3815,6 +3815,8 @@  native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
 
     if (flow->tunnel.ip_src) {
         in6_addr_set_mapped_ipv4(&s_ip6, flow->tunnel.ip_src);
+    } else if (ipv6_addr_is_set(&flow->tunnel.ipv6_src)) {
+        s_ip6 = flow->tunnel.ipv6_src;
     }
 
     err = tnl_route_lookup_flow(ctx, flow, &d_ip6, &s_ip6, &out_dev);
diff --git a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at
index 6d9ac6841..3f2cf8429 100644
--- a/tests/tunnel-push-pop-ipv6.at
+++ b/tests/tunnel-push-pop-ipv6.at
@@ -642,3 +642,87 @@  Listening ports:
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([tunnel_push_pop_ipv6 - local_ip configuration])
+
+OVS_VSWITCHD_START(
+    [add-port br0 p0 \
+     -- set Interface p0 type=dummy ofport_request=1 \
+                         other-config:hwaddr=aa:55:aa:55:00:00])
+AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg])
+AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy])
+AT_CHECK([ovs-vsctl add-port int-br t2 \
+          -- set Interface t2 type=geneve \
+                              options:local_ip=2001:beef::88 \
+                              options:remote_ip=2001:cafe::92 \
+                              options:key=123 ofport_request=2])
+
+dnl Setup multiple IP addresses.
+AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:cafe::88/64], [0], [OK
+])
+AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:beef::88/64], [0], [OK
+])
+dnl Checking that a local route for added IP was successfully installed.
+AT_CHECK([ovs-appctl ovs/route/show | grep Cached | sort], [0], [dnl
+Cached: 2001:beef::/64 dev br0 SRC 2001:beef::88 local
+Cached: 2001:cafe::/64 dev br0 SRC 2001:cafe::88 local
+])
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+AT_CHECK([ovs-ofctl add-flow int-br action=normal])
+
+dnl This Neighbor Advertisement from p0 has two effects:
+dnl 1. The neighbor cache will learn that 2001:cafe::92 is at f8:bc:12:44:34:b6.
+dnl 2. The br0 mac learning will learn that f8:bc:12:44:34:b6 is on p0.
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 dnl
+ 'recirc_id(0),in_port(1),dnl
+  eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(0x86dd),dnl
+  ipv6(src=2001:cafe::92,dst=2001:cafe::88,label=0,proto=58,tclass=0,hlimit=255,frag=no),dnl
+  icmpv6(type=136,code=0),dnl
+  nd(target=2001:cafe::92,sll=00:00:00:00:00:00,tll=f8:bc:12:44:34:b6)'
+])
+
+dnl Check that local_ip is used for encapsulation in the trace.
+AT_CHECK([ovs-appctl ofproto/trace int-br in_port=LOCAL \
+                | grep -E 'tunnel|actions'], [0], [dnl
+     -> output to native tunnel
+     -> tunneling to 2001:cafe::92 via br0
+     -> tunneling from aa:55:aa:55:00:00 2001:beef::88 to f8:bc:12:44:34:b6 2001:cafe::92
+Datapath actions: tnl_push(tnl_port(6081),header(size=70,type=5,dnl
+eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x86dd),dnl
+ipv6(src=2001:beef::88,dst=2001:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),dnl
+udp(src=0,dst=6081,csum=0xffff),geneve(vni=0x7b)),out_port(100)),1
+])
+
+dnl Now check that the packet actually has the local_ip in the header.
+AT_CHECK([ovs-vsctl -- set Interface p0 options:tx_pcap=p0.pcap])
+
+packet=50540000000a5054000000091234
+eth=f8bc124434b6aa55aa55000086dd
+ip6=60000000001e11402001beef0000000000000000000000882001cafe000000000000000000000092
+dnl Source port is based on a packet hash, so it may differ depending on the
+dnl compiler flags and CPU type.  Same for UDP checksum.  Masked with '....'.
+udp=....17c1001e....
+geneve=0000655800007b00
+encap=${eth}${ip6}${udp}${geneve}
+dnl Output to tunnel from a int-br internal port.
+dnl Checking that the packet arrived and it was correctly encapsulated.
+AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}"])
+OVS_WAIT_UNTIL([test $(ovs-pcap p0.pcap | grep -c "${encap}${packet}") -eq 1])
+dnl Sending again to exercise the non-miss upcall path.
+AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}"])
+OVS_WAIT_UNTIL([test $(ovs-pcap p0.pcap | grep -c "${encap}${packet}") -eq 2])
+
+dnl Finally, checking that the datapath flow also has a local_ip.
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep tnl_push \
+            | strip_ufid | strip_used], [0], [dnl
+recirc_id(0),in_port(2),packet_type(ns=0,id=0),dnl
+eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234), dnl
+packets:1, bytes:14, used:0.0s, dnl
+actions:tnl_push(tnl_port(6081),header(size=70,type=5,dnl
+eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x86dd),dnl
+ipv6(src=2001:beef::88,dst=2001:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),dnl
+udp(src=0,dst=6081,csum=0xffff),geneve(vni=0x7b)),out_port(100)),1
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 04d17b71f..97405636f 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -779,6 +779,88 @@  AT_CHECK([ovs-appctl dpctl/dump-flows | grep -q 'slow_path(action)'], [0])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([tunnel_push_pop - local_ip configuration])
+
+OVS_VSWITCHD_START(
+    [add-port br0 p0 \
+     -- set Interface p0 type=dummy ofport_request=1 \
+                         other-config:hwaddr=aa:55:aa:55:00:00])
+AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg])
+AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy])
+AT_CHECK([ovs-vsctl add-port int-br t2 \
+          -- set Interface t2 type=geneve \
+                              options:local_ip=2.2.2.88 \
+                              options:remote_ip=1.1.2.92 \
+                              options:key=123 ofport_request=2])
+
+dnl Setup multiple IP addresses.
+AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 1.1.2.88/24], [0], [OK
+])
+AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 2.2.2.88/24], [0], [OK
+])
+dnl Checking that a local route for added IP was successfully installed.
+AT_CHECK([ovs-appctl ovs/route/show | grep Cached | sort], [0], [dnl
+Cached: 1.1.2.0/24 dev br0 SRC 1.1.2.88 local
+Cached: 2.2.2.0/24 dev br0 SRC 2.2.2.88 local
+])
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+AT_CHECK([ovs-ofctl add-flow int-br action=normal])
+
+dnl This ARP reply from p0 has two effects:
+dnl 1. The ARP cache will learn that 1.1.2.92 is at f8:bc:12:44:34:b6.
+dnl 2. The br0 mac learning will learn that f8:bc:12:44:34:b6 is on p0.
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 dnl
+ 'recirc_id(0),in_port(1),dnl
+  eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),dnl
+  arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'
+])
+
+dnl Check that local_ip is used for encapsulation in the trace.
+AT_CHECK([ovs-appctl ofproto/trace int-br in_port=LOCAL \
+                | grep -E 'tunnel|actions'], [0], [dnl
+     -> output to native tunnel
+     -> tunneling to 1.1.2.92 via br0
+     -> tunneling from aa:55:aa:55:00:00 2.2.2.88 to f8:bc:12:44:34:b6 1.1.2.92
+Datapath actions: tnl_push(tnl_port(6081),header(size=50,type=5,dnl
+eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x0800),dnl
+ipv4(src=2.2.2.88,dst=1.1.2.92,proto=17,tos=0,ttl=64,frag=0x4000),dnl
+udp(src=0,dst=6081,csum=0x0),geneve(vni=0x7b)),out_port(100)),1
+])
+
+dnl Now check that the packet actually has the local_ip in the header.
+AT_CHECK([ovs-vsctl -- set Interface p0 options:tx_pcap=p0.pcap])
+
+packet=50540000000a5054000000091234
+eth=f8bc124434b6aa55aa5500000800
+ip4=450000320000400040113305020202580101025c
+dnl Source port is based on a packet hash, so it may differ depending on the
+dnl compiler flags and CPU type.  Masked with '....'.
+udp=....17c1001e0000
+geneve=0000655800007b00
+encap=${eth}${ip4}${udp}${geneve}
+dnl Output to tunnel from a int-br internal port.
+dnl Checking that the packet arrived and it was correctly encapsulated.
+AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}"])
+OVS_WAIT_UNTIL([test $(ovs-pcap p0.pcap | grep -c "${encap}${packet}") -eq 1])
+dnl Sending again to exercise the non-miss upcall path.
+AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}"])
+OVS_WAIT_UNTIL([test $(ovs-pcap p0.pcap | grep -c "${encap}${packet}") -eq 2])
+
+dnl Finally, checking that the datapath flow also has a local_ip.
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep tnl_push \
+            | strip_ufid | strip_used], [0], [dnl
+recirc_id(0),in_port(2),packet_type(ns=0,id=0),dnl
+eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234), dnl
+packets:1, bytes:14, used:0.0s, dnl
+actions:tnl_push(tnl_port(6081),header(size=50,type=5,dnl
+eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:00,dl_type=0x0800),dnl
+ipv4(src=2.2.2.88,dst=1.1.2.92,proto=17,tos=0,ttl=64,frag=0x4000),dnl
+udp(src=0,dst=6081,csum=0x0),geneve(vni=0x7b)),out_port(100)),1
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([tunnel_push_pop - underlay bridge match])
 
 OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00])