[ovs-dev,3/5] netdev-offload-tc: Handle check_pkt_len datapath action.

This change implements support for the check_pkt_len
action using the TC police action, which supports MTU
checking.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/netdev-offload-tc.c |  171 +++++++++++++++++-
 lib/tc.c                |  455 +++++++++++++++++++++++++++++++++++++++++++----
 lib/tc.h                |   12 +
 3 files changed, 594 insertions(+), 44 deletions(-)

On Mon, Mar 28, 2022 at 7:11 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
> This change implements support for the check_pkt_len
> action using the TC police action, which supports MTU
> checking.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

Hello Eelco, I've run into a few problems with this patch.

I've found that the following invocations will cause a core dump:

ovs-ofctl add-flow br0
"table=0,udp,actions=load:0x1->NXM_NX_REG1[],resubmit(,1)"
ovs-ofctl add-flow br0
"table=1,reg1=0x1,actions=check_pkt_larger(20)->NXM_NX_REG0[1],
check_pkt_larger(40)->NXM_NX_REG1[1],check_pkt_larger(60)->NXM_NX_REG2[1],
check_pkt_larger(80)->NXM_NX_REG3[1],check_pkt_larger(100)->NXM_NX_REG4[1],
check_pkt_larger(110)->NXM_NX_REG5[1],check_pkt_larger(120)->NXM_NX_REG6[1],
check_pkt_larger(140)->NXM_NX_REG7[1],check_pkt_larger(160)->NXM_NX_REG8[1],
check_pkt_larger(180)->NXM_NX_REG9[1],check_pkt_larger(200)->NXM_NX_REG10[1],
check_pkt_larger(220)->NXM_NX_REG11[1],resubmit(,2)"

Secondly, and I think this one is my mistake somehow, the following invocation

ovs-ofctl add-flow br0
"table=0,udp,actions=load:0x1->NXM_NX_REG1[],resubmit(,1)"
ovs-ofctl add-flow br0
"table=1,reg1=0x1,actions=check_pkt_larger(800)->NXM_NX_REG0[1],resubmit(,2)"
ovs-ofctl add-flow br0 "table=2,reg0=0x1,actions=drop"
ovs-ofctl add-flow br0 "table=2,reg0=0x0,actions=normal"

Will produce the following flow rule action in dpctl:

actions:check_pkt_len(size=800,gt(drop),le(drop))

For the first issue, I think there should be some protection against
over-recursion. And the second, I think I'm doing something wrong.


Cheers,
M

