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 <echau...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev