On Mon, Mar 28, 2022 at 7:11 AM Eelco Chaudron 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
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[] = {
> +
