diff mbox series

[ovs-dev,RFC,v3,4/5] ovn: Support OVS action 'check_pkt_larger' in OVN

Message ID 20190110180125.4878-1-nusiddiq@redhat.com
State RFC
Headers show
Series Address MTU issue for larger packets in OVN | expand

Commit Message

Numan Siddique Jan. 10, 2019, 6:01 p.m. UTC
From: Numan Siddique <nusiddiq@redhat.com>

Previous commit added a new OVS action 'check_pkt_larger'. This
patch supports that action in OVN. The syntax to use this would be

reg0[0] = check_pkt_larger(LEN)

Upcoming commit will make use of this action in ovn-northd and
will generate an ICMP4 packet if the packet length is greater than
the specified length.

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 include/ovn/actions.h     | 10 +++++++-
 ovn/controller/pinctrl.c  | 15 +++++++++++
 ovn/lib/actions.c         | 53 +++++++++++++++++++++++++++++++++++++++
 ovn/ovn-sb.xml            | 21 ++++++++++++++++
 ovn/utilities/ovn-trace.c |  4 ++-
 tests/ovn.at              | 10 ++++++++
 6 files changed, 111 insertions(+), 2 deletions(-)

Comments

Ben Pfaff Feb. 12, 2019, 3:22 a.m. UTC | #1
On Thu, Jan 10, 2019 at 11:31:25PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> Previous commit added a new OVS action 'check_pkt_larger'. This
> patch supports that action in OVN. The syntax to use this would be
> 
> reg0[0] = check_pkt_larger(LEN)
> 
> Upcoming commit will make use of this action in ovn-northd and
> will generate an ICMP4 packet if the packet length is greater than
> the specified length.
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Thanks

In pinctrl_handle_icmp(), this attempts to add two integers that are in
network byte order, without first converting them to host byte order:

            nh->ip_tot_len += htons(orig_ip_datagram_len);

I think that the code that includes the original IP header in
pinctrl_handle_icmp() should handle the case where the original IP
header is longer than 20 bytes (and it should also handle the case where
it is short or invalid or followed by fewer than 8 bytes of payload).

I guess that ovn-trace should allow the user to specify what value to
return.

Thanks,

Ben.
diff mbox series

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 095b60df0..6571e2aee 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -81,7 +81,8 @@  struct ovn_extend_table;
     OVNACT(PUT_ND_RA_OPTS,    ovnact_put_opts)        \
     OVNACT(ND_NS,             ovnact_nest)            \
     OVNACT(SET_METER,         ovnact_set_meter)       \
