From a119828ea608c38611f6ee60e55a7376ca471d6f Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Wed, 23 Oct 2024 19:06:08 +0200 Subject: [PATCH] meta-flow: Fix nw_frag mask while parsing from string. mf_from_frag_string() sets all the upper bits of the nw_frag to make sure the exact match check will pass. This was not taken into account while splitting nw_frag and IP TOS bits into separate fields and the mask clean up was removed from the cls_rule_set_frag_masked() which is now called match_set_nw_frag_masked(). This leads to the case where the match parsed from the string is not considered equal to the match parsed from the OpenFlow, due to difference in masks. And that is causing ovs-ofctl replace-flows to always replace flows that match on nw_frag, even if they are exactly the same triggering unnecessary flow table updates and revalidation. $ cat frag_flows.txt ip,in_port=1,nw_frag=yes actions=drop $ ovs-ofctl dump-flows --no-stat --no-names br0 ip,in_port=1,nw_frag=yes actions=drop $ ovs-ofctl -vvconn replace-flows br0 frag_flows.txt 2>&1 | grep MOD NXT_FLOW_MOD: DEL_STRICT ip,in_port=1,nw_frag=yes actions=drop NXT_FLOW_MOD: ADD ip,in_port=1,nw_frag=yes actions=drop Clear the extra mask bits while setting match/flow structure from the field to avoid the issue. The issue mainly affects non-exact matches 'non_later' and 'yes', since exact matches are special-handled in many places / considered equal to not having a mask at all. Note: ideally we would not use byte-sized is_all_ones() for exact match checking, but use field-specific checks instead. However, this leads to a much larger change throughout OVS code base and would be much harder to backport. For now, fixing the issue in the way the code was originally implemented. Fixes: 9e44d715638a ("Don't overload IP TOS with the frag matching bits.") Acked-by: Aaron Conole Acked-by: Eelco Chaudron Signed-off-by: Ilya Maximets --- lib/meta-flow.c | 3 ++- tests/ovs-ofctl.at | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/lib/meta-flow.c b/lib/meta-flow.c index c7e1673c06d..2595fd634b4 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -2654,7 +2654,8 @@ mf_set(const struct mf_field *mf, break; case MFF_IP_FRAG: - match_set_nw_frag_masked(match, value->u8, mask->u8); + match_set_nw_frag_masked(match, value->u8, + mask->u8 & FLOW_NW_FRAG_MASK); break; case MFF_ARP_SPA: diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index e2f4429ae55..2363b72aa8c 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -3100,6 +3100,51 @@ AT_CHECK([ovs-ofctl -O OpenFlow14 dump-flows br0 | ofctl_strip | sed '/OFPST_FLO OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ovs-ofctl replace-flows with fragments]) +OVS_VSWITCHD_START + +AT_DATA([frag_flows.txt], [dnl + ip,nw_frag=first actions=drop + ip,nw_frag=later actions=drop + ip,nw_frag=no actions=NORMAL + ip,nw_frag=not_later actions=NORMAL + ip,nw_frag=yes actions=LOCAL +]) +AT_DATA([replace_flows.txt], [dnl + ip,nw_frag=first actions=NORMAL + ip,nw_frag=later actions=LOCAL + ip,nw_frag=no actions=drop + ip,nw_frag=not_later actions=drop + ip,nw_frag=yes actions=drop +]) + +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 frag_flows.txt]) +on_exit 'ovs-ofctl -O OpenFlow13 dump-flows br0' + +dnl Check that flow replacement works. +AT_CHECK([ovs-ofctl -vvconn:console:dbg -O OpenFlow13 \ + replace-flows br0 replace_flows.txt 2>&1 | grep FLOW_MOD \ + | sed 's/.*\(OFPT_FLOW_MOD.*\)/\1/' | strip_xids | sort], [0], [dnl +OFPT_FLOW_MOD (OF1.3): ADD ip,nw_frag=first actions=NORMAL +OFPT_FLOW_MOD (OF1.3): ADD ip,nw_frag=later actions=LOCAL +OFPT_FLOW_MOD (OF1.3): ADD ip,nw_frag=no actions=drop +OFPT_FLOW_MOD (OF1.3): ADD ip,nw_frag=not_later actions=drop +OFPT_FLOW_MOD (OF1.3): ADD ip,nw_frag=yes actions=drop +]) + +dnl Check that replacement to the same set doesn't cause flow modifications. +AT_CHECK([ovs-ofctl -vvconn:console:dbg -O OpenFlow13 \ + replace-flows br0 replace_flows.txt 2>&1 | grep FLOW_MOD \ + | sed 's/.*\(OFPT_FLOW_MOD.*\)/\1/' | strip_xids | sort], [0], []) + +dnl Compare the flow dump against the expected set. +cat replace_flows.txt > expout +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 \ + | ofctl_strip | sed '/OFPST_FLOW/d' | sort], [0], [expout]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ovs-ofctl replace-flows with --bundle]) OVS_VSWITCHD_START