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

Eelco Chaudron echaudro at redhat.com
Mon Jul 12 08:28:15 UTC 2021



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?

//Eelco



More information about the dev mailing list