> ---
>  lib/netdev-offload-tc.c |  171 +++++++++++++++++-
>  lib/tc.c                |  455 +++++++++++++++++++++++++++++++++++++++++++----
>  lib/tc.h                |   12 +
>  3 files changed, 594 insertions(+), 44 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index a7838e503..bc15244cf 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -62,6 +62,13 @@ struct chain_node {
>      uint32_t chain;
>  };
>
> +static int
> +netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower,
> +                           struct offload_info *info,
> +                           const struct nlattr *actions, size_t actions_len,
> +                           bool *recirc_act, bool more_actions,
> +                           struct tc_action **need_jump_update);
> +
>  static bool
>  is_internal_port(const char *type)
>  {
> @@ -825,6 +832,45 @@ _parse_tc_flower_to_actions(struct tc_flower *flower, struct ofpbuf *buf,
>              nl_msg_put_u32(buf, OVS_ACTION_ATTR_RECIRC, action->chain);
>          }
>          break;
> +        case TC_ACT_POLICE_MTU: {
> +            size_t offset, act_offset;
> +            uint32_t jump;
> +
> +            offset = nl_msg_start_nested(buf,
> +                                         OVS_ACTION_ATTR_CHECK_PKT_LEN);
> +
> +            nl_msg_put_u16(buf, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
> +                           action->police.mtu);
> +
> +            act_offset = nl_msg_start_nested(
> +                buf, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
> +            i = _parse_tc_flower_to_actions(flower, buf, i + 1,
> +                                            action->police.result_jump);
> +            nl_msg_end_nested(buf, act_offset);
> +
> +            act_offset = nl_msg_start_nested(
> +                buf, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
> +
> +            jump = flower->actions[i - 1].jump_action;
> +            if (jump == JUMP_ACTION_STOP) {
> +                jump = max_index;
> +            }
> +            if (jump != 0) {
> +                i = _parse_tc_flower_to_actions(flower, buf, i, jump);
> +            }
> +            nl_msg_end_nested(buf, act_offset);
> +
> +            i--;
> +            nl_msg_end_nested(buf, offset);
> +        }
> +        break;
> +        }
> +
> +        if (action->jump_action && action->type != TC_ACT_POLICE_MTU) {
> +            /* If there is a jump, it means this was the end of an action
> +             * set and we need to end this branch. */
> +            i++;
> +            break;
>          }
>      }
>      return i;
> @@ -1578,11 +1624,121 @@ parse_match_ct_state_to_flower(struct tc_flower *flower, struct match *match)
>      }
>  }
>
> +
> +static int
> +parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower,
> +                           struct offload_info *info, struct tc_action *action,
> +                           const struct nlattr *nla, bool last_action,
> +                           struct tc_action **need_jump_update,
> +                           bool *recirc_act)
> +{
> +    struct tc_action *ge_jump_update = NULL, *le_jump_update = NULL;
> +    const struct nlattr *nl_actions;
> +    int err, le_offset, gt_offset;
> +    uint16_t pkt_len;
> +
> +    static const struct nl_policy ovs_cpl_policy[] = {
> +        [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = { .type = NL_A_U16 },
> +        [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = { .type = NL_A_NESTED },
> +        [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL]
> +            = { .type = NL_A_NESTED },
> +    };
> +    struct nlattr *a[ARRAY_SIZE(ovs_cpl_policy)];
> +
> +    if (!nl_parse_nested(nla, ovs_cpl_policy, a, ARRAY_SIZE(a))) {
> +        VLOG_INFO("Received invalid formatted OVS_ACTION_ATTR_CHECK_PKT_LEN!");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (!a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] ||
> +        !a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL]) {
> +        VLOG_INFO("Received invalid OVS_CHECK_PKT_LEN_ATTR_ACTION_IF_*!");
> +        return EOPNOTSUPP;
> +    }
> +
> +    pkt_len = nl_attr_get_u16(a[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]);
> +
> +    /* Add the police mtu action first in the allocated slot. */
> +    action->police.mtu = pkt_len;
> +    action->type = TC_ACT_POLICE_MTU;
> +    le_offset = flower->action_count++;
> +
> +    /* Parse and add the greater than action(s).
> +     * NOTE: The last_action parameter means that there are no more actions
> +     *       after the if () then ... else () case. */
> +    nl_actions = a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> +    err = netdev_tc_parse_nl_actions(netdev, flower, info,
> +                                     nl_attr_get(nl_actions),
> +                                     nl_attr_get_size(nl_actions),
> +                                     recirc_act, !last_action,
> +                                     &ge_jump_update);
> +    if (err) {
> +        return err;
> +    }
> +
> +    /* Update goto offset for le actions */
> +    flower->actions[le_offset].police.result_jump = flower->action_count;
> +
> +    gt_offset = flower->action_count;
> +
> +    /* Parse and add the less than action(s). */
> +    nl_actions = a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> +    err = netdev_tc_parse_nl_actions(netdev, flower, info,
> +                                     nl_attr_get(nl_actions),
> +                                     nl_attr_get_size(nl_actions),
> +                                     recirc_act, !last_action,
> +                                     &le_jump_update);
> +
> +    if (gt_offset == flower->action_count && last_action) {
> +        /* No le actions where added, fix gt offset */
> +        flower->actions[le_offset].police.result_jump = JUMP_ACTION_STOP;
> +    }
> +
> +    /* Update goto offset for gt actions to skip the le ones. */
> +    if (last_action) {
> +        flower->actions[gt_offset - 1].jump_action = JUMP_ACTION_STOP;
> +
> +        if (need_jump_update) {
> +            *need_jump_update = NULL;
> +        }
> +    } else {
> +        if (gt_offset == flower->action_count) {
> +            flower->actions[gt_offset - 1].jump_action = 0;
> +        } else {
> +            flower->actions[gt_offset - 1].jump_action = flower->action_count;
> +        }
> +        /* If we have nested if() else () the if actions jump over the else
> +         * and will end-up in the outer else () case, which it should have
> +         * skipped. To void this we return the "potential" inner if() goto to
> +         * need_jump_update, so it can be updated on return!
> +         */
> +        if (need_jump_update) {
> +            *need_jump_update = &flower->actions[gt_offset - 1];
> +        }
> +    }
> +
> +    if (le_jump_update != NULL) {
> +        le_jump_update->jump_action =
> +            flower->actions[gt_offset - 1].jump_action;
> +    }
> +    if (ge_jump_update != NULL) {
> +        ge_jump_update->jump_action =
> +            flower->actions[gt_offset - 1].jump_action;
> +    }
> +
> +    if (err) {
> +        return err;
> +    }
> +
> +    return 0;
> +}
> +
>  static int
>  netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower,
>                             struct offload_info *info,
>                             const struct nlattr *actions, size_t actions_len,
> -                           bool *recirc_act)
> +                           bool *recirc_act, bool more_actions,
> +                           struct tc_action **need_jump_update)
>  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>      const struct nlattr *nla;
> @@ -1702,6 +1858,16 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower,
>              action->type = TC_ACT_GOTO;
>              action->chain = 0;  /* 0 is reserved and not used by recirc. */
>              flower->action_count++;
> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CHECK_PKT_LEN) {
> +            err = parse_check_pkt_len_action(netdev, flower, info, action, nla,
> +                                             nl_attr_len_pad(nla,
> +                                                             left) >= left
> +                                             && !more_actions,
> +                                             need_jump_update,
> +                                             recirc_act);
> +            if (err) {
> +                return err;
> +            }
>          } else {
>              VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>                          nl_attr_type(nla));
> @@ -1974,7 +2140,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>
>      /* Parse all (nested) actions. */
>      err = netdev_tc_parse_nl_actions(netdev, &flower, info,
> -                                     actions, actions_len, &recirc_act);
> +                                     actions, actions_len, &recirc_act,
> +                                     false, NULL);
>      if (err) {
>          return err;
>      }
> diff --git a/lib/tc.c b/lib/tc.c
> index df73a43d4..812b0432c 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -994,6 +994,18 @@ nl_parse_flower_flags(struct nlattr **attrs, struct tc_flower *flower)
>      flower->offloaded_state = nl_get_flower_offloaded_state(attrs);
>  }
>
> +static void
> +nl_parse_action_pc(uint32_t action_pc, struct tc_action *action)
> +{
> +    if (action_pc == TC_ACT_STOLEN) {
> +        action->jump_action = JUMP_ACTION_STOP;
> +    } else if (action_pc & TC_ACT_JUMP) {
> +        action->jump_action = action_pc & TC_ACT_EXT_VAL_MASK;
> +    } else {
> +        action->jump_action = 0;
> +    }
> +}
> +
>  static const struct nl_policy pedit_policy[] = {
>              [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC,
>                                       .min_len = sizeof(struct tc_pedit),
> @@ -1093,6 +1105,7 @@ nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
>
>      action->type = TC_ACT_PEDIT;
>
> +    nl_parse_action_pc(pe->action, action);
>      return 0;
>  }
>
> @@ -1267,6 +1280,7 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower)
>          if (err) {
>              return err;
>          }
> +        nl_parse_action_pc(tun->action, action);
>      } else if (tun->t_action == TCA_TUNNEL_KEY_ACT_RELEASE) {
>          flower->tunnel = true;
>      } else {
> @@ -1303,7 +1317,11 @@ get_user_hz(void)
>  static void
>  nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
>  {
> -    flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
> +    uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
> +
> +    if (flower->lastused < lastused) {
> +        flower->lastused = lastused;
> +    }
>  }
>
>  static int
> @@ -1328,6 +1346,7 @@ nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower)
>          action = &flower->actions[flower->action_count++];
>          action->chain = p->action & TC_ACT_EXT_VAL_MASK;
>          action->type = TC_ACT_GOTO;
> +        nl_parse_action_pc(p->action, action);
>      } else if (p->action != TC_ACT_SHOT) {
>          VLOG_ERR_RL(&error_rl, "unknown gact action: %d", p->action);
>          return EINVAL;
> @@ -1386,8 +1405,9 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower)
>
>      mirred_tm = mirred_attrs[TCA_MIRRED_TM];
>      tm = nl_attr_get_unspec(mirred_tm, sizeof *tm);
> -    nl_parse_tcf(tm, flower);
>
> +    nl_parse_tcf(tm, flower);
> +    nl_parse_action_pc(m->action, action);
>      return 0;
>  }
>
> @@ -1516,6 +1536,7 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower *flower)
>      }
>      action->type = TC_ACT_CT;
>
> +    nl_parse_action_pc(ct->action, action);
>      return 0;
>  }
>
> @@ -1561,6 +1582,8 @@ nl_parse_act_vlan(struct nlattr *options, struct tc_flower *flower)
>                      v->action, v->v_action);
>          return EINVAL;
>      }
> +
> +    nl_parse_action_pc(v->action, action);
>      return 0;
>  }
>
> @@ -1654,6 +1677,7 @@ nl_parse_act_mpls(struct nlattr *options, struct tc_flower *flower)
>          return EINVAL;
>      }
>
> +    nl_parse_action_pc(m->action, action);
>      return 0;
>  }
>
> @@ -1694,9 +1718,60 @@ nl_parse_act_csum(struct nlattr *options, struct tc_flower *flower)
>          return EINVAL;
>      }
>
> +    /* The action_pc should be set on the previous action. */
> +    if (flower->action_count < TCA_ACT_MAX_NUM) {
> +        struct tc_action *action = &flower->actions[flower->action_count];
> +
> +        nl_parse_action_pc(c->action, action);
> +    }
>      return 0;
>  }
>
> +static const struct nl_policy police_policy[] = {
> +    [TCA_POLICE_TBF] = { .type = NL_A_UNSPEC,
> +                         .min_len = sizeof(struct tc_police),
> +                         .optional = false, },
> +    [TCA_POLICE_RESULT] = { .type = NL_A_U32, .optional = false, },
> +    [TCA_POLICE_TM]     = { .type = NL_A_UNSPEC,
> +                            .min_len = sizeof(struct tcf_t),
> +                            .optional = false, },
> +};
> +
> +static int
> +nl_parse_act_police_mtu(struct nlattr *options, struct tc_flower *flower)
> +{
> +    struct nlattr *police_attrs[ARRAY_SIZE(police_policy)];
> +    const struct tc_police *police;
> +    struct nlattr *police_result;
> +    struct tc_action *action;
> +    const struct tcf_t *tm;
> +    uint32_t action_pc;
> +
> +    if (!nl_parse_nested(options, police_policy, police_attrs,
> +                         ARRAY_SIZE(police_policy))) {
> +        VLOG_ERR_RL(&error_rl, "Failed to parse police action options");
> +        return EPROTO;
> +    }
> +    police = nl_attr_get_unspec(police_attrs[TCA_CSUM_PARMS], sizeof *police);
> +    police_result = police_attrs[TCA_POLICE_RESULT];
> +
> +    action = &flower->actions[flower->action_count++];
> +    action->type = TC_ACT_POLICE_MTU;
> +    action_pc = nl_attr_get_u32(police_result);
> +    if (action_pc & TC_ACT_JUMP) {
> +        action->police.result_jump = action_pc & TC_ACT_EXT_VAL_MASK;
> +    } else {
> +        action->police.result_jump = JUMP_ACTION_STOP;
> +    }
> +    action->police.mtu = police->mtu;
> +
> +    tm = nl_attr_get_unspec(police_attrs[TCA_POLICE_TM], sizeof *tm);
> +    nl_parse_tcf(tm, flower);
> +    nl_parse_action_pc(police->action, action);
> +    return 0;
> +}
> +
> +
>  static const struct nl_policy act_policy[] = {
>      [TCA_ACT_KIND] = { .type = NL_A_STRING, .optional = false, },
>      [TCA_ACT_COOKIE] = { .type = NL_A_UNSPEC, .optional = true, },
> @@ -1715,7 +1790,7 @@ static const struct nl_policy stats_policy[] = {
>
>  static int
>  nl_parse_single_action(struct nlattr *action, struct tc_flower *flower,
> -                       bool terse)
> +                       bool terse, bool *csum)
>  {
>      struct nlattr *act_options;
>      struct nlattr *act_stats;
> @@ -1737,6 +1812,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower,
>          return EPROTO;
>      }
>
> +    *csum = false;
>      act_kind = nl_attr_get_string(action_attrs[TCA_ACT_KIND]);
>      act_options = action_attrs[TCA_ACT_OPTIONS];
>      act_cookie = action_attrs[TCA_ACT_COOKIE];
> @@ -1757,10 +1833,13 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower,
>          err = nl_parse_act_pedit(act_options, flower);
>      } else if (!strcmp(act_kind, "csum")) {
>          nl_parse_act_csum(act_options, flower);
> +        *csum = true;
>      } else if (!strcmp(act_kind, "skbedit")) {
>          /* Added for TC rule only (not in OvS rule) so ignore. */
>      } else if (!strcmp(act_kind, "ct")) {
>          nl_parse_act_ct(act_options, flower);
> +    } else if (!strcmp(act_kind, "police")) {
> +        nl_parse_act_police_mtu(act_options, flower);
>      } else {
>          VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind);
>          err = EINVAL;
> @@ -1818,6 +1897,10 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower,
>      static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {};
>      struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)];
>      const int max_size = ARRAY_SIZE(actions_orders_policy);
> +    int previous_action_count = 0;
> +    bool need_jump_adjust = false;
> +    int jump_adjust = 0;
> +    bool csum = false;
>
>      for (int i = TCA_ACT_MIN_PRIO; i < max_size; i++) {
>          actions_orders_policy[i].type = NL_A_NESTED;
> @@ -1838,7 +1921,70 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower,
>                  VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM);
>                  return EOPNOTSUPP;
>              }
> -            err = nl_parse_single_action(actions_orders[i], flower, terse);
> +            err = nl_parse_single_action(actions_orders[i], flower, terse,
> +                                         &csum);
> +
> +            if (flower->action_count == previous_action_count) {
> +
> +                struct tc_action *action;
> +
> +                /* We had no update on the TC action count, which means
> +                 * we had a none TC type action. So need to adjust existing
> +                 * jump offsets. */
> +                jump_adjust++;
> +
> +                if (need_jump_adjust || (csum && flower->action_count > 0)) {
> +
> +                    if (csum && flower->action_count > 0) {
> +                        /* The csum action is special as it might carry
> +                         * a jump count for the previous TC_ACT and therefore
> +                         * should be adjusted with jump_adjust as it got
> +                         * copied. */
> +                        action = &flower->actions[flower->action_count - 1];
> +                        if (action->jump_action
> +                            && action->jump_action != JUMP_ACTION_STOP) {
> +                            action->jump_action -= (jump_adjust - 1);
> +                        }
> +                    }
> +
> +                    for (int j = 0; j < flower->action_count; j++) {
> +                        action = &flower->actions[j];
> +
> +                        if (action->type == TC_ACT_POLICE_MTU
> +                            && action->police.result_jump != JUMP_ACTION_STOP
> +                            && (action->police.result_jump - 1) >
> +                            flower->action_count) {
> +
> +                            action->police.result_jump--;
> +                        }
> +
> +                        if (action->jump_action
> +                            && action->jump_action != JUMP_ACTION_STOP
> +                            && (action->jump_action - 1) >
> +                            flower->action_count) {
> +
> +                            action->jump_action--;
> +                        }
> +                    }
> +                }
> +            } else {
> +                struct tc_action *action;
> +
> +                action = &flower->actions[previous_action_count];
> +                if (action->type == TC_ACT_POLICE_MTU &&
> +                    action->police.result_jump != JUMP_ACTION_STOP) {
> +                    action->police.result_jump -= jump_adjust;
> +                    need_jump_adjust = true;
> +                }
> +
> +                if (action->jump_action
> +                    && action->jump_action != JUMP_ACTION_STOP) {
> +                    action->jump_action -= jump_adjust;
> +                    need_jump_adjust = true;
> +                }
> +
> +                previous_action_count = flower->action_count;
> +            }
>
>              if (err) {
>                  return err;
> @@ -2031,14 +2177,14 @@ tc_get_tc_cls_policy(enum tc_offload_policy policy)
>  }
>
>  static void
> -nl_msg_put_act_csum(struct ofpbuf *request, uint32_t flags)
> +nl_msg_put_act_csum(struct ofpbuf *request, uint32_t flags, uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "csum");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
>      {
> -        struct tc_csum parm = { .action = TC_ACT_PIPE,
> +        struct tc_csum parm = { .action = action_pc,
>                                  .update_flags = flags };
>
>          nl_msg_put_unspec(request, TCA_CSUM_PARMS, &parm, sizeof parm);
> @@ -2048,7 +2194,7 @@ nl_msg_put_act_csum(struct ofpbuf *request, uint32_t flags)
>
>  static void
>  nl_msg_put_act_pedit(struct ofpbuf *request, struct tc_pedit *parm,
> -                     struct tc_pedit_key_ex *ex)
> +                     struct tc_pedit_key_ex *ex, uint32_t action_pc)
>  {
>      size_t ksize = sizeof *parm + parm->nkeys * sizeof(struct tc_pedit_key);
>      size_t offset, offset_keys_ex, offset_key;
> @@ -2057,7 +2203,7 @@ nl_msg_put_act_pedit(struct ofpbuf *request, struct tc_pedit *parm,
>      nl_msg_put_string(request, TCA_ACT_KIND, "pedit");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
>      {
> -        parm->action = TC_ACT_PIPE;
> +        parm->action = action_pc;
>
>          nl_msg_put_unspec(request, TCA_PEDIT_PARMS_EX, parm, ksize);
>          offset_keys_ex = nl_msg_start_nested(request, TCA_PEDIT_KEYS_EX);
> @@ -2074,14 +2220,14 @@ nl_msg_put_act_pedit(struct ofpbuf *request, struct tc_pedit *parm,
>
>  static void
>  nl_msg_put_act_push_vlan(struct ofpbuf *request, ovs_be16 tpid,
> -                         uint16_t vid, uint8_t prio)
> +                         uint16_t vid, uint8_t prio, uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "vlan");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
>      {
> -        struct tc_vlan parm = { .action = TC_ACT_PIPE,
> +        struct tc_vlan parm = { .action = action_pc,
>                                  .v_action = TCA_VLAN_ACT_PUSH };
>
>          nl_msg_put_unspec(request, TCA_VLAN_PARMS, &parm, sizeof parm);
> @@ -2093,14 +2239,14 @@ nl_msg_put_act_push_vlan(struct ofpbuf *request, ovs_be16 tpid,
>  }
>
>  static void
> -nl_msg_put_act_pop_vlan(struct ofpbuf *request)
> +nl_msg_put_act_pop_vlan(struct ofpbuf *request, uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "vlan");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
>      {
> -        struct tc_vlan parm = { .action = TC_ACT_PIPE,
> +        struct tc_vlan parm = { .action = action_pc,
>                                  .v_action = TCA_VLAN_ACT_POP };
>
>          nl_msg_put_unspec(request, TCA_VLAN_PARMS, &parm, sizeof parm);
> @@ -2109,14 +2255,15 @@ nl_msg_put_act_pop_vlan(struct ofpbuf *request)
>  }
>
>  static void
> -nl_msg_put_act_pop_mpls(struct ofpbuf *request, ovs_be16 proto)
> +nl_msg_put_act_pop_mpls(struct ofpbuf *request, ovs_be16 proto,
> +                        uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "mpls");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS | NLA_F_NESTED);
>      {
> -        struct tc_mpls parm = { .action = TC_ACT_PIPE,
> +        struct tc_mpls parm = { .action = action_pc,
>                                  .m_action = TCA_MPLS_ACT_POP };
>
>          nl_msg_put_unspec(request, TCA_MPLS_PARMS, &parm, sizeof parm);
> @@ -2127,14 +2274,15 @@ nl_msg_put_act_pop_mpls(struct ofpbuf *request, ovs_be16 proto)
>
>  static void
>  nl_msg_put_act_push_mpls(struct ofpbuf *request, ovs_be16 proto,
> -                         uint32_t label, uint8_t tc, uint8_t ttl, uint8_t bos)
> +                         uint32_t label, uint8_t tc, uint8_t ttl, uint8_t bos,
> +                         uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "mpls");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS | NLA_F_NESTED);
>      {
> -        struct tc_mpls parm = { .action = TC_ACT_PIPE,
> +        struct tc_mpls parm = { .action = action_pc,
>                                  .m_action = TCA_MPLS_ACT_PUSH };
>
>          nl_msg_put_unspec(request, TCA_MPLS_PARMS, &parm, sizeof parm);
> @@ -2149,14 +2297,14 @@ nl_msg_put_act_push_mpls(struct ofpbuf *request, ovs_be16 proto,
>
>  static void
>  nl_msg_put_act_set_mpls(struct ofpbuf *request, uint32_t label, uint8_t tc,
> -                        uint8_t ttl, uint8_t bos)
> +                        uint8_t ttl, uint8_t bos, uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "mpls");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS | NLA_F_NESTED);
>      {
> -        struct tc_mpls parm = { .action = TC_ACT_PIPE,
> +        struct tc_mpls parm = { .action = action_pc,
>                                  .m_action = TCA_MPLS_ACT_MODIFY };
>
>          nl_msg_put_unspec(request, TCA_MPLS_PARMS, &parm, sizeof parm);
> @@ -2225,14 +2373,14 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, bool id_present,
>                                struct in6_addr *ipv6_dst,
>                                ovs_be16 tp_dst, uint8_t tos, uint8_t ttl,
>                                struct tun_metadata tun_metadata,
> -                              uint8_t no_csum)
> +                              uint8_t no_csum, uint32_t action_pc)
>  {
>      size_t offset;
>
>      nl_msg_put_string(request, TCA_ACT_KIND, "tunnel_key");
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
>      {
> -        struct tc_tunnel_key tun = { .action = TC_ACT_PIPE,
> +        struct tc_tunnel_key tun = { .action = action_pc,
>                                       .t_action = TCA_TUNNEL_KEY_ACT_SET };
>
>          nl_msg_put_unspec(request, TCA_TUNNEL_KEY_PARMS, &tun, sizeof tun);
> @@ -2285,7 +2433,8 @@ nl_msg_put_act_gact(struct ofpbuf *request, uint32_t chain)
>  }
>
>  static void
> -nl_msg_put_act_ct(struct ofpbuf *request, struct tc_action *action)
> +nl_msg_put_act_ct(struct ofpbuf *request, struct tc_action *action,
> +                  uint32_t action_pc)
>  {
>      uint16_t ct_action = 0;
>      size_t offset;
> @@ -2294,7 +2443,7 @@ nl_msg_put_act_ct(struct ofpbuf *request, struct tc_action *action)
>      offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS | NLA_F_NESTED);
>      {
>          struct tc_ct ct = {
> -                .action = TC_ACT_PIPE,
> +            .action = action_pc,
>          };
>
>          if (!action->ct.clear) {
> @@ -2499,10 +2648,57 @@ csum_update_flag(struct tc_flower *flower,
>      return EOPNOTSUPP;
>  }
>
> +static bool
> +rewrite_pedits_need_csum_update(struct tc_action *action)
> +{
> +    int i, j;
> +
> +    for (i = 0; i < ARRAY_SIZE(flower_pedit_map); i++) {
> +        struct flower_key_to_pedit *m = &flower_pedit_map[i];
> +        ovs_be32 *mask, *data, first_word_mask, last_word_mask;
> +        int cnt = 0, cur_offset = 0;
> +
> +        if (!m->size) {
> +            continue;
> +        }
> +
> +        calc_offsets(action, m, &cur_offset, &cnt, &last_word_mask,
> +                     &first_word_mask, &mask, &data);
> +
> +        for (j = 0; j < cnt; j++,  mask++) {
> +            ovs_be32 mask_word = *mask;
> +
> +            if (j == 0) {
> +                mask_word &= first_word_mask;
> +            }
> +            if (j == cnt - 1) {
> +                mask_word &= last_word_mask;
> +            }
> +            if (!mask_word) {
> +                continue;
> +            }
> +
> +            switch (m->htype) {
> +            case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
> +            case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
> +            case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
> +            case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
> +                return true;
> +            case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK:
> +            case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
> +            case __PEDIT_HDR_TYPE_MAX:
> +                break;
> +            }
> +        }
> +    }
> +    return false;
> +}
> +
>  static int
>  nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
>                                   struct tc_flower *flower,
> -                                 struct tc_action *action)
> +                                 struct tc_action *action,
> +                                 uint32_t action_pc)
>  {
>      struct {
>          struct tc_pedit sel;
> @@ -2569,7 +2765,8 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
>              }
>          }
>      }
> -    nl_msg_put_act_pedit(request, &sel.sel, sel.keys_ex);
> +    nl_msg_put_act_pedit(request, &sel.sel, sel.keys_ex,
> +                         flower->csum_update_flags ? TC_ACT_PIPE : action_pc);
>
>      return 0;
>  }
> @@ -2590,7 +2787,7 @@ nl_msg_put_flower_acts_release(struct ofpbuf *request, uint16_t act_index)
>   * last pedit action. */
>  static void
>  nl_msg_put_csum_act(struct ofpbuf *request, struct tc_flower *flower,
> -                    uint16_t *act_index)
> +                    uint32_t action_pc, uint16_t *act_index)
>  {
>      size_t act_offset;
>
> @@ -2600,7 +2797,7 @@ nl_msg_put_csum_act(struct ofpbuf *request, struct tc_flower *flower,
>      }
>
>      act_offset = nl_msg_start_nested(request, (*act_index)++);
> -    nl_msg_put_act_csum(request, flower->csum_update_flags);
> +    nl_msg_put_act_csum(request, flower->csum_update_flags, action_pc);
>      nl_msg_put_act_flags(request);
>      nl_msg_end_nested(request, act_offset);
>
> @@ -2608,6 +2805,124 @@ nl_msg_put_csum_act(struct ofpbuf *request, struct tc_flower *flower,
>      flower->csum_update_flags = 0;
>  }
>
> +static int
> +get_action_index_for_tc_actions(struct tc_flower *flower, uint16_t act_index,
> +                                struct tc_action *action, int action_count,
> +                                bool tunnel_key_released)
> +{
> +    bool need_csum = false;
> +
> +    if (action_count < 0) {
> +        /* Only forward jumps are supported */
> +        return -EINVAL;
> +    }
> +
> +    for (int i = 0; i < action_count; i++, action++) {
> +        if (action->type != TC_ACT_PEDIT && need_csum) {
> +            need_csum = false;
> +            act_index++;
> +        }
> +
> +        switch (action->type) {
> +
> +        case TC_ACT_OUTPUT:
> +            if (!tunnel_key_released && flower->tunnel) {
> +                act_index++;
> +                tunnel_key_released = true;
> +            }
> +            if (action->out.ingress) {
> +                act_index++;
> +            }
> +            act_index++;
> +            break;
> +
> +        case TC_ACT_ENCAP:
> +            if (!tunnel_key_released && flower->tunnel) {
> +                act_index++;
> +                tunnel_key_released = true;
> +            }
> +            act_index++;
> +            break;
> +
> +        case TC_ACT_PEDIT:
> +            if (!need_csum) {
> +                need_csum = rewrite_pedits_need_csum_update(action);
> +            }
> +            if (i == (action_count - 1) && need_csum) {
> +                need_csum = false;
> +                act_index++;
> +            }
> +            act_index++;
> +            break;
> +
> +        case TC_ACT_POLICE_MTU:
> +        case TC_ACT_VLAN_POP:
> +        case TC_ACT_VLAN_PUSH:
> +        case TC_ACT_MPLS_POP:
> +        case TC_ACT_MPLS_PUSH:
> +        case TC_ACT_MPLS_SET:
> +        case TC_ACT_GOTO:
> +        case TC_ACT_CT:
> +            /* Increase act_index by one if we are sure this type of action
> +             * will only add one tc action in the kernel. */
> +            act_index++;
> +            break;
> +
> +            /* If we can't determine how many tc actions will be added by the
> +             * kernel return -EOPNOTSUPP.
> +             *
> +             * Please do NOT add a default case. */
> +        }
> +    }
> +
> +    return act_index;
> +}
> +
> +static int
> +nl_msg_put_act_police_mtu(struct ofpbuf *request, struct tc_flower *flower,
> +                          struct tc_action *action, uint32_t action_pc,
> +                          int action_index, uint16_t act_index, bool released)
> +{
> +    uint32_t tc_action;
> +    size_t offset;
> +
> +    if (action->police.result_jump != JUMP_ACTION_STOP) {
> +        int jump_index;
> +        int action_count = action->police.result_jump - action_index - 1;
> +
> +        jump_index = get_action_index_for_tc_actions(flower,
> +                                                     act_index - 1,
> +                                                     action + 1,
> +                                                     action_count,
> +                                                     released);
> +        if (jump_index < 0) {
> +            return -jump_index;
> +        }
> +        tc_action = TC_ACT_JUMP | (jump_index & TC_ACT_EXT_VAL_MASK);
> +    } else {
> +        tc_action = TC_ACT_STOLEN;
> +    }
> +
> +    nl_msg_put_string(request, TCA_ACT_KIND, "police");
> +    offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> +    {
> +        struct tc_police p = { .action = action_pc,
> +            .mtu = action->police.mtu };
> +
> +        nl_msg_put_unspec(request, TCA_POLICE_TBF, &p, sizeof p);
> +
> +        /* The value in jump_action is the total number of TC_ACT_*
> +         * we need to jump, not the actual number of TCA_ACT_KIND
> +         * (act_index) actions. As certain TC_ACT_* actions can be
> +         * translated into multiple TCA_ACT_KIND ones.
> +         *
> +         * See nl_msg_put_flower_acts() below for more details. */
> +        nl_msg_put_u32(request, TCA_POLICE_RESULT, tc_action);
> +    }
> +    nl_msg_end_nested(request, offset);
> +    return 0;
> +}
> +
>  static int
>  nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>  {
> @@ -2621,17 +2936,59 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>      offset = nl_msg_start_nested(request, TCA_FLOWER_ACT);
>      {
>          int error;
> +        uint32_t prev_action_pc = TC_ACT_PIPE;
>
>          action = flower->actions;
>          for (i = 0; i < flower->action_count; i++, action++) {
> -            if (action->type != TC_ACT_PEDIT) {
> -                nl_msg_put_csum_act(request, flower, &act_index);
> +            uint32_t action_pc; /* Programmatic Control */
> +
> +            if (!action->jump_action) {
> +                action_pc = TC_ACT_PIPE;
> +            } else if (action->jump_action == JUMP_ACTION_STOP) {
> +                action_pc = TC_ACT_STOLEN;
> +            } else {
> +                /* The value in jump_action is the total number of TC_ACT_*
> +                 * we need to jump, not the actual number of TCA_ACT_KIND
> +                 * (act_index) actions. As certain TC_ACT_* actions can be
> +                 * translated into multiple TCA_ACT_KIND ones.
> +                 *
> +                 * If we can determine the number of actual actions being
> +                 * inserted we will update the count, if not we will return
> +                 * -EOPNOTSUPP.
> +                 */
> +                int jump_index;
> +                int act_index_start = act_index - 1;
> +                int action_count = (action->jump_action &
> +                                    TC_ACT_EXT_VAL_MASK) - i;
> +
> +                if (flower->csum_update_flags &&
> +                    (action->type != TC_ACT_PEDIT
> +                     || prev_action_pc & TC_ACT_JUMP)) {
> +                    act_index_start++;
> +                }
> +
> +                jump_index = get_action_index_for_tc_actions(flower,
> +                                                             act_index_start,
> +                                                             action,
> +                                                             action_count,
> +                                                             released);
> +                if (jump_index < 0) {
> +                    return -jump_index;
> +                }
> +
> +                action_pc = TC_ACT_JUMP | jump_index;
>              }
> +
> +            if (action->type != TC_ACT_PEDIT || prev_action_pc & TC_ACT_JUMP) {
> +                nl_msg_put_csum_act(request, flower, prev_action_pc,
> +                                    &act_index);
> +            }
> +
>              switch (action->type) {
>              case TC_ACT_PEDIT: {
>                  act_offset = nl_msg_start_nested(request, act_index++);
>                  error = nl_msg_put_flower_rewrite_pedits(request, flower,
> -                                                         action);
> +                                                         action, action_pc);
>                  if (error) {
>                      return error;
>                  }
> @@ -2639,7 +2996,8 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>
>                  if (i == flower->action_count - 1) {
>                      /* If this is the last action check csum calc again. */
> -                    nl_msg_put_csum_act(request, flower, &act_index);
> +                    nl_msg_put_csum_act(request, flower, action_pc,
> +                                        &act_index);
>                  }
>              }
>              break;
> @@ -2660,14 +3018,15 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>                                                action->encap.tos,
>                                                action->encap.ttl,
>                                                action->encap.data,
> -                                              action->encap.no_csum);
> +                                              action->encap.no_csum,
> +                                              action_pc);
>                  nl_msg_put_act_flags(request);
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
>              case TC_ACT_VLAN_POP: {
>                  act_offset = nl_msg_start_nested(request, act_index++);
> -                nl_msg_put_act_pop_vlan(request);
> +                nl_msg_put_act_pop_vlan(request, action_pc);
>                  nl_msg_put_act_flags(request);
>                  nl_msg_end_nested(request, act_offset);
>              }
> @@ -2677,14 +3036,16 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>                  nl_msg_put_act_push_vlan(request,
>                                           action->vlan.vlan_push_tpid,
>                                           action->vlan.vlan_push_id,
> -                                         action->vlan.vlan_push_prio);
> +                                         action->vlan.vlan_push_prio,
> +                                         action_pc);
>                  nl_msg_put_act_flags(request);
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
>              case TC_ACT_MPLS_POP: {
>                  act_offset = nl_msg_start_nested(request, act_index++);
> -                nl_msg_put_act_pop_mpls(request, action->mpls.proto);
> +                nl_msg_put_act_pop_mpls(request, action->mpls.proto,
> +                                        action_pc);
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
> @@ -2692,7 +3053,8 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>                  act_offset = nl_msg_start_nested(request, act_index++);
>                  nl_msg_put_act_push_mpls(request, action->mpls.proto,
>                                           action->mpls.label, action->mpls.tc,
> -                                         action->mpls.ttl, action->mpls.bos);
> +                                         action->mpls.ttl, action->mpls.bos,
> +                                         action_pc);
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
> @@ -2700,7 +3062,7 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>                  act_offset = nl_msg_start_nested(request, act_index++);
>                  nl_msg_put_act_set_mpls(request, action->mpls.label,
>                                          action->mpls.tc, action->mpls.ttl,
> -                                        action->mpls.bos);
> +                                        action->mpls.bos, action_pc);
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
> @@ -2735,12 +3097,13 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>                          nl_msg_put_act_mirred(request, ifindex, TC_ACT_STOLEN,
>                                                TCA_EGRESS_REDIR);
>                      }
> +                    action->jump_action = JUMP_ACTION_STOP;
>                  } else {
>                      if (ingress) {
> -                        nl_msg_put_act_mirred(request, ifindex, TC_ACT_PIPE,
> +                        nl_msg_put_act_mirred(request, ifindex, action_pc,
>                                                TCA_INGRESS_MIRROR);
>                      } else {
> -                        nl_msg_put_act_mirred(request, ifindex, TC_ACT_PIPE,
> +                        nl_msg_put_act_mirred(request, ifindex, action_pc,
>                                                TCA_EGRESS_MIRROR);
>                      }
>                  }
> @@ -2768,12 +3131,26 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>              break;
>              case TC_ACT_CT: {
>                  act_offset = nl_msg_start_nested(request, act_index++);
> -                nl_msg_put_act_ct(request, action);
> +                nl_msg_put_act_ct(request, action, action_pc);
>                  nl_msg_put_act_cookie(request, &flower->act_cookie);
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
> +            case TC_ACT_POLICE_MTU: {
> +                act_offset = nl_msg_start_nested(request, act_index++);
> +                if (nl_msg_put_act_police_mtu(request, flower, action,
> +                                              action_pc, i, act_index,
> +                                              released)) {
> +                    return -EOPNOTSUPP;
> +                }
> +                nl_msg_put_act_cookie(request, &flower->act_cookie);
> +                nl_msg_put_act_flags(request);
> +                nl_msg_end_nested(request, act_offset);
> +            }
> +            break;
>              }
> +
> +            prev_action_pc = action_pc;
>          }
>      }
>
> diff --git a/lib/tc.h b/lib/tc.h
> index d6cdddd16..7ccd71d01 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -174,6 +174,7 @@ enum tc_action_type {
>      TC_ACT_MPLS_SET,
>      TC_ACT_GOTO,
>      TC_ACT_CT,
> +    TC_ACT_POLICE_MTU,
>  };
>
>  enum nat_type {
> @@ -256,14 +257,19 @@ struct tc_action {
>              bool force;
>              bool commit;
>          } ct;
> -
>          struct {
>              struct tc_flower_key key;
>              struct tc_flower_key mask;
>          } rewrite;
> -     };
> +        struct {
> +            uint32_t result_jump;
> +            uint16_t mtu;
> +        } police;
> +    };
>
> -     enum tc_action_type type;
> +    enum tc_action_type type;
> +    uint32_t jump_action;
> +#define JUMP_ACTION_STOP 0xffffffff
>  };
>
>  /* assert that if we overflow with a masked write of uint32_t to the last byte
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 1 Apr 2022, at 19:54, Mike Pattrick wrote:

> On Mon, Mar 28, 2022 at 7:11 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>>
>> This change implements support for the check_pkt_len
>> action using the TC police action, which supports MTU
>> checking.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>
> Hello Eelco, I've run into a few problems with this patch.
>
> I've found that the following invocations will cause a core dump:
>
> ovs-ofctl add-flow br0
> "table=0,udp,actions=load:0x1->NXM_NX_REG1[],resubmit(,1)"
> ovs-ofctl add-flow br0
> "table=1,reg1=0x1,actions=check_pkt_larger(20)->NXM_NX_REG0[1],
> check_pkt_larger(40)->NXM_NX_REG1[1],check_pkt_larger(60)->NXM_NX_REG2[1],
> check_pkt_larger(80)->NXM_NX_REG3[1],check_pkt_larger(100)->NXM_NX_REG4[1],
> check_pkt_larger(110)->NXM_NX_REG5[1],check_pkt_larger(120)->NXM_NX_REG6[1],
> check_pkt_larger(140)->NXM_NX_REG7[1],check_pkt_larger(160)->NXM_NX_REG8[1],
> check_pkt_larger(180)->NXM_NX_REG9[1],check_pkt_larger(200)->NXM_NX_REG10[1],
> check_pkt_larger(220)->NXM_NX_REG11[1],resubmit(,2)"

As discussed offline, this is not related to this patchset, and is already present in the current version.

You were also kind enough to offer to fix this and sent a patch upstream (maybe you could also include a general test case for this?).

> Secondly, and I think this one is my mistake somehow, the following invocation
>
> ovs-ofctl add-flow br0
> "table=0,udp,actions=load:0x1->NXM_NX_REG1[],resubmit(,1)"
> ovs-ofctl add-flow br0
> "table=1,reg1=0x1,actions=check_pkt_larger(800)->NXM_NX_REG0[1],resubmit(,2)"
> ovs-ofctl add-flow br0 "table=2,reg0=0x1,actions=drop"
> ovs-ofctl add-flow br0 "table=2,reg0=0x0,actions=normal"
>
> Will produce the following flow rule action in dpctl:
>
> actions:check_pkt_len(size=800,gt(drop),le(drop))

I added a quick testcase as follows and it seems to work:

[ebuild:~/...DK_v21.11/ovs_github]$ git diff
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 4cde53e2e..f7b4a5118 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -573,6 +573,23 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 64 10.1.1.2 | FORMAT_PING]
 AT_CHECK_ACTIONS([check_pkt_len(size=200,gt(5),le(check_pkt_len(size=100,gt(5),le(4)))),3])


+AT_CHECK([ovs-appctl revalidator/wait], [0])
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_DATA([flows.txt], [dnl
+table=0,in_port=2 actions=output:1
+table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1)
+table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
+table=4,in_port=1,reg0=0x1 actions=drop
+table=4,in_port=1,reg0=0x0 actions=normal
+])
+AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
+sleep 1
+NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 64 10.1.1.2 | FORMAT_PING], [0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+], [], [ovs-appctl dpctl/dump-flows; ovs-ofctl dump-flows br0])
+AT_CHECK_ACTIONS([check_pkt_len(size=200,gt(drop),le(1,3,4,5))])
+

Note that for the le() action I get 1,3,4,5 as the MAC was not yet learned. Maybe in your case the src and dst mac were the same so the FDB decided to drop the packet?

You probably need to figure out why the NORMAL rule decided to insert a DROP. You can use ofproto trace and see.

<SNIP>

On Mon, Mar 28, 2022 at 7:11 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
> This change implements support for the check_pkt_len
> action using the TC police action, which supports MTU
> checking.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

Looks good!

Acked-by: Mike Pattrick <mkp@redhat.com>