Fix ACL reject action for UDP packets.

The icmp packet generated by ovn-controller for reject ACL action for non TCP packets is not getting delivered to the sender of the original packet. This is because the icmp packets are skipped from out_pre_lb/out_pre_acl logical switch egress pipeline and this results in these icmp packets getting dropped in the ACL stage because of invalid ct flags. This patch fixes this issue by removing those logical flows. The IP checksum generated by ovn-controller is invalid. This patch fixes this issue as well. Tested-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Signed-off-by: Numan Siddique <numans@ovn.org>
ovn-org · May 6, 2020 · f792b1a · f792b1a
1 parent 4052d48
commit f792b1a
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 88 deletions.
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
@@ -1465,7 +1465,7 @@ static void
 pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
                     struct dp_packet *pkt_in,
                     const struct match *md, struct ofpbuf *userdata,
-                    bool include_orig_ip_datagram)
+                    bool set_icmp_code)
 {
     /* This action only works for IP packets, and the switch should only send
      * us IP packets this way, but check here just to be sure. */
@@ -1512,62 +1512,94 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
         packet_set_ipv4(&packet, ip_flow->nw_src, ip_flow->nw_dst,
                         ip_flow->nw_tos, 255);
 
+        uint8_t icmp_code =  1;
+        if (set_icmp_code && in_ip->ip_proto == IPPROTO_UDP) {
+            icmp_code = 3;
+        }
+
         struct icmp_header *ih = dp_packet_put_zeros(&packet, sizeof *ih);
         dp_packet_set_l4(&packet, ih);
-        packet_set_icmp(&packet, ICMP4_DST_UNREACH, 1);
-
-        if (include_orig_ip_datagram) {
-            /* RFC 1122: 3.2.2	MUST send at least the IP header and 8 bytes
-             * of header. MAY send more.
-             * RFC says return as much as we can without exceeding 576
-             * bytes.
-             * So, lets return as much as we can. */
-
-            /* Calculate available room to include the original IP + data. */
-            nh = dp_packet_l3(&packet);
-            uint16_t room = 576 - (sizeof *eh + ntohs(nh->ip_tot_len));
-            if (in_ip_len > room) {
-                in_ip_len = room;
-            }
-            dp_packet_put(&packet, in_ip, in_ip_len);
-
-            /* dp_packet_put may reallocate the buffer. Get the l3 and l4
-             * header pointers again. */
-            nh = dp_packet_l3(&packet);
-            ih = dp_packet_l4(&packet);
-            uint16_t ip_total_len = ntohs(nh->ip_tot_len) + in_ip_len;
-            nh->ip_tot_len = htons(ip_total_len);
-            ih->icmp_csum = 0;
-            ih->icmp_csum = csum(ih, sizeof *ih + in_ip_len);
-            nh->ip_csum = 0;
-            nh->ip_csum = csum(nh, sizeof *nh);
-        }
+        packet_set_icmp(&packet, ICMP4_DST_UNREACH, icmp_code);
+
+        /* RFC 1122: 3.2.2	MUST send at least the IP header and 8 bytes
+         * of header. MAY send more.
+         * RFC says return as much as we can without exceeding 576
+         * bytes.
+         * So, lets return as much as we can. */
+
+        /* Calculate available room to include the original IP + data. */
+        nh = dp_packet_l3(&packet);
+        uint16_t room = 576 - (sizeof *eh + ntohs(nh->ip_tot_len));
+        if (in_ip_len > room) {
+            in_ip_len = room;
+        }
+        dp_packet_put(&packet, in_ip, in_ip_len);
+
+        /* dp_packet_put may reallocate the buffer. Get the l3 and l4
+            * header pointers again. */
+        nh = dp_packet_l3(&packet);
+        ih = dp_packet_l4(&packet);
+        uint16_t ip_total_len = ntohs(nh->ip_tot_len) + in_ip_len;
+        nh->ip_tot_len = htons(ip_total_len);
+        ih->icmp_csum = 0;
+        ih->icmp_csum = csum(ih, sizeof *ih + in_ip_len);
+        nh->ip_csum = 0;
+        nh->ip_csum = csum(nh, sizeof *nh);
+
     } else {
         struct ip6_hdr *nh = dp_packet_put_zeros(&packet, sizeof *nh);
         struct icmp6_data_header *ih;
         uint32_t icmpv6_csum;
+        struct ip6_hdr *in_ip = dp_packet_l3(pkt_in);
 
         eh->eth_type = htons(ETH_TYPE_IPV6);
         dp_packet_set_l3(&packet, nh);
         nh->ip6_vfc = 0x60;
         nh->ip6_nxt = IPPROTO_ICMPV6;
-        nh->ip6_plen = htons(sizeof(*nh) + ICMP6_DATA_HEADER_LEN);
+        nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN);
         packet_set_ipv6(&packet, &ip_flow->ipv6_src, &ip_flow->ipv6_dst,
                         ip_flow->nw_tos, ip_flow->ipv6_label, 255);
 
         ih = dp_packet_put_zeros(&packet, sizeof *ih);
         dp_packet_set_l4(&packet, ih);
         ih->icmp6_base.icmp6_type = ICMP6_DST_UNREACH;
         ih->icmp6_base.icmp6_code = 1;
