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 <[email protected]> > >>>> --- > >>>> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
