[ovs-dev] [PATCH] netdev-offload-tc: verify the flower rule installed

Ilya Maximets i.maximets at ovn.org
Thu Jul 8 20:18:50 UTC 2021


On 5/17/21 3:20 PM, Eelco Chaudron wrote:
> When OVs installs the flower rule, it only checks for the OK from the
> kernel. It does not check if the rule requested matches the one
> actually programmed. This change will add this check and warns the
> user if this is not the case.
> 
> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
> ---
>  lib/tc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/lib/tc.c b/lib/tc.c
> index a27cca2cc..e134f6a06 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2979,6 +2979,50 @@ nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *flower)
>      return 0;
>  }
>  
> +static bool
> +cmp_tc_flower_match_action(const struct tc_flower *a,
> +                           const struct tc_flower *b)
> +{
> +    if (memcmp(&a->mask, &b->mask, sizeof a->mask)) {
> +        VLOG_DBG_RL(&error_rl, "tc flower compare failed mask compare");
> +        return false;
> +    }
> +
> +    /* We can not memcmp() the key as some keys might be set while the mask
> +     * is not.*/
> +
> +    for (int i = 0; i < sizeof a->key; i++) {
> +        uint8_t mask = ((uint8_t *)&a->mask)[i];
> +        uint8_t key_a = ((uint8_t *)&a->key)[i] & mask;
> +        uint8_t key_b = ((uint8_t *)&b->key)[i] & mask;
> +
> +        if (key_a != key_b) {
> +            VLOG_DBG_RL(&error_rl, "tc flower compare failed key compare at "
> +                        "%d", i);
> +            return false;
> +        }
> +    }
> +
> +    /* Compare the actions. */
> +    const struct tc_action *action_a = a->actions;
> +    const struct tc_action *action_b = b->actions;
> +
> +    if (a->action_count != b->action_count) {
> +        VLOG_DBG_RL(&error_rl, "tc flower compare failed action length check");
> +        return false;
> +    }
> +
> +    for (int i = 0; i < a->action_count; i++, action_a++, action_b++) {
> +        if (memcmp(action_a, action_b, sizeof *action_a)) {
> +            VLOG_DBG_RL(&error_rl, "tc flower compare failed action compare "
> +                        "for %d", i);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  int
>  tc_replace_flower(struct tcf_id *id, struct tc_flower *flower)
>  {
> @@ -3010,6 +3054,21 @@ tc_replace_flower(struct tcf_id *id, struct tc_flower *flower)
>  
>          id->prio = tc_get_major(tc->tcm_info);
>          id->handle = tc->tcm_handle;
> +
> +        if (id->prio != TC_RESERVED_PRIORITY_POLICE) {
> +            struct tc_flower flower_out;
> +            struct tcf_id id_out;
> +            int ret;
> +
> +            ret = parse_netlink_to_tc_flower(reply, &id_out, &flower_out,
> +                                             false);
> +
> +            if (ret || !cmp_tc_flower_match_action(flower, &flower_out)) {
> +                VLOG_WARN_RL(&error_rl, "Kernel flower acknowledgment does "
> +                             "not match request!\n Set dpif_netlink to dbg to "
> +                             "see which rule caused this error.");

So we're only printing the warning and not reverting the change
and not returning an error, right?  So, OVS will continue to
work with the incorrect rule installed?

I think, we should revert the incorrect change and return the
error, so the flow could be installed to the OVS kernel datapath,
but maybe this is a task for a separate change.

What do you think?

Best regards, Ilya Maximets.


More information about the dev mailing list