+
+        if (set_icmp_code && in_ip->ip6_nxt == IPPROTO_UDP) {
+            ih->icmp6_base.icmp6_code = ICMP6_DST_UNREACH_NOPORT;
+        }
         ih->icmp6_base.icmp6_cksum = 0;
 
-        uint8_t *data = dp_packet_put_zeros(&packet, sizeof *nh);
-        memcpy(data, dp_packet_l3(pkt_in), sizeof(*nh));
+        nh = dp_packet_l3(&packet);
+
+        /* RFC 4443: 3.1.
+         *
+         * 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+         * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+         * |     Type      |     Code      |          Checksum             |
+         * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+         * |                             Unused                            |
+         * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+         * |                    As much of invoking packet                 |
+         * +                as possible without the ICMPv6 packet          +
+         * |                exceeding the minimum IPv6 MTU [IPv6]          |
+         * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+         */
+
+        uint16_t room = 1280 - (sizeof *eh + sizeof *nh +
+                                ICMP6_DATA_HEADER_LEN);
+        uint16_t in_ip_len = (uint16_t) sizeof *in_ip + ntohs(in_ip->ip6_plen);
+        if (in_ip_len > room) {
+            in_ip_len = room;
+        }
+
+        dp_packet_put(&packet, in_ip, in_ip_len);
+        nh->ip6_plen = htons(ICMP6_DATA_HEADER_LEN + in_ip_len);
 
         icmpv6_csum = packet_csum_pseudoheader6(dp_packet_l3(&packet));
         ih->icmp6_base.icmp6_cksum = csum_finish(
             csum_continue(icmpv6_csum, ih,
-                          sizeof(*nh) + ICMP6_DATA_HEADER_LEN));
+                          in_ip_len + ICMP6_DATA_HEADER_LEN));
     }
 
     if (ip_flow->vlans[0].tci & htons(VLAN_CFI)) {
@@ -2658,12 +2690,12 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
 
     case ACTION_OPCODE_ICMP:
         pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
-                            &userdata, false);
+                            &userdata, true);
         break;
 
     case ACTION_OPCODE_ICMP4_ERROR:
         pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
-                            &userdata, true);
+                            &userdata, false);
         break;
 
     case ACTION_OPCODE_TCP_RESET:

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
@@ -4740,12 +4740,10 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
          * Not to do conntrack on ND and ICMP destination
          * unreachable packets. */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110,
-                      "nd || nd_rs || nd_ra || icmp4.type == 3 || "
-                      "icmp6.type == 1 || "
+                      "nd || nd_rs || nd_ra || "
                       "(udp && udp.src == 546 && udp.dst == 547)", "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110,
-                      "nd || nd_rs || nd_ra || icmp4.type == 3 || "
-                      "icmp6.type == 1 || "
+                      "nd || nd_rs || nd_ra || "
                       "(udp && udp.src == 546 && udp.dst == 547)", "next;");
 
         /* Ingress and Egress Pre-ACL Table (Priority 100).
@@ -4857,12 +4855,10 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
 {
     /* Do not send ND packets to conntrack */
     ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110,
