On 10/28/24 12:33, Aaron Conole wrote: > Ilya Maximets <[email protected]> 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 <[email protected]> >> --- > > 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 <[email protected]> > Thanks, Aaron and Eelco! Applied and backported down to 2.17. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
