| Message ID | 20241023170625.1802298-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | a119828ea608c38611f6ee60e55a7376ca471d6f |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] meta-flow: Fix nw_frag mask while parsing from string. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Ilya Maximets <i.maximets@ovn.org> writes: > 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.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Clearly no one noticed it for a while :) Also, thanks for fixing it in the `mf_set` rather than in `match_set_nw_frag_masked`. The original `cls_rule_set_frag_masked` was a bit messy due to the overload. Your fix continues to look clean to me. Acked-by: Aaron Conole <aconole@redhat.com>
On 23 Oct 2024, at 19:06, Ilya Maximets wrote: > 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.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Thanks for fixing this. The changes look fine to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 10/28/24 12:33, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> 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.") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > Clearly no one noticed it for a while :) ovn-kubernetes has one of these flows and it gets replaced every 10 seconds. :) But yeah, it went a long time unnoticed. > > Also, thanks for fixing it in the `mf_set` rather than in > `match_set_nw_frag_masked`. The original `cls_rule_set_frag_masked` was > a bit messy due to the overload. Your fix continues to look clean to me. > > Acked-by: Aaron Conole <aconole@redhat.com> > Thanks, Aaron and Eelco! Applied and backported down to 2.17. Best regards, Ilya Maximets.
diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 499be04b6..7ebdc1d2c 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -2545,7 +2545,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 e2f4429ae..2363b72aa 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
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.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/meta-flow.c | 3 ++- tests/ovs-ofctl.at | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-)