ofproto-dpif-xlate: Always mask ip proto field.

The ofproto layer currently treats nw_proto field as overloaded to mean
both that a proper nw layer exists, as well as the value contained in
the header for the nw proto.  However, this is incorrect behavior as
relevant standards permit that any value, including '0' should be treated
as a valid value.

Because of this overload, when the ofproto layer builds action list for
a packet with nw_proto of 0, it won't build the complete action list that
we expect to be built for the packet.  That will cause a bad behavior
where all packets passing the datapath will fall into an incomplete
action set.

The fix here is to unwildcard nw_proto, allowing us to preserve setting
actions for protocols which we know have support for the actions we
program.  This means that a traffic which contains nw_proto == 0 cannot
cause connectivity breakage with other traffic on the link.

Reported-by: David Marchand <dmarchand@redhat.com>
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134873
Acked-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

include/openvswitch/meta-flow.h

@@ -2366,6 +2366,10 @@ void mf_format_subvalue(const union mf_subvalue *subvalue, struct ds *s);
 void field_array_set(enum mf_field_id id, const union mf_value *,
                      struct field_array *);
 
+/* Mask the required l3 prerequisites if a 'set' action occurs. */
+void mf_set_mask_l3_prereqs(const struct mf_field *, const struct flow *,
+                            struct flow_wildcards *);
+
 #ifdef __cplusplus
 }
 #endif

lib/meta-flow.c

@@ -3676,3 +3676,28 @@ mf_bitmap_not(struct mf_bitmap x)
     bitmap_not(x.bm, MFF_N_IDS);
     return x;
 }
+
+void
+mf_set_mask_l3_prereqs(const struct mf_field *mf, const struct flow *fl,
+                       struct flow_wildcards *wc)
+{
+    if (is_ip_any(fl) &&
+        ((mf->id == MFF_IPV4_SRC) ||
+         (mf->id == MFF_IPV4_DST) ||
+         (mf->id == MFF_IPV6_SRC) ||
+         (mf->id == MFF_IPV6_DST) ||
+         (mf->id == MFF_IPV6_LABEL) ||
+         (mf->id == MFF_IP_DSCP) ||
+         (mf->id == MFF_IP_ECN) ||
+         (mf->id == MFF_IP_TTL))) {
+        WC_MASK_FIELD(wc, nw_proto);
+    } else if ((fl->dl_type == htons(ETH_TYPE_ARP)) &&
+               ((mf->id == MFF_ARP_OP) ||
+                (mf->id == MFF_ARP_SHA) ||
+                (mf->id == MFF_ARP_THA) ||
+                (mf->id == MFF_ARP_SPA) ||
+                (mf->id == MFF_ARP_TPA))) {
+        /* mask only the lower 8 bits. */
+        wc->masks.nw_proto = 0xff;
+    }
+}

ofproto/ofproto-dpif-xlate.c

@@ -5186,6 +5186,7 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
     }
 
     ctx->wc->masks.nw_ttl = 0xff;
+    WC_MASK_FIELD(ctx->wc, nw_proto);
     if (flow->nw_ttl > 1) {
         flow->nw_ttl--;
         return false;
@@ -7094,19 +7095,22 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_SET_IPV4_SRC:
             if (flow->dl_type == htons(ETH_TYPE_IP)) {
                 memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
+                WC_MASK_FIELD(wc, nw_proto);
                 flow->nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4;
             }
             break;
 
         case OFPACT_SET_IPV4_DST:
             if (flow->dl_type == htons(ETH_TYPE_IP)) {
                 memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
+                WC_MASK_FIELD(wc, nw_proto);
                 flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
             }
             break;
 
         case OFPACT_SET_IP_DSCP:
             if (is_ip_any(flow)) {
+                WC_MASK_FIELD(wc, nw_proto);
                 wc->masks.nw_tos |= IP_DSCP_MASK;
                 flow->nw_tos &= ~IP_DSCP_MASK;
                 flow->nw_tos |= ofpact_get_SET_IP_DSCP(a)->dscp;
@@ -7115,6 +7119,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_IP_ECN:
             if (is_ip_any(flow)) {
+                WC_MASK_FIELD(wc, nw_proto);
                 wc->masks.nw_tos |= IP_ECN_MASK;
                 flow->nw_tos &= ~IP_ECN_MASK;
                 flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
@@ -7123,6 +7128,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_IP_TTL:
             if (is_ip_any(flow)) {
+                WC_MASK_FIELD(wc, nw_proto);
                 wc->masks.nw_ttl = 0xff;
                 flow->nw_ttl = ofpact_get_SET_IP_TTL(a)->ttl;
             }
@@ -7190,6 +7196,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
             /* Set the field only if the packet actually has it. */
             if (mf_are_prereqs_ok(mf, flow, wc)) {
+                mf_set_mask_l3_prereqs(mf, flow, wc);
                 mf_mask_field_masked(mf, ofpact_set_field_mask(set_field), wc);
                 mf_set_flow_value_masked(mf, set_field->value,
                                          ofpact_set_field_mask(set_field),
@@ -7246,6 +7253,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_DEC_TTL:
             wc->masks.nw_ttl = 0xff;
+            WC_MASK_FIELD(wc, nw_proto);
             if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
                 return;
             }

tests/ofproto-dpif.at

@@ -720,7 +720,7 @@ table=2 ip actions=set_field:192.168.3.91->ip_src,output(11)
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
 AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,nw_frag=no,icmp_type=8,icmp_code=0'], [0], [stdout])
 AT_CHECK([tail -2 stdout], [0],
-  [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_src=192.168.0.1,nw_frag=no
+  [Megaflow: recirc_id=0,eth,icmp,in_port=1,nw_src=192.168.0.1,nw_frag=no
 Datapath actions: 10,set(ipv4(src=192.168.3.91)),11,set(ipv4(src=192.168.3.90)),13
 ])
 OVS_VSWITCHD_STOP
@@ -783,7 +783,7 @@ AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_ds
 # Must match on the source address to be able to restore it's value for
 # the second bucket
 AT_CHECK([tail -2 stdout], [0],
-  [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_src=192.168.0.1,nw_frag=no
+  [Megaflow: recirc_id=0,eth,icmp,in_port=1,nw_src=192.168.0.1,nw_frag=no
 Datapath actions: set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),11
 ])
 OVS_VSWITCHD_STOP
@@ -815,7 +815,7 @@ done
 AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/dp_hash(.*\/0xf)/dp_hash(0xXXXX\/0xf)/' |  sed 's/packets.*actions:/actions:/' | strip_ufid | strip_used | sort], [0], [dnl
 flow-dump from the main thread:
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), actions:hash(sym_l4(0)),recirc(0x1)
-recirc_id(0x1),dp_hash(0xXXXX/0xf),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(src=192.168.0.1,frag=no), actions:set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),10
+recirc_id(0x1),dp_hash(0xXXXX/0xf),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(src=192.168.0.1,proto=1,frag=no), actions:set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),10
 ])
 
 OVS_VSWITCHD_STOP
