diff mbox

[ovs-dev] ovn-northd: fix logical router icmp response for directed broadcasts

Message ID 1465422666-6789-1-git-send-email-flavio@flaviof.com
State Not Applicable
Headers show

Commit Message

Flaviof June 8, 2016, 9:51 p.m. UTC 
Responding to icmp queries where the L3 destination is a directed broadcast
was not being properly handled, causing the reply to be sent to all logical
ports except for the one port that should receive it.

Reference to the mailing list thread:
http://openvswitch.org/pipermail/discuss/2016-June/021619.html

This is a proposal for using choice C in the mail discussion; where handling
of icmp queries to broadcast is performed by a separate logical rule.
Unit test has been augmented to exercise this scenario.

Note that since broadcast is contained to node where ovn-controller is running,
there may be no real concern for a potential DOS attack scenario.

Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
---
 ovn/northd/ovn-northd.c | 28 +++++++++++++++++++++++++---
 tests/ovn.at            | 39 +++++++++++++++++++++++++++------------
 2 files changed, 52 insertions(+), 15 deletions(-)

Comments

Flaviof June 9, 2016, 1:59 a.m. UTC | #1 
On Wed, Jun 8, 2016 at 5:51 PM, Flavio Fernandes <flavio@flaviof.com> wrote:

> Responding to icmp queries where the L3 destination is a directed broadcast
> was not being properly handled, causing the reply to be sent to all logical
> ports except for the one port that should receive it.
>
> Reference to the mailing list thread:
> http://openvswitch.org/pipermail/discuss/2016-June/021619.html
>
> This is a proposal for using choice C in the mail discussion; where
> handling
> of icmp queries to broadcast is performed by a separate logical rule.
> Unit test has been augmented to exercise this scenario.
>
> Note that since broadcast is contained to node where ovn-controller is
> running,
> there may be no real concern for a potential DOS attack scenario.
>
> Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
> ---
>


Update:
While testing this change, I noticed that the action eth_dst
is not affecting dl_dst. So, assuming option 'c' is the way
to go, there is still some more teaking to do here!

