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: 9e44d71 ("Don't overload IP TOS with the frag matching bits.")
Acked-by: Aaron Conole <[email protected]>
Acked-by: Eelco Chaudron <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>

lib/meta-flow.c

@@ -2652,7 +2652,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:

tests/ovs-ofctl.at

@@ -3086,6 +3086,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