[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