-    OVNACT(OVNFIELD_LOAD,     ovnact_load)
+    OVNACT(OVNFIELD_LOAD,     ovnact_load)            \
+    OVNACT(CHECK_PKT_LARGER,  ovnact_check_pkt_larger)
 
 /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
 enum OVS_PACKED_ENUM ovnact_type {
@@ -311,6 +312,13 @@  struct ovnact_set_meter {
     uint64_t burst;                  /* burst rate field, in kbps. */
 };
 
+/* OVNACT_CHECK_IP_PKT_LARGER. */
+struct ovnact_check_pkt_larger {
+    struct ovnact ovnact;
+    uint16_t pkt_len;
+    struct expr_field dst;      /* 1-bit destination field. */
+};
+
 /* Internal use by the helpers below. */
 void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
 void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 59bfe33cc..ba6d4a24e 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -452,6 +452,8 @@  pinctrl_handle_icmp(const struct flow *ip_flow, struct dp_packet *pkt_in,
         packet_set_icmp(&packet, ICMP4_DST_UNREACH, 1);
         uint8_t *n_ovnfield_acts = ofpbuf_try_pull(userdata,
                                                    sizeof *n_ovnfield_acts);
+        bool include_orig_ip_datagram = false;
+
         if (n_ovnfield_acts && *n_ovnfield_acts) {
             for (uint8_t i = 0; i < *n_ovnfield_acts; i++) {
                  struct ovnfield_act_header *oah =
@@ -465,12 +467,25 @@  pinctrl_handle_icmp(const struct flow *ip_flow, struct dp_packet *pkt_in,
                      ovs_be16 *mtu = ofpbuf_try_pull(userdata, sizeof *mtu);
                      if (mtu) {
                          ih->icmp_fields.frag.mtu = *mtu;
+                         include_orig_ip_datagram = true;
                      }
                      break;
                  }
                  }
             }
         }
+
+        if (include_orig_ip_datagram) {
+            size_t orig_ip_datagram_len = sizeof(struct ip_header) + 8;
+            uint8_t *data = dp_packet_put_zeros(&packet, orig_ip_datagram_len);
+            memcpy(data, dp_packet_l3(pkt_in), orig_ip_datagram_len);
+            nh->ip_tot_len += htons(orig_ip_datagram_len);
+            ih->icmp_csum = 0;
+            ih->icmp_csum = csum(ih, sizeof *ih + orig_ip_datagram_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;
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index a2000667e..ea9120b79 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -2366,6 +2366,55 @@  encode_OVNFIELD_LOAD(const struct ovnact_load *load,
     }
 }
 
+static void
+parse_check_pkt_larger(struct action_context *ctx,
+                       const struct expr_field *dst,
+                       struct ovnact_check_pkt_larger *cipl)
+{
+    int pkt_len;
+
+    lexer_get(ctx->lexer); /* Skip check_pkt_len. */
+    if (!lexer_force_match(ctx->lexer, LEX_T_LPAREN)
+        || !lexer_get_int(ctx->lexer, &pkt_len)
+        || !lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
+        return;
+    }
+
+    /* Validate that the destination is a 1-bit, modifiable field. */
+    char *error = expr_type_check(dst, 1, true);
+    if (error) {
+        lexer_error(ctx->lexer, "%s", error);
+        free(error);
+        return;
+    }
+    cipl->dst = *dst;
+    cipl->pkt_len = pkt_len;
+}
+
+static void
+format_CHECK_PKT_LARGER(const struct ovnact_check_pkt_larger *cipl,
+                        struct ds *s)
+{
+    expr_field_format(&cipl->dst, s);
+    ds_put_format(s, " = check_pkt_larger(%d);", cipl->pkt_len);
+}
+
+static void
+encode_CHECK_PKT_LARGER(const struct ovnact_check_pkt_larger *cipl,
+                        const struct ovnact_encode_params *ep OVS_UNUSED,
+                        struct ofpbuf *ofpacts)
+{
+    struct ofpact_check_pkt_larger *check_pkt_larger =
+        ofpact_put_CHECK_PKT_LARGER(ofpacts);
+    check_pkt_larger->pkt_len = cipl->pkt_len;
+    check_pkt_larger->dst = expr_resolve_field(&cipl->dst);
+}
+
+static void
+ovnact_check_pkt_larger_free(struct ovnact_check_pkt_larger *cipl OVS_UNUSED)
+{
+}
+
 /* Parses an assignment or exchange or put_dhcp_opts action. */
 static void
 parse_set_action(struct action_context *ctx)
@@ -2395,6 +2444,10 @@  parse_set_action(struct action_context *ctx)
                 && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
             parse_put_nd_ra_opts(ctx, &lhs,
                                  ovnact_put_PUT_ND_RA_OPTS(ctx->ovnacts));
+        } else if (!strcmp(ctx->lexer->token.s, "check_pkt_larger")
+                && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
+            parse_check_pkt_larger(ctx, &lhs,
+                                   ovnact_put_CHECK_PKT_LARGER(ctx->ovnacts));
         } else {
             parse_assignment_action(ctx, false, &lhs);
         }
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 0784df537..3588c2a95 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1766,6 +1766,27 @@ 
 
           <p><b>Example:</b> <code>set_meter(100, 1000);</code></p>
         </dd>
+
+        <dt><code><var>R</var> = check_pkt_larger(<var>L</var>)</code></dt>
+        <dd>
+          <p>
+            <b>Parameters</b>: packet length <var>L</var> to check for
+            in bytes.
+          </p>
+
+          <p>
+            <b>Result</b>: stored to a 1-bit subfield <var>R</var>.
+          </p>
+
+          <p>
+            This is a logical equivalent of the OpenFlow
+            <code>check_pkt_larger</code> action. If the packet is larger
+            than the length specified in <var>L</var>, it stores 1 in the
+            subfield <var>R</var>.
+          </p>
+
+          <p><b>Example: </b><code>reg0[6] = check_pkt_larger(1000);</code></p>
+        </dd>
       </dl>
 
       <dl>
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 6f9c3df09..0ebb5723d 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -2109,8 +2109,10 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
 
         case OVNACT_OVNFIELD_LOAD:
             break;
-        }
 
+        case OVNACT_CHECK_PKT_LARGER:
+            break;
+        }
     }
     ds_destroy(&s);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index 6c7a054af..442b3a917 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1330,6 +1330,16 @@  ip4.src <-> ip6.src[0..31];
     encodes as push:NXM_NX_IPV6_SRC[0..31],push:NXM_OF_IP_SRC[],pop:NXM_NX_IPV6_SRC[0..31],pop:NXM_OF_IP_SRC[]
     has prereqs eth.type == 0x800 && eth.type == 0x86dd
 
+# check_pkt_larger
+reg0[0] = check_pkt_larger(1500);
+    encodes as check_pkt_larger(1500)->NXM_NX_XXREG0[96]
+
+reg0 = check_pkt_larger(1500);
+    Cannot use 32-bit field reg0[0..31] where 1-bit field is required.
+
+reg0 = check_pkt_larger(foo);
+    Syntax error at `foo' expecting `;'.
+
 # Miscellaneous negative tests.
 ;
     Syntax error at `;'.