[ovs-dev,v2,4/6] ovn: Add a new OVN action 'icmp4_error'
diff mbox series

Message ID 20190412130548.28591-1-nusiddiq@redhat.com
State Superseded
Headers show
Series
  • Address MTU issue for larger packets in OVN
Related show

Commit Message

Numan Siddique April 12, 2019, 1:05 p.m. UTC
From: Numan Siddique <nusiddiq@redhat.com>

This action is similar to the existing 'icmp4' OVN action except that
that this action is expected to be used to generate an ICMPv4 packet
in response to an error in original IP packet. When this action
injects the icmpv4 packet, it also copies the original IP datagram
following the icmp4 header as per RFC 1122: 3.2.2

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
---
 include/ovn/actions.h     |  7 ++++++
 ovn/controller/pinctrl.c  | 47 +++++++++++++++++++++++++++++++++++++--
 ovn/lib/actions.c         | 22 ++++++++++++++++++
 ovn/ovn-sb.xml            | 20 ++++++++++++-----
 ovn/utilities/ovn-trace.c |  5 +++++
 tests/ovn.at              | 15 +++++++++++++
 6 files changed, 109 insertions(+), 7 deletions(-)

Patch
diff mbox series

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 89e28c50c..af8b0a4a0 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -65,6 +65,7 @@  struct ovn_extend_table;
     OVNACT(CLONE,             ovnact_nest)            \
     OVNACT(ARP,               ovnact_nest)            \
     OVNACT(ICMP4,             ovnact_nest)            \
+    OVNACT(ICMP4_ERROR,       ovnact_nest)            \
     OVNACT(ICMP6,             ovnact_nest)            \
     OVNACT(TCP_RESET,         ovnact_nest)            \
     OVNACT(ND_NA,             ovnact_nest)            \
@@ -471,6 +472,12 @@  enum action_opcode {
      /* MTU value (to put in the icmp4 header field - frag_mtu) follow the
      * action header. */
     ACTION_OPCODE_PUT_ICMP4_FRAG_MTU,
+
+    /* "icmp4_error { ...actions... }".
+     *
+     * The actions, in OpenFlow 1.3 format, follow the action_header.
+     */
+    ACTION_OPCODE_ICMP4_ERROR,
 };
 
 /* Header. */
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 63202e6d4..2f22a57b2 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -531,7 +531,8 @@  pinctrl_handle_arp(struct rconn *swconn, const struct flow *ip_flow,
 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)
+                    const struct match *md, struct ofpbuf *userdata,
+                    bool include_orig_ip_datagram)
 {
     /* 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. */
@@ -556,6 +557,16 @@  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
     eh->eth_src = ip_flow->dl_src;
 
     if (get_dl_type(ip_flow) == htons(ETH_TYPE_IP)) {
+        struct ip_header *in_ip = dp_packet_l3(pkt_in);
+        uint16_t in_ip_len = ntohs(in_ip->ip_tot_len);
+        if (in_ip_len < IP_HEADER_LEN) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+            VLOG_WARN_RL(&rl,
+                        "ICMP action on IP packet with invalid length (%u)",
+                        in_ip_len);
+            return;
+        }
+
         struct ip_header *nh = dp_packet_put_zeros(&packet, sizeof *nh);
 
         eh->eth_type = htons(ETH_TYPE_IP);
@@ -571,6 +582,33 @@  pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow,
         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);
+        }
     } else {
         struct ip6_hdr *nh = dp_packet_put_zeros(&packet, sizeof *nh);
         struct icmp6_error_header *ih;
@@ -1674,7 +1712,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);
+                            &userdata, false);
+        break;
+
+    case ACTION_OPCODE_ICMP4_ERROR:
+        pinctrl_handle_icmp(swconn, &headers, &packet, &pin.flow_metadata,
+                            &userdata, true);
         break;
 
     case ACTION_OPCODE_TCP_RESET:
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index e8940b091..f78e6ffcb 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -1153,6 +1153,12 @@  parse_ICMP4(struct action_context *ctx)
     parse_nested_action(ctx, OVNACT_ICMP4, "ip4");
 }
 
+static void
+parse_ICMP4_ERROR(struct action_context *ctx)
+{
+    parse_nested_action(ctx, OVNACT_ICMP4_ERROR, "ip4");
+}
+
 static void
 parse_ICMP6(struct action_context *ctx)
 {
@@ -1210,6 +1216,12 @@  format_ICMP4(const struct ovnact_nest *nest, struct ds *s)
     format_nested_action(nest, "icmp4", s);
 }
 
