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

[Marcelo Ricardo Leitner]

On Mon, Jul 12, 2021 at 10:28:15AM +0200, Eelco Chaudron wrote:
>
>
> On 9 Jul 2021, at 20:23, Ilya Maximets wrote:
>
> > On 7/9/21 10:35 AM, Eelco Chaudron wrote:
> >>
> >>
> >> On 8 Jul 2021, at 22:18, Ilya Maximets wrote:
> >>
> >>> 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?
> >>
> >> The goal was to make sure we do not break anything, in case there is an existing kernel bug. As unfortunately, we are missing a good set of TC unit tests.
> >>
> >> With the "warning only" option, we can backport this. And if in the field we do not see any (false) reports, a follow-up patch can do as you suggested.
> >
> > Make sense.  I removed '\n' from a warning (these doesn't look good in the log)
> > and applied to master.
>
> Thanks!
>
> > You and Marcelo are talking about backporting, do you think it make sense to
> > backport to stable branches?
>
> If it applies cleanly, I would suggest backporting it all the way to 2.13. Marcelo?

I don't know how different is the support for 2.13 and 2.15. I mean,
if 2.13 is only for critical patches or so. Anyhow, I'd say 2.15 yes
and 2.13 on best effort. :)

>
> //Eelco
>