From: Ben Pfaff Date: Thu, 12 Apr 2012 00:08:13 +0000 (-0700) Subject: learn: Fix bugs when learn actions use subfields wider than 64 bits. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=337b9cec45b813ef467b39a8d55741030f7404ff;p=openvswitch learn: Fix bugs when learn actions use subfields wider than 64 bits. Bug #10576. Reported-by: James Schmidt Signed-off-by: Ben Pfaff --- diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index 7586a520..f8b863c9 100644 --- a/include/openflow/nicira-ext.h +++ b/include/openflow/nicira-ext.h @@ -787,6 +787,9 @@ enum nx_mp_algorithm { * field[ofs:ofs+n_bits-1]. Actions are executed in the same order as the * flow_mod_specs. * + * A single NXAST_REG_LOAD action writes no more than 64 bits, so n_bits + * greater than 64 yields multiple NXAST_REG_LOAD actions. + * * The flow_mod_spec destination spec for 'dst' of 2 (when 'src' is 0) is * empty. It has the following meaning: * diff --git a/lib/learn.c b/lib/learn.c index 94fc1b02..d2db792e 100644 --- a/lib/learn.c +++ b/lib/learn.c @@ -177,7 +177,14 @@ learn_check(const struct nx_action_learn *learn, const struct flow *flow) if (dst_type == NX_LEARN_DST_MATCH && src_type == NX_LEARN_SRC_IMMEDIATE) { - mf_set_subfield(&dst, value, &rule); + if (n_bits <= 64) { + mf_set_subfield(&dst, value, &rule); + } else { + /* We're only setting subfields to allow us to check + * prerequisites. No prerequisite depends on the value of + * a field that is wider than 64 bits. So just skip + * setting it entirely. */ + } } } } @@ -223,10 +230,10 @@ learn_execute(const struct nx_action_learn *learn, const struct flow *flow, int n_bits = header & NX_LEARN_N_BITS_MASK; int src_type = header & NX_LEARN_SRC_MASK; int dst_type = header & NX_LEARN_DST_MASK; - uint64_t value; + union mf_subvalue value; - struct nx_action_reg_load *load; struct mf_subfield dst; + int chunk, ofs; if (!header) { break; @@ -236,27 +243,43 @@ learn_execute(const struct nx_action_learn *learn, const struct flow *flow, struct mf_subfield src; get_subfield(n_bits, &p, &src); - value = mf_get_subfield(&src, flow); + mf_read_subfield(&src, flow, &value); } else { - value = get_bits(n_bits, &p); + int p_bytes = 2 * DIV_ROUND_UP(n_bits, 16); + + memset(&value, 0, sizeof value); + bitwise_copy(p, p_bytes, 0, + &value, sizeof value, 0, + n_bits); + p = (const uint8_t *) p + p_bytes; } switch (dst_type) { case NX_LEARN_DST_MATCH: get_subfield(n_bits, &p, &dst); - mf_set_subfield(&dst, value, &fm->cr); + mf_write_subfield(&dst, &value, &fm->cr); break; case NX_LEARN_DST_LOAD: get_subfield(n_bits, &p, &dst); - load = ofputil_put_NXAST_REG_LOAD(&actions); - load->ofs_nbits = nxm_encode_ofs_nbits(dst.ofs, dst.n_bits); - load->dst = htonl(dst.field->nxm_header); - load->value = htonll(value); + for (ofs = 0; ofs < n_bits; ofs += chunk) { + struct nx_action_reg_load *load; + + chunk = MIN(n_bits - ofs, 64); + + load = ofputil_put_NXAST_REG_LOAD(&actions); + load->ofs_nbits = nxm_encode_ofs_nbits(dst.ofs + ofs, chunk); + load->dst = htonl(dst.field->nxm_header); + bitwise_copy(&value, sizeof value, ofs, + &load->value, sizeof load->value, 0, + chunk); + } break; case NX_LEARN_DST_OUTPUT: - ofputil_put_OFPAT10_OUTPUT(&actions)->port = htons(value); + if (n_bits <= 16 || is_all_zeros(value.u8, sizeof value - 2)) { + ofputil_put_OFPAT10_OUTPUT(&actions)->port = value.be16[7]; + } break; } } diff --git a/tests/learn.at b/tests/learn.at index 861c2f9a..84319377 100644 --- a/tests/learn.at +++ b/tests/learn.at @@ -112,6 +112,48 @@ NXST_FLOW reply: OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([learning action - TCPv4 port learning]) +OVS_VSWITCHD_START( + [add-port br0 eth0 -- set Interface eth0 type=dummy -- \ + add-port br0 eth1 -- set Interface eth1 type=dummy -- \ + add-port br0 eth2 -- set Interface eth2 type=dummy]) +# Set up flow table for TCPv4 port learning. +AT_CHECK([[ovs-ofctl add-flow br0 'table=0 tcp actions=learn(table=1, hard_timeout=60, eth_type=0x800, nw_proto=6, NXM_OF_IP_SRC[]=NXM_OF_IP_DST[], NXM_OF_IP_DST[]=NXM_OF_IP_SRC[], NXM_OF_TCP_SRC[]=NXM_OF_TCP_DST[], NXM_OF_TCP_DST[]=NXM_OF_TCP_SRC[]), flood']]) + +# Trace a TCPv4 packet arriving on port 3. +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(3),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:06),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no),tcp(src=40000,dst=80)' -generate], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], [Datapath actions: 2,0,1 +]) + +# Check for the learning entry. +AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip | sort], [0], [dnl + table=1, hard_timeout=60,tcp,nw_src=192.168.0.1,nw_dst=192.168.0.2,tp_src=80,tp_dst=40000 actions=drop +NXST_FLOW reply: +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([learning action - TCPv6 port learning]) +OVS_VSWITCHD_START( + [add-port br0 eth0 -- set Interface eth0 type=dummy -- \ + add-port br0 eth1 -- set Interface eth1 type=dummy -- \ + add-port br0 eth2 -- set Interface eth2 type=dummy]) +# Set up flow table for TCPv6 port learning. +AT_CHECK([[ovs-ofctl add-flow br0 'table=0 tcp6 actions=learn(table=1, hard_timeout=60, eth_type=0x86dd, nw_proto=6, NXM_NX_IPV6_SRC[]=NXM_NX_IPV6_DST[], NXM_NX_IPV6_DST[]=NXM_NX_IPV6_SRC[], NXM_OF_TCP_SRC[]=NXM_OF_TCP_DST[], NXM_OF_TCP_DST[]=NXM_OF_TCP_SRC[]), flood']]) + +# Trace a TCPv6 packet arriving on port 3. +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(3),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:06),eth_type(0x86dd),ipv6(src=fec0::2,dst=fec0::1,label=0,proto=6,tclass=0,hlimit=255,frag=no),tcp(src=40000,dst=80)' -generate], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], [Datapath actions: 2,0,1 +]) + +# Check for the learning entry. +AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip | sort], [0], [dnl + table=1, hard_timeout=60,tcp6,ipv6_src=fec0::1,ipv6_dst=fec0::2,tp_src=80,tp_dst=40000 actions=drop +NXST_FLOW reply: +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([learning action - fin_timeout feature]) # This is a totally artificial use of the "learn" action. The only purpose # is to check that specifying fin_idle_timeout or fin_hard_timeout causes