@@ -830,7 +830,7 @@ AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_ds
 # Must match on the source address to be able to restore it's value for
 # the third bucket
 AT_CHECK([tail -2 stdout], [0],
-  [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_src=192.168.0.1,nw_frag=no
+  [Megaflow: recirc_id=0,eth,icmp,in_port=1,nw_src=192.168.0.1,nw_frag=no
 Datapath actions: set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),11
 ])
 OVS_VSWITCHD_STOP
@@ -1407,17 +1407,17 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=111,tos=0,ttl=2,frag=no)' -generate], [0], [stdout])
 AT_CHECK([tail -4 stdout], [0], [
 Final flow: ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=111,nw_tos=0,nw_ecn=0,nw_ttl=1,nw_frag=no
-Megaflow: recirc_id=0,eth,ip,in_port=1,nw_ttl=2,nw_frag=no
+Megaflow: recirc_id=0,eth,ip,in_port=1,nw_proto=111,nw_ttl=2,nw_frag=no
 Datapath actions: set(ipv4(ttl=1)),2,userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)),4
 ])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=111,tos=0,ttl=3,frag=no)'], [0], [stdout])
 AT_CHECK([tail -2 stdout], [0],
-  [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_ttl=3,nw_frag=no
+  [Megaflow: recirc_id=0,eth,ip,in_port=1,nw_proto=111,nw_ttl=3,nw_frag=no
 Datapath actions: set(ipv4(ttl=2)),2,set(ipv4(ttl=1)),3,4
 ])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=10,tclass=0x70,hlimit=128,frag=no)'], [0], [stdout])
 AT_CHECK([tail -2 stdout], [0],
-  [Megaflow: recirc_id=0,eth,ipv6,in_port=1,nw_ttl=128,nw_frag=no
+  [Megaflow: recirc_id=0,eth,ipv6,in_port=1,nw_proto=10,nw_ttl=128,nw_frag=no
 Datapath actions: set(ipv6(hlimit=127)),2,set(ipv6(hlimit=126)),3,4
 ])
 
@@ -1527,7 +1527,7 @@ AT_CHECK([ovs-vsctl -- \
         --id=@q2 create Queue dscp=2], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(9),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=1.1.1.1,dst=2.2.2.2,proto=1,tos=0xff,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 AT_CHECK([tail -2 stdout], [0],
-  [Megaflow: recirc_id=0,skb_priority=0,eth,ip,in_port=9,nw_tos=252,nw_frag=no
+  [Megaflow: recirc_id=0,skb_priority=0,eth,icmp,in_port=9,nw_tos=252,nw_frag=no
 Datapath actions: dnl
 100,dnl
 set(ipv4(tos=0x4/0xfc)),set(skb_priority(0x1)),1,dnl
@@ -11703,7 +11703,7 @@ ovs-ofctl dump-flows br0
 
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 AT_CHECK([tail -3 stdout], [0], [dnl
-Megaflow: recirc_id=0,eth,ip,reg0=0/0x1,in_port=1,nw_src=10.10.10.2,nw_frag=no
+Megaflow: recirc_id=0,eth,icmp,reg0=0/0x1,in_port=1,nw_src=10.10.10.2,nw_frag=no
 Datapath actions: drop
 Translation failed (Recursion too deep), packet is dropped.
 ])