-                  "nd || nd_rs || nd_ra || icmp4.type == 3 ||"
-                  "icmp6.type == 1",
+                  "nd || nd_rs || nd_ra",
                   "next;");
     ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 110,
-                  "nd || nd_rs || nd_ra || icmp4.type == 3 ||"
-                  "icmp6.type == 1",
+                  "nd || nd_rs || nd_ra",
                   "next;");
 
     /* Do not send service monitor packets to conntrack. */
@@ -5041,9 +5037,10 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
         ds_put_format(&actions, "%s ", extra_actions->string);
     }
     ds_put_format(&actions, "reg0 = 0; "
-                  "eth.dst <-> eth.src; ip4.dst <-> ip4.src; "
-                  "icmp4 { outport <-> inport; %s };",
-                  ingress ? "output;" : "next(pipeline=ingress,table=0);");
+                  "icmp4 { eth.dst <-> eth.src; ip4.dst <-> ip4.src; "
+                  "outport <-> inport; %s };",
+                  ingress ? "next(pipeline=egress,table=5);"
+                          : "next(pipeline=ingress,table=19);");
     ovn_lflow_add_with_hint(lflows, od, stage,
                             acl->priority + OVN_ACL_PRI_OFFSET,
                             ds_cstr(&match), ds_cstr(&actions), stage_hint);
@@ -5060,7 +5057,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
     ds_put_format(&actions, "reg0 = 0; icmp6 { "
                   "eth.dst <-> eth.src; ip6.dst <-> ip6.src; "
                   "outport <-> inport; %s };",
-                  ingress ? "output;" : "next(pipeline=ingress,table=0);");
+                  ingress ? "next(pipeline=egress,table=5);"
+                          : "next(pipeline=ingress,table=19);");
     ovn_lflow_add_with_hint(lflows, od, stage,
                             acl->priority + OVN_ACL_PRI_OFFSET,
                             ds_cstr(&match), ds_cstr(&actions), stage_hint);

diff --git a/tests/ovn.at b/tests/ovn.at
@@ -11823,13 +11823,13 @@ test_ip_packet() {
 
     local ip_ttl=ff
     local packet=${eth_dst}${eth_src}08004500001400004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}
-
+    local orig_pkt_in_reply=4500001400004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}
     local reply_icmp_ttl=ff
     local icmp_type_code_response=0301
     local icmp_data=00000000
     local reply_icmp_payload=${icmp_type_code_response}${exp_icmp_chksum}${icmp_data}
-    local reply=${eth_src}${eth_dst}08004500001c00004000${reply_icmp_ttl}01${exp_ip_chksum}${ipv4_dst}${ipv4_src}${reply_icmp_payload}
-    echo $reply >> vif$inport.expected
+    local reply=${eth_src}${eth_dst}08004500003000004000${reply_icmp_ttl}01${exp_ip_chksum}${ipv4_dst}${ipv4_src}${reply_icmp_payload}
+    echo $reply$orig_pkt_in_reply >> vif$inport.expected
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
 }
@@ -11846,7 +11846,7 @@ test_ipv6_packet() {
     local ip6_hdr=6000000000083aff${ipv6_src}${ipv6_dst}
     local packet=${eth_dst}${eth_src}86dd${ip6_hdr}0000000000000000
 
-    local reply=${eth_src}${eth_dst}86dd6000000000303aff${ipv6_dst}${ipv6_src}0101${exp_icmp_chksum}00000000${ip6_hdr}
+    local reply=${eth_src}${eth_dst}86dd6000000000383aff${ipv6_dst}${ipv6_src}0101${exp_icmp_chksum}00000000${ip6_hdr}0000000000000000
     echo $reply >> vif$inport.expected
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
@@ -11924,11 +11924,11 @@ ovn-nbctl --log acl-add sw0 from-lport 1000 "inport == \"sw0-p21\"" reject
 # Allow some time for ovn-northd and ovn-controller to catch up.
 ovn-nbctl --timeout=3 --wait=hv sync
 
-test_ip_packet 11 1 000000000011 000000000021 $(ip_to_hex 192 168 1 11) $(ip_to_hex 192 168 1 21) 0000 7d8d fcfe
-test_ip_packet 21 2 000000000021 000000000011 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 11) 0000 7d8d fcfe
-test_ip_packet 31 3 000000000031 000000000012 $(ip_to_hex 192 168 1 31) $(ip_to_hex 192 168 1 12) 0000 7d82 fcfe
+test_ip_packet 11 1 000000000011 000000000021 $(ip_to_hex 192 168 1 11) $(ip_to_hex 192 168 1 21) 0000 f85b f576
+test_ip_packet 21 2 000000000021 000000000011 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 11) 0000 f85b f576
+test_ip_packet 31 3 000000000031 000000000012 $(ip_to_hex 192 168 1 31) $(ip_to_hex 192 168 1 12) 0000 f850 f56b
 