https://gist.github.com/4e2a080248bbde35ebbc2de956c4a194
diff mbox

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index cac0148..fe86d05 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1957,9 +1957,8 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
          * (i.e. the incoming locally attached net) does not matter.
          * The ip.ttl also does not matter (RFC1812 section 4.2.2.9) */
         match = xasprintf(
-            "(ip4.dst == "IP_FMT" || ip4.dst == "IP_FMT") && "
-            "icmp4.type == 8 && icmp4.code == 0",
-            IP_ARGS(op->ip), IP_ARGS(op->bcast));
+            "ip4.dst == "IP_FMT" && icmp4.type == 8 && icmp4.code == 0",
+            IP_ARGS(op->ip));
         char *actions = xasprintf(
             "ip4.dst = ip4.src; "
             "ip4.src = "IP_FMT"; "
@@ -1973,6 +1972,29 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
         free(match);
         free(actions);
 
+	/* ICMP echo reply for L3 directed broadcast packets. Note that
+	 * 1) this is only applicable to a specific inport and 2) the
+         * destination mac is overriden to create a unicast response
+         * that is sent out of inport. */
+        match = xasprintf(
+            "inport == %s && ip4.dst == "IP_FMT" && "
+            "icmp4.type == 8 && icmp4.code == 0",
+            op->json_key, IP_ARGS(op->bcast));
+        actions = xasprintf(
+            "eth.dst = "ETH_ADDR_FMT"; "
+            "ip4.dst = ip4.src; "
+            "ip4.src = "IP_FMT"; "
+            "ip.ttl = 255; "
+            "icmp4.type = 0; "
+            "inport = \"\"; /* Allow sending out inport. */ "
+            "next; ",
+            ETH_ADDR_ARGS(op->mac),
+	    IP_ARGS(op->ip));
+        ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
+                      match, actions);
+        free(match);
+        free(actions);
+
         /* ARP reply.  These flows reply to ARP requests for the router's own
          * IP address. */
         match = xasprintf(
diff --git a/tests/ovn.at b/tests/ovn.at
index 633cf35..1ccf7c6 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3127,13 +3127,14 @@  trim_zeros() {
 for i in 1 2; do
     : > vif$i.expected
 done
-# test_ipv4_icmp_request INPORT ETH_SRC ETH_DST IPV4_SRC IPV4_DST IP_CHKSUM ICMP_CHKSUM [EXP_IP_CHKSUM EXP_ICMP_CHKSUM]
+# test_ipv4_icmp_request INPORT ETH_SRC ETH_DST IPV4_SRC IPV4_DST IP_CHKSUM ICMP_CHKSUM \
+#                        [EXP_ETH_SRC EXP_IPV4_SRC EXP_IP_CHKSUM EXP_ICMP_CHKSUM]
 #
 # Causes a packet to be received on INPORT.  The packet is an ICMPv4
 # request with ETH_SRC, ETH_DST, IPV4_SRC, IPV4_DST, IP_CHSUM and
-# ICMP_CHKSUM as specified.  If EXP_IP_CHKSUM and EXP_ICMP_CHKSUM are
-# provided, then it should be the ip and icmp checksums of the packet
-# responded; otherwise, no reply is expected.
+# ICMP_CHKSUM as specified.  If EXP_* params are provided, then it
+# should be the source ethernet, source ip, ip and icmp checksums
+# of the packet responded; otherwise, no reply is expected.
 # In the absence of an ip checksum calculation helpers, this relies
 # on the caller to provide the checksums for the ip and icmp headers.
 # XXX This should be more systematic.
@@ -3142,12 +3143,14 @@  done
 # ETH_SRC and ETH_DST are each 12 hex digits.
 # IPV4_SRC and IPV4_DST are each 8 hex digits.
 # IP_CHSUM and ICMP_CHKSUM are each 4 hex digits.
+# EXP_ETH_SRC is 12 hex digits.
+# EXP_IPV4_SRC is 8 hex digits.
 # EXP_IP_CHSUM and EXP_ICMP_CHKSUM are each 4 hex digits.
 test_ipv4_icmp_request() {
     local inport=$1 eth_src=$2 eth_dst=$3 ipv4_src=$4 ipv4_dst=$5 ip_chksum=$6 icmp_chksum=$7
-    local exp_ip_chksum=$8 exp_icmp_chksum=$9
     shift; shift; shift; shift; shift; shift; shift
-    shift; shift
+    local exp_eth_src=$1 exp_ipv4_src=$2 exp_ip_chksum=$3 exp_icmp_chksum=$4
+    shift; shift; shift; shift
 
     # Use ttl to exercise section 4.2.2.9 of RFC1812
     local ip_ttl=01
@@ -3159,30 +3162,42 @@  test_ipv4_icmp_request() {
     local packet=${eth_dst}${eth_src}08004500005400004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}${icmp_payload}
 
     as hv1 ovs-appctl netdev-dummy/receive vif$inport $packet
-    if test X$exp_icmp_chksum != X; then
+    if test X$exp_eth_src != X; then
         # Expect to receive the reply, if any. In same port where packet was sent.
         # Note: src and dst fields are expected to be reversed.
         local icmp_type_code_response=0000
         local reply_icmp_ttl=fe
         local reply_icmp_payload=${icmp_type_code_response}${exp_icmp_chksum}${icmp_id}${icmp_seq}${icmp_data}
-        local reply=${eth_src}${eth_dst}08004500005400004000${reply_icmp_ttl}01${exp_ip_chksum}${ipv4_dst}${ipv4_src}${reply_icmp_payload}
+        local reply=${eth_src}${exp_eth_src}08004500005400004000${reply_icmp_ttl}01${exp_ip_chksum}${exp_ipv4_src}${ipv4_src}${reply_icmp_payload}
         echo $reply >> vif$inport.expected
     fi
 }
 
 # Send ping packet to router's ip addresses, from each of the 2 logical ports.
 rtr_l1_ip=$(ip_to_hex 192 168 1 1)
+rtr_l1_ip_bcast=$(ip_to_hex 192 168 1 255)
 rtr_l2_ip=$(ip_to_hex 172 16 1 1)
+rtr_l2_ip_bcast=$(ip_to_hex 172 16 1 255)
 l1_ip=$(ip_to_hex 192 168 1 2)
 l2_ip=$(ip_to_hex 172 16 1 2)
 
 # Ping router ip address that is on same subnet as the logical port
-test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l1_ip 0000 8510 02ff 8d10
-test_ipv4_icmp_request 2 000000010204 0000000102f2 $l2_ip $rtr_l2_ip 0000 8510 02ff 8d10
+test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l1_ip 0000 8510 0000000102f1 $rtr_l1_ip 02ff 8d10
+test_ipv4_icmp_request 2 000000010204 0000000102f2 $l2_ip $rtr_l2_ip 0000 8510 0000000102f2 $rtr_l2_ip 02ff 8d10
 
 # Ping router ip address that is on the other side of the logical ports
-test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l2_ip 0000 8510 02ff 8d10
-test_ipv4_icmp_request 2 000000010204 0000000102f2 $l2_ip $rtr_l1_ip 0000 8510 02ff 8d10
+test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l2_ip 0000 8510 0000000102f1 $rtr_l2_ip 02ff 8d10
+test_ipv4_icmp_request 2 000000010204 0000000102f2 $l2_ip $rtr_l1_ip 0000 8510 0000000102f2 $rtr_l1_ip 02ff 8d10
+
+# Ping directed broadcast that is on the same subnet as the logical port
+test_ipv4_icmp_request 1 000000010203 ffffffffffff $l1_ip $rtr_l1_ip_bcast 0000 8510 0000000102f1 $rtr_l1_ip 03fd 8d10
+test_ipv4_icmp_request 2 000000010204 ffffffffffff $l2_ip $rtr_l2_ip_bcast 0000 8510 0000000102f2 $rtr_l2_ip 03fd 8d10
+
+# Ping directed broadcast that is on the other side of the logical ports.
+# Expect no packets in response, because bcast domain will is outside sender's subnet.
+test_ipv4_icmp_request 1 000000010203 ffffffffffff $l1_ip $rtr_l2_ip_bcast 0000 8510
+test_ipv4_icmp_request 2 000000010204 ffffffffffff $l2_ip $rtr_l1_ip_bcast 0000 8510
+
 
 echo "---------NB dump-----"
 ovn-nbctl show