diff mbox series

[ovs-dev,RFC] Add a new OVS action check_pkt_larger

Message ID 20181105161510.4662-1-nusiddiq@redhat.com
State Superseded
Headers show
Series [ovs-dev,RFC] Add a new OVS action check_pkt_larger | expand

Commit Message

Numan Siddique Nov. 5, 2018, 4:15 p.m. UTC
From: Numan Siddique <nusiddiq@redhat.com>

This patch adds a new action 'check_pkt_larger' which checks if the
packet is larger than the given size and stores the result in the
destination register.

Usage: check_pkt_larger:len->REGISTER
Eg. match=...,actions=check_pkt_larger:1442->NXM_NX_REG0[0],next;

When translating this action, SLOW_ACTION is set so that datapath
flow is not added.

This action is intended to be used by OVN to check the packet length
and generate an ICMP packet with type 3, code 4 and next hop mtu
in the logical router pipeline if the MTU of the physical interface
is lesser than the packet length. More information can be found here [1]

TODO:
 - Add test case.
 - Change the action format from check_pkt_larger:len to check_pkt_larger(len)

Request to suggest a better name for the action in case 'check_pkt_larger'
seems odd.

[1] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 include/openvswitch/ofp-actions.h | 11 ++++
 lib/ofp-actions.c                 | 95 ++++++++++++++++++++++++++++++-
 ofproto/ofproto-dpif-xlate.c      | 23 ++++++++
 3 files changed, 128 insertions(+), 1 deletion(-)

Comments

Ben Pfaff Nov. 5, 2018, 8:56 p.m. UTC | #1
On Mon, Nov 05, 2018 at 09:45:10PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> This patch adds a new action 'check_pkt_larger' which checks if the
> packet is larger than the given size and stores the result in the
> destination register.
> 
> Usage: check_pkt_larger:len->REGISTER
> Eg. match=...,actions=check_pkt_larger:1442->NXM_NX_REG0[0],next;
> 
> When translating this action, SLOW_ACTION is set so that datapath
> flow is not added.
> 
> This action is intended to be used by OVN to check the packet length
> and generate an ICMP packet with type 3, code 4 and next hop mtu
> in the logical router pipeline if the MTU of the physical interface
> is lesser than the packet length. More information can be found here [1]
> 
> TODO:
>  - Add test case.
>  - Change the action format from check_pkt_larger:len to check_pkt_larger(len)
> 
> Request to suggest a better name for the action in case 'check_pkt_larger'
> seems odd.
> 
> [1] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Thanks for working on this.

It looks like this patch makes the assumption that every packet goes
through userspace, but that isn't true.  The action needs to be
implemented in the OVS datapaths so that the kernel or userspace can
direct oversized packets to the slow path.

Thanks,

Ben.
Numan Siddique Nov. 7, 2018, 3:39 p.m. UTC | #2
On Tue, Nov 6, 2018, 2:26 AM Ben Pfaff <blp@ovn.org wrote:

> On Mon, Nov 05, 2018 at 09:45:10PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > This patch adds a new action 'check_pkt_larger' which checks if the
> > packet is larger than the given size and stores the result in the
> > destination register.
> >
> > Usage: check_pkt_larger:len->REGISTER
> > Eg. match=...,actions=check_pkt_larger:1442->NXM_NX_REG0[0],next;
> >
> > When translating this action, SLOW_ACTION is set so that datapath
> > flow is not added.
> >
> > This action is intended to be used by OVN to check the packet length
> > and generate an ICMP packet with type 3, code 4 and next hop mtu
> > in the logical router pipeline if the MTU of the physical interface
> > is lesser than the packet length. More information can be found here [1]
> >
> > TODO:
> >  - Add test case.
> >  - Change the action format from check_pkt_larger:len to
> check_pkt_larger(len)
> >
> > Request to suggest a better name for the action in case
> 'check_pkt_larger'
> > seems odd.
> >
> > [1] -
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>
> Thanks for working on this.
>
> It looks like this patch makes the assumption that every packet goes
> through userspace, but that isn't true.  The action needs to be
> implemented in the OVS datapaths so that the kernel or userspace can
> direct oversized packets to the slow path.
>
> Thanks,
>

Thanks for the comments. I'll address these before I submit a formal patch.

Thanks
Numan


> Ben.
>
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index e0588afad..2ec045fe5 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -123,6 +123,8 @@  struct vl_mff_map;
     OFPACT(NAT,             ofpact_nat,         ofpact, "nat")          \
     OFPACT(OUTPUT_TRUNC,    ofpact_output_trunc,ofpact, "output_trunc") \
     OFPACT(CLONE,           ofpact_nest,        actions, "clone")       \
