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