-test_ipv6_packet 11 1 000000000011 000000000021 fe80000000000000020001fffe000001 fe80000000000000020001fffe000002 6183
+test_ipv6_packet 11 1 000000000011 000000000021 fe80000000000000020001fffe000001 fe80000000000000020001fffe000002 617b
 
 test_tcp_syn_packet 11 1 000000000011 000000000021 $(ip_to_hex 192 168 1 11) $(ip_to_hex 192 168 1 21) 0000 8b40 3039 0000 b85f 70e4
 test_tcp_syn_packet 21 2 000000000021 000000000011 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 11) 0000 8b40 3039 0000 b85f 70e4
@@ -12881,13 +12881,13 @@ test_ip_packet() {
 
     local ip_ttl=01
     local packet=${eth_dst}${eth_src}08004500001400004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}
-
+    local orig_pkt_in_reply=4500001400004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}
     local reply_icmp_ttl=fe
     local icmp_type_code_response=0b00
     local icmp_data=00000000
     local reply_icmp_payload=${icmp_type_code_response}${exp_icmp_chksum}${icmp_data}
-    local reply=${eth_src}${eth_dst}08004500001c00004000${reply_icmp_ttl}01${exp_ip_chksum}${ip_router}${ipv4_src}${reply_icmp_payload}
-    echo $reply >> vif$inport.expected
+    local reply=${eth_src}${eth_dst}08004500003000004000${reply_icmp_ttl}01${exp_ip_chksum}${ip_router}${ipv4_src}${reply_icmp_payload}
+    echo $reply$orig_pkt_in_reply >> vif$inport.expected
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
 }
@@ -12905,7 +12905,7 @@ test_ip6_packet() {
     local ip6_hdr=6000000000151101${ipv6_src}${ipv6_dst}
     local packet=${eth_dst}${eth_src}86dd${ip6_hdr}dbb8303900155bac6b646f65206676676e6d66720a
 
-    local reply=${eth_src}${eth_dst}86dd6000000000303afe${ipv6_router}${ipv6_src}0300${exp_icmp_chksum}00000000${ip6_hdr}
+    local reply=${eth_src}${eth_dst}86dd6000000000453afe${ipv6_router}${ipv6_src}0300${exp_icmp_chksum}00000000${ip6_hdr}dbb8303900155bac6b646f65206676676e6d66720a
     echo $reply >> vif$inport.expected
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
@@ -12947,8 +12947,8 @@ OVN_POPULATE_ARP
 # allow some time for ovn-northd and ovn-controller to catch up.
 ovn-nbctl --wait=hv sync
 
-test_ip_packet 1 1 000000000001 00000000ff01 $(ip_to_hex 192 168 1 1) $(ip_to_hex 192 168 2 1) $(ip_to_hex 192 168 1 254) 0000 7dae f4ff
-test_ip6_packet 1 1 000000000001 00000000ff01 20010db8000100000000000000000011 20010db8000200000000000000000011 20010db8000100000000000000000001 d461
+test_ip_packet 1 1 000000000001 00000000ff01 $(ip_to_hex 192 168 1 1) $(ip_to_hex 192 168 2 1) $(ip_to_hex 192 168 1 254) 0000 f87c ea96
+test_ip6_packet 1 1 000000000001 00000000ff01 20010db8000100000000000000000011 20010db8000200000000000000000011 20010db8000100000000000000000001 1c22
 OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected])
 
 OVN_CLEANUP([hv1], [hv2])