+static void
+format_ICMP4_ERROR(const struct ovnact_nest *nest, struct ds *s)
+{
+    format_nested_action(nest, "icmp4_error", s);
+}
+
 static void
 format_ICMP6(const struct ovnact_nest *nest, struct ds *s)
 {
@@ -1287,6 +1299,14 @@  encode_ICMP4(const struct ovnact_nest *on,
     encode_nested_actions(on, ep, ACTION_OPCODE_ICMP, ofpacts);
 }
 
+static void
+encode_ICMP4_ERROR(const struct ovnact_nest *on,
+                   const struct ovnact_encode_params *ep,
+                   struct ofpbuf *ofpacts)
+{
+    encode_nested_actions(on, ep, ACTION_OPCODE_ICMP4_ERROR, ofpacts);
+}
+
 static void
 encode_ICMP6(const struct ovnact_nest *on,
              const struct ovnact_encode_params *ep,
@@ -2412,6 +2432,8 @@  parse_action(struct action_context *ctx)
         parse_ARP(ctx);
     } else if (lexer_match_id(ctx->lexer, "icmp4")) {
         parse_ICMP4(ctx);
+    } else if (lexer_match_id(ctx->lexer, "icmp4_error")) {
+        parse_ICMP4_ERROR(ctx);
     } else if (lexer_match_id(ctx->lexer, "icmp6")) {
         parse_ICMP6(ctx);
     } else if (lexer_match_id(ctx->lexer, "tcp_reset")) {
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 4e080abff..0d35fe4fc 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1820,19 +1820,22 @@ 
 
       <dl>
         <dt><code>icmp4 { <var>action</var>; </code>...<code> };</code></dt>
+        <dt>
+          <code>icmp4_error { <var>action</var>; </code>...<code> };</code>
+        </dt>
         <dd>
           <p>
             Temporarily replaces the IPv4 packet being processed by an ICMPv4
             packet and executes each nested <var>action</var> on the ICMPv4
-            packet.  Actions following the <var>icmp4</var> action, if any,
+            packet.  Actions following these actions, if any,
             apply to the original, unmodified packet.
           </p>
 
           <p>
-            The ICMPv4 packet that this action operates on is initialized based
-            on the IPv4 packet being processed, as follows.  These are default
-            values that the nested actions will probably want to change.
-            Ethernet and IPv4 fields not listed here are not changed:
+            The ICMPv4 packet that these actions operates on is initialized
+            based on the IPv4 packet being processed, as follows.  These are
+            default values that the nested actions will probably want to
+            change. Ethernet and IPv4 fields not listed here are not changed:
           </p>
 
           <ul>
@@ -1843,6 +1846,13 @@ 
             <li><code>icmp4.code = 1</code> (host unreachable)</li>
           </ul>
 
+          <p>
+              <code>icmp4_error</code> action is expected to be used to
+              generate an ICMPv4 packet in response to an error in original
+              IP packet. When this action generates the ICMPv4 packet, it
+              also copies the original IP datagram following the ICMPv4 header
+              as per RFC 1122: 3.2.2.
+          </p>
           <p><b>Prerequisite:</b> <code>ip4</code></p>
         </dd>
 
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index e03179c8f..28e2bb075 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -2116,6 +2116,11 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
                           super);
             break;
 
+        case OVNACT_ICMP4_ERROR:
+            execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id,
+                          pipeline, super);
+            break;
+
         case OVNACT_ICMP6:
             execute_icmp6(ovnact_get_ICMP6(a), dp, uflow, table_id, pipeline,
                           super);
diff --git a/tests/ovn.at b/tests/ovn.at
index 02ab94a87..92a6bed26 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1295,6 +1295,21 @@  icmp4 { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; output;
     encodes as controller(userdata=00.00.00.0a.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.28.00.00.23.20.00.25.00.00.00.00.00.00.00.03.00.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.00.04.00.04.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
     has prereqs ip4
 
+# icmp4_error
+icmp4_error { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
+    encodes as controller(userdata=00.00.00.0e.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
+    has prereqs ip4
+
+icmp4_error { };
+    formats as icmp4_error { drop; };
+    encodes as controller(userdata=00.00.00.0e.00.00.00.00)
+    has prereqs ip4
+
+# icmp4_error with icmp4.frag_mtu
+icmp4_error { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; output;
+    encodes as controller(userdata=00.00.00.0e.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.28.00.00.23.20.00.25.00.00.00.00.00.00.00.03.00.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.00.04.00.04.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
+    has prereqs ip4
+
 icmp4.frag_mtu = 1500;
     encodes as controller(userdata=00.00.00.0d.00.00.00.00.05.dc,pause)