+    OFPACT(CHECK_PKT_LARGER, ofpact_check_pkt_larger, ofpact,           \
+           "check_pkt_larger")                                          \
                                                                         \
     /* Debugging actions.                                               \
      *                                                                  \
@@ -572,6 +574,15 @@  struct ofpact_meter {
     uint32_t provider_meter_id;
 };
 
+/* OFPACT_CHECK_PKT_LARGER.
+ *
+ * Used for NXAST_CHECK_PKT_LARGER. */
+struct ofpact_check_pkt_larger {
+    struct ofpact ofpact;
+    uint16_t pkt_len;
+    struct mf_subfield dst;
+};
+
 /* OFPACT_WRITE_ACTIONS, OFPACT_CLONE.
  *
  * Used for OFPIT11_WRITE_ACTIONS, NXAST_CLONE. */
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index a80a4a308..5fd61de8a 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -355,6 +355,8 @@  enum ofp_raw_action_type {
     /* NX1.3+(48): void. */
     NXAST_RAW_DEC_NSH_TTL,
 
+    /* NX1.0+(49): struct nx_action_check_pkt_larger, ... */
+    OFPAT_RAW_CHECK_PKT_LARGER,
 /* ## ------------------ ## */
 /* ## Debugging actions. ## */
 /* ## ------------------ ## */
@@ -492,6 +494,7 @@  ofpact_next_flattened(const struct ofpact *ofpact)
     case OFPACT_ENCAP:
     case OFPACT_DECAP:
     case OFPACT_DEC_NSH_TTL:
+    case OFPACT_CHECK_PKT_LARGER:
         return ofpact_next(ofpact);
 
     case OFPACT_CLONE:
@@ -7399,6 +7402,94 @@  check_WRITE_METADATA(const struct ofpact_metadata *a OVS_UNUSED,
     return 0;
 }
 
+/* Check packet length action. */
+
+struct nx_action_check_pkt_larger {
+    ovs_be16 type;              /* OFPAT_VENDOR. */
+    ovs_be16 len;               /* 24. */
+    ovs_be32 vendor;            /* NX_VENDOR_ID. */
+    ovs_be16 subtype;           /* NXAST_OUTPUT_REG. */
+    uint8_t pad[6];
+    ovs_be16 pkt_len;           /* Length of the packet to check. */
+    ovs_be16 ofs_nbits;         /* (ofs << 6) | (n_bits - 1). */
+    ovs_be32 dst;               /* Destination register. */
+};
+
+OFP_ASSERT(sizeof(struct nx_action_check_pkt_larger) == 24);
+
+static enum ofperr
+decode_OFPAT_RAW_CHECK_PKT_LARGER(
+    const struct nx_action_check_pkt_larger *ncpl,
+    enum ofp_version ofp_version OVS_UNUSED, struct ofpbuf *out)
+{
+    struct ofpact_check_pkt_larger *check_pkt_larger =
+        ofpact_put_CHECK_PKT_LARGER(out);
+    check_pkt_larger->pkt_len = ntohs(ncpl->pkt_len);
+    const struct mf_field *mf = mf_from_nxm_header(ntohl(ncpl->dst), NULL);
+    check_pkt_larger->dst.field = mf;
+    check_pkt_larger->dst.ofs = nxm_decode_ofs(ncpl->ofs_nbits);
+    check_pkt_larger->dst.n_bits = nxm_decode_n_bits(ncpl->ofs_nbits);;
+    return 0;
+}
+
+static void
+encode_CHECK_PKT_LARGER(const struct ofpact_check_pkt_larger *check_pkt_larger,
+                 enum ofp_version ofp_version OVS_UNUSED,
+                 struct ofpbuf *out OVS_UNUSED)
+{
+    struct nx_action_check_pkt_larger *ncpl = put_OFPAT_CHECK_PKT_LARGER(out);
+    ncpl->pkt_len = htons(check_pkt_larger->pkt_len);
+    ncpl->ofs_nbits = nxm_encode_ofs_nbits(
+        check_pkt_larger->dst.ofs, check_pkt_larger->dst.n_bits);
+    if (check_pkt_larger->dst.field) {
+        ncpl->dst = htonl(nxm_header_from_mff(check_pkt_larger->dst.field));
+    }
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+parse_CHECK_PKT_LARGER(char *arg, const struct ofpact_parse_params *pp)
+{
+    char *value;
+    char *delim;
+    char *key;
+    char *error = set_field_split_str(arg, &key, &value, &delim);
+    if (error) {
+        return error;
+    }
+    delim[0] = '\0';
+    struct mf_subfield dst;
+    error = mf_parse_subfield(&dst, key);
+    if (error) {
+        return error;
+    }
+
+    struct ofpact_check_pkt_larger *check_pkt_larger =
+        ofpact_put_CHECK_PKT_LARGER(pp->ofpacts);
+    error = str_to_u16(value, NULL, &check_pkt_larger->pkt_len);
+    if (error) {
+        return error;
+    }
+    check_pkt_larger->dst = dst;
+    return NULL;
+}
+
+static void
+format_CHECK_PKT_LARGER(const struct ofpact_check_pkt_larger *a,
+                        const struct ofpact_format_params *fp)
+{
+    ds_put_format(fp->s, "%scheck_pkt_larger:%s%"PRIu32"->",
+                  colors.param, colors.end, a->pkt_len);
+    mf_format_subfield(&a->dst, fp->s);
+}
+
+static enum ofperr
+check_CHECK_PKT_LARGER(const struct ofpact_check_pkt_larger *a OVS_UNUSED,
+                       const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
+
+
 /* Goto-Table instruction. */
 
 static void
@@ -7685,6 +7776,7 @@  action_set_classify(const struct ofpact *a)
     case OFPACT_WRITE_METADATA:
     case OFPACT_DEBUG_RECIRC:
     case OFPACT_DEBUG_SLOW:
+    case OFPACT_CHECK_PKT_LARGER:
         return ACTION_SLOT_INVALID;
 
     default:
@@ -7884,6 +7976,7 @@  ovs_instruction_type_from_ofpact_type(enum ofpact_type type)
     case OFPACT_ENCAP:
     case OFPACT_DECAP:
     case OFPACT_DEC_NSH_TTL:
+    case OFPACT_CHECK_PKT_LARGER:
     default:
         return OVSINST_OFPIT11_APPLY_ACTIONS;
     }
@@ -8754,6 +8847,7 @@  ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port)
     case OFPACT_ENCAP:
     case OFPACT_DECAP:
     case OFPACT_DEC_NSH_TTL:
+    case OFPACT_CHECK_PKT_LARGER:
     default:
         return false;
     }