@@ -12977,12 +12977,12 @@ test_ip_packet() {
 
     local ip_ttl=ff
     local packet=${eth_dst}${eth_src}08004500001400004000${ip_ttl}${l4_proto}${ip_chksum}${ipv4_src}${ip_router}
-
+    local orig_pkt_in_reply=4500001400004000${ip_ttl}${l4_proto}${ip_chksum}${ipv4_src}${ip_router}
     local reply_icmp_ttl=fe
     local icmp_data=00000000
     local reply_icmp_payload=${exp_icmp_code}${exp_icmp_chksum}${icmp_data}
-    local reply=${eth_src}${eth_dst}08004500001c00004000${reply_icmp_ttl}01${exp_ip_chksum}${ip_router}${ipv4_src}${reply_icmp_payload}
-    echo $reply >> vif$inport.expected
+    local reply=${eth_src}${eth_dst}08004500003000004000${reply_icmp_ttl}01${exp_ip_chksum}${ip_router}${ipv4_src}${reply_icmp_payload}
+    echo $reply$orig_pkt_in_reply >> vif$inport.expected
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
 }
@@ -13048,7 +13048,9 @@ test_ip6_packet() {
     local ip6_hdr=60000000${ipv6_len}${ipv6_proto}ff${ipv6_src}${ipv6_dst}
     local packet=${eth_dst}${eth_src}86dd${ip6_hdr}${data}
 
-    local reply=${eth_src}${eth_dst}86dd6000000000303afe${ipv6_dst}${ipv6_src}${exp_icmp_code}${exp_icmp_chksum}00000000${ip6_hdr}
+    local reply_ip_len=`expr 48 + ${#data} / 2`
+    reply_ip_len=$(printf "%x" $reply_ip_len)
+    local reply=${eth_src}${eth_dst}86dd6000000000${reply_ip_len}3afe${ipv6_dst}${ipv6_src}${exp_icmp_code}${exp_icmp_chksum}00000000${ip6_hdr}${data}
     echo $reply >> vif$inport.expected
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
@@ -13090,13 +13092,13 @@ OVN_POPULATE_ARP
 # allow some time for ovn-northd and ovn-controller to catch up.
 ovn-nbctl --wait=hv sync
 
-test_ip_packet 1 1 000000000001 00000000ff01 $(ip_to_hex 192 168 1 1) $(ip_to_hex 192 168 1 254) 11 0000 7dae fcfc 0303
-test_ip_packet 1 1 000000000001 00000000ff01 $(ip_to_hex 192 168 1 1) $(ip_to_hex 192 168 1 254) 84 0000 7dae fcfd 0302
-test_ip6_packet 1 1 000000000001 00000000ff01 20010db8000100000000000000000011 20010db8000100000000000000000001 11 0015 dbb8303900155bac6b646f65206676676e6d66720a 0104 d570
+test_ip_packet 1 1 000000000001 00000000ff01 $(ip_to_hex 192 168 1 1) $(ip_to_hex 192 168 1 254) 11 0000 f87c f485 0303
+test_ip_packet 1 1 000000000001 00000000ff01 $(ip_to_hex 192 168 1 1) $(ip_to_hex 192 168 1 254) 84 0000 f87c f413 0302
+test_ip6_packet 1 1 000000000001 00000000ff01 20010db8000100000000000000000011 20010db8000100000000000000000001 11 0015 dbb8303900155bac6b646f65206676676e6d66720a 0104 1d31
 OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected])
 
 test_tcp_syn_packet 2 2 000000000002 00000000ff02 $(ip_to_hex 192 168 2 1) $(ip_to_hex 192 168 2 254) 0000 8b40 3039 0000 b680 6e05
-test_ip6_packet 2 2 000000000002 00000000ff02 20010db8000200000000000000000011 20010db8000200000000000000000001 84 0004 01020304 0103 627e
+test_ip6_packet 2 2 000000000002 00000000ff02 20010db8000200000000000000000011 20010db8000200000000000000000001 84 0004 01020304 0103 5e74
 test_tcp6_packet 2 2 000000000002 00000000ff02 20010db8000200000000000000000011 20010db8000200000000000000000001 8b40 3039 0000 98cd
 OVN_CHECK_PACKETS([hv2/vif2-tx.pcap], [vif2.expected])