[ovs-dev] [PATCH] netdev-offload-tc: verify the flower rule installed
Ilya Maximets
i.maximets at ovn.org
Thu Jul 8 20:18:50 UTC 2021
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?
Best regards, Ilya Maximets.
More information about the dev
mailing list