@@ -8990,7 +9084,6 @@  ofpacts_parse__(char *str, const struct ofpact_parse_params *pp,
         enum ofpact_type type;
         char *error = NULL;
         ofp_port_t port;
-
         if (ofpact_type_from_name(key, &type)) {
             error = ofpact_parse(type, value, pp);
             inst = ovs_instruction_type_from_ofpact_type(type);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 839fddd99..d9ea2feee 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5565,6 +5565,7 @@  reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len)
         case OFPACT_UNROLL_XLATE:
         case OFPACT_WRITE_ACTIONS:
         case OFPACT_WRITE_METADATA:
+        case OFPACT_CHECK_PKT_LARGER:
             break;
 
         case OFPACT_CT:
@@ -5873,6 +5874,7 @@  freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end,
         case OFPACT_CT:
         case OFPACT_CT_CLEAR:
         case OFPACT_NAT:
+        case OFPACT_CHECK_PKT_LARGER:
             /* These may not generate PACKET INs. */
             break;
 
@@ -6065,6 +6067,22 @@  compose_ct_clear_action(struct xlate_ctx *ctx)
     }
 }
 
+static void
+xlate_check_pkt_larger(struct xlate_ctx *ctx,
+                       struct ofpact_check_pkt_larger *check_pkt_larger)
+{
+    if (!ctx->xin->packet || !check_pkt_larger->dst.field) {
+        return;
+    }
+
+    union mf_subvalue value;
+    memset(&value, 0, sizeof value);
+    value.u8_val =
+        dp_packet_size(ctx->xin->packet) > check_pkt_larger->pkt_len;
+    mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow);
+    ctx->xout->slow |= SLOW_ACTION;
+}
+
 static void
 rewrite_flow_encap_ethernet(struct xlate_ctx *ctx,
                             struct flow *flow,
@@ -6387,6 +6405,7 @@  recirc_for_mpls(const struct ofpact *a, struct xlate_ctx *ctx)
     case OFPACT_WRITE_ACTIONS:
     case OFPACT_WRITE_METADATA:
     case OFPACT_GOTO_TABLE:
+    case OFPACT_CHECK_PKT_LARGER:
     default:
         break;
     }
@@ -6842,6 +6861,10 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_DEBUG_SLOW:
             ctx->xout->slow |= SLOW_ACTION;
             break;
+
+        case OFPACT_CHECK_PKT_LARGER:
+            xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a));
+            break;
         }
 
         /* Check if need to store this and the remaining actions for later