| Message ID | 1443559234-7330-3-git-send-email-joestringer@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Sep 29, 2015 at 01:40:25PM -0700, Joe Stringer wrote: > Commit 7eb4b1f1d "ofp-actions: Support OF1.5 (draft) masked Set-Field, > merge with reg_load." purportedly merged the set_field and reg_load > fields, including the commandline parsing to use the metafield syntax > like "load:192.168.0.1->OXM_OF_IPV4_SRC". However, the actual reg_load > parsing code was not updated. > > This patch takes advantage of the newly refactored parse_set_field() > helpers to make parse_reg_load() conform to the documentation, which > allows more thorough validation of inputs, and allows reg_load to parse > fields that are wider than 64 bits, as well as masked fields. > > Note that this disallows syntax that was previously allowed, such as: > load:0x0a000001->OXM_OF_IPV4_SRC > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > > Note that v2.4.0 includes the aforementioned commit; we should consider > either backporting this fix to branch-2.4, or reverting the > documentation modification on branch-2.4 to be consistent with the > previous syntax. I'm having a little trouble following here. Can you give an example of a "load" or a "set_field" for which (before this commit) the documentation and the implementation disagree? Also I think that this commit leads to an inconsistency between parsing and formatting. For example, take a look at ofproto-dpif.at line 1537, which is part of the output of "ovs-ofctl dump-flows": cookie=0xd, n_packets=3, n_bytes=180, arp,dl_src=80:88:88:88:88:88 actions=load:0x2->NXM_OF_ARP_OP[[]],CONTROLLER:65535,load:0xc0a88001->NXM_OF_ARP_SPA[[]],CONTROLLER:65535,load:0x404444444441->NXM_NX_ARP_THA[[]],load:0x1010101->NXM_OF_ARP_SPA[[]],load:0x2020202->NXM_OF_ARP_TPA[[]],CONTROLLER:65535 It contains (after m4 processing) "load:0xc0a88001->NXM_OF_ARP_SPA[]", which "ovs-ofctl add-flow" now rejects: $ ovs-ofctl add-flow br0 "actions=load:0xc0a88001->NXM_OF_ARP_SPA[]" ovs-ofctl: 0xc0a88001: invalid IP address I'm pretty nervous about rejecting syntax that's been valid for a long time.
On 29 September 2015 at 23:35, Ben Pfaff <blp@nicira.com> wrote: > On Tue, Sep 29, 2015 at 01:40:25PM -0700, Joe Stringer wrote: >> Commit 7eb4b1f1d "ofp-actions: Support OF1.5 (draft) masked Set-Field, >> merge with reg_load." purportedly merged the set_field and reg_load >> fields, including the commandline parsing to use the metafield syntax >> like "load:192.168.0.1->OXM_OF_IPV4_SRC". However, the actual reg_load >> parsing code was not updated. >> >> This patch takes advantage of the newly refactored parse_set_field() >> helpers to make parse_reg_load() conform to the documentation, which >> allows more thorough validation of inputs, and allows reg_load to parse >> fields that are wider than 64 bits, as well as masked fields. >> >> Note that this disallows syntax that was previously allowed, such as: >> load:0x0a000001->OXM_OF_IPV4_SRC >> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> --- >> >> Note that v2.4.0 includes the aforementioned commit; we should consider >> either backporting this fix to branch-2.4, or reverting the >> documentation modification on branch-2.4 to be consistent with the >> previous syntax. > > I'm having a little trouble following here. Can you give an example of > a "load" or a "set_field" for which (before this commit) the > documentation and the implementation disagree? Hmm, reviewing it looks like I'm mistaken. I saw the original description removed in the patch but didn't notice that the new text still described the original syntax. > Also I think that this commit leads to an inconsistency between parsing > and formatting. For example, take a look at ofproto-dpif.at line 1537, > which is part of the output of "ovs-ofctl dump-flows": > > cookie=0xd, n_packets=3, n_bytes=180, arp,dl_src=80:88:88:88:88:88 actions=load:0x2->NXM_OF_ARP_OP[[]],CONTROLLER:65535,load:0xc0a88001->NXM_OF_ARP_SPA[[]],CONTROLLER:65535,load:0x404444444441->NXM_NX_ARP_THA[[]],load:0x1010101->NXM_OF_ARP_SPA[[]],load:0x2020202->NXM_OF_ARP_TPA[[]],CONTROLLER:65535 > > It contains (after m4 processing) "load:0xc0a88001->NXM_OF_ARP_SPA[]", > which "ovs-ofctl add-flow" now rejects: > > $ ovs-ofctl add-flow br0 "actions=load:0xc0a88001->NXM_OF_ARP_SPA[]" > ovs-ofctl: 0xc0a88001: invalid IP address > > I'm pretty nervous about rejecting syntax that's been valid for a long > time. Fair enough, I can drop this approach, so these issues should not be problematic. I think the main change I was looking for in this series is to support >64bit values by switching the value parsing to use parse_int_string() instead of strtoull() directly, using the mf field's expected length to figure out how long the string is allowed to be. The other matter was that there seems to be a discrepancy between the error checking code in set_field vs reg_load, for instance checking whether the field is read-only, or passing it through mf_is_value_valid(). Again, that doesn't require the current approach to address.
On Wed, Sep 30, 2015 at 09:47:16AM -0700, Joe Stringer wrote: > I think the main change I was looking for in this series is to support > >64bit values by switching the value parsing to use parse_int_string() > instead of strtoull() directly, using the mf field's expected length > to figure out how long the string is allowed to be. OK, doing that makes sense. > The other matter was that there seems to be a discrepancy between the > error checking code in set_field vs reg_load, for instance checking > whether the field is read-only, or passing it through > mf_is_value_valid(). Again, that doesn't require the current approach > to address. Some of that comes down to backward compatibility, since the requirements imposed by OpenFlow on set_field have subtle differences from those imposed by OVS on reg_load. That doesn't have to be imposed on the basis of how the action is written in text form, I guess, but any given load or set_field has to satisfy either one or the other set of requirements.
diff --git a/NEWS b/NEWS index ca22c8e..ef0348b 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,9 @@ Post-v2.4.0 targets to run a new system testsuite. These tests can be run inside a Vagrant box. See INSTALL.md for details - Dropped support for GRE64 tunnel. + - ovs-ofctl syntax for the "reg_load" action now validates inputs to + ensure they are of the format that the field accepts. Simply loading + hexadecimal values into any field will no longer work. v2.4.0 - 20 Aug 2015 diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index d33e429..153c712 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -2571,36 +2571,63 @@ error: } static char * OVS_WARN_UNUSED_RESULT -parse_reg_load(char *arg, struct ofpbuf *ofpacts) +parse_reg_load__(char *arg, struct ofpbuf *ofpacts) { struct ofpact_set_field *sf = ofpact_put_reg_load(ofpacts); - const char *full_arg = arg; - uint64_t value = strtoull(arg, (char **) &arg, 0); + union mf_value immediate, mask; + const struct mf_field *mf; + char *key, *value, *delim; struct mf_subfield dst; - char *error; + char *error = NULL; - if (strncmp(arg, "->", 2)) { - return xasprintf("%s: missing `->' following value", full_arg); + error = set_field_split_str(arg, &key, &value, &delim); + if (error) { + return error; } - arg += 2; - error = mf_parse_subfield(&dst, arg); + + error = mf_parse_subfield(&dst, key); if (error) { return error; } + delim[0] = '\0'; + mf = dst.field; - if (dst.n_bits < 64 && (value >> dst.n_bits) != 0) { - return xasprintf("%s: value %"PRIu64" does not fit into %d bits", - full_arg, value, dst.n_bits); + error = set_field_parse__(mf, key, value, &immediate, &mask); + if (error) { + return error; } - sf->field = dst.field; + if (!bitwise_is_all_zeros(&immediate, mf->n_bytes, dst.n_bits, + mf->n_bytes * 8 - dst.n_bits)) { + struct ds ds; + + ds_init(&ds); + mf_format(mf, &immediate, &mask, &ds); + error = xasprintf("%s: value %s does not fit into %d bits", + arg, ds_cstr(&ds), dst.n_bits); + ds_destroy(&ds); + return error; + } + + sf->field = mf; memset(&sf->value, 0, sizeof sf->value); - bitwise_put(value, &sf->value, dst.field->n_bytes, dst.ofs, dst.n_bits); - bitwise_put(UINT64_MAX, &sf->mask, - dst.field->n_bytes, dst.ofs, dst.n_bits); + bitwise_copy(&immediate, mf->n_bytes, 0, &sf->value, mf->n_bytes, + dst.ofs, dst.n_bits); + bitwise_copy(&mask, mf->n_bytes, 0, &sf->mask, mf->n_bytes, + dst.ofs, dst.n_bits); + return NULL; } +static char * OVS_WARN_UNUSED_RESULT +parse_reg_load(char *arg, struct ofpbuf *ofpacts) +{ + char *copy = xstrdup(arg); + char *error = parse_reg_load__(copy, ofpacts); + free(copy); + return error; +} + static void format_SET_FIELD(const struct ofpact_set_field *a, struct ds *s) { diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 9609d2d..c211a50 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -1292,7 +1292,7 @@ cookie=0x7 table=5 in_port=84 actions=load:5->NXM_NX_REG4[[]],load:6->NXM_NX_TUN cookie=0x8 table=6 in_port=85 actions=mod_tp_src:85,controller,resubmit(86,7) cookie=0x9 table=7 in_port=86 actions=mod_tp_dst:86,controller,controller cookie=0xa dl_src=40:44:44:44:44:41 actions=mod_vlan_vid:99,mod_vlan_pcp:1,controller -cookie=0xd dl_src=80:88:88:88:88:88 arp actions=load:2->OXM_OF_ARP_OP[[]],controller,load:0xc0a88001->OXM_OF_ARP_SPA[[]],controller,load:0x404444444441->OXM_OF_ARP_THA[[]],load:0x01010101->OXM_OF_ARP_SPA[[]],load:0x02020202->OXM_OF_ARP_TPA[[]],controller +cookie=0xd dl_src=80:88:88:88:88:88 arp actions=load:2->OXM_OF_ARP_OP[[]],controller,load:192.168.128.1->OXM_OF_ARP_SPA[[]],controller,load:40:44:44:44:44:41->OXM_OF_ARP_THA[[]],load:1.1.1.1->OXM_OF_ARP_SPA[[]],load:2.2.2.2->OXM_OF_ARP_TPA[[]],controller ]) AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) @@ -1564,7 +1564,7 @@ cookie=0xd dl_src=60:66:66:66:66:66 actions=pop_mpls:0x0800,controller cookie=0xc dl_src=70:77:77:77:77:77 actions=push_mpls:0x8848,load:1000->OXM_OF_MPLS_LABEL[[]],load:7->OXM_OF_MPLS_TC[[]],controller cookie=0xd dl_src=60:66:66:66:00:01 actions=pop_mpls:0x0800,dec_ttl,controller -cookie=0xd dl_src=60:66:66:66:00:02 actions=pop_mpls:0x0800,load:0xa000001->OXM_OF_IPV4_DST[[]],controller +cookie=0xd dl_src=60:66:66:66:00:02 actions=pop_mpls:0x0800,load:10.0.0.1->OXM_OF_IPV4_DST[[]],controller cookie=0xd dl_src=60:66:66:66:00:03 actions=pop_mpls:0x0800,move:OXM_OF_IPV4_DST[[]]->OXM_OF_IPV4_SRC[[]],controller cookie=0xd dl_src=60:66:66:66:00:04 actions=pop_mpls:0x0800,push:OXM_OF_IPV4_DST[[]],pop:OXM_OF_IPV4_SRC[[]],controller cookie=0xd dl_src=60:66:66:66:00:05 actions=pop_mpls:0x0800,multipath(eth_src,50,modulo_n,256,0,OXM_OF_IPV4_SRC[[0..7]]),controller @@ -3020,7 +3020,7 @@ OVS_VSWITCHD_START ADD_OF_PORTS([br0], [1], [2]) ovs-vsctl -- set Interface p2 type=dummy options:pcap=p2.pcap -ovs-ofctl add-flow br0 'in_port=1,arp actions=load:2->OXM_OF_ARP_OP[[]],2,load:0xc0a88001->OXM_OF_ARP_SPA[[]],2,load:0x404444444441->OXM_OF_ARP_THA[[]],2' +ovs-ofctl add-flow br0 'in_port=1,arp actions=load:2->OXM_OF_ARP_OP[[]],2,load:192.168.128.1->OXM_OF_ARP_SPA[[]],2,load:40:44:44:44:44:41->OXM_OF_ARP_THA[[]],2' # Input some packets that should follow the arp modification slow-path. for i in 1 2 3; do
Commit 7eb4b1f1d "ofp-actions: Support OF1.5 (draft) masked Set-Field, merge with reg_load." purportedly merged the set_field and reg_load fields, including the commandline parsing to use the metafield syntax like "load:192.168.0.1->OXM_OF_IPV4_SRC". However, the actual reg_load parsing code was not updated. This patch takes advantage of the newly refactored parse_set_field() helpers to make parse_reg_load() conform to the documentation, which allows more thorough validation of inputs, and allows reg_load to parse fields that are wider than 64 bits, as well as masked fields. Note that this disallows syntax that was previously allowed, such as: load:0x0a000001->OXM_OF_IPV4_SRC Signed-off-by: Joe Stringer <joestringer@nicira.com> --- Note that v2.4.0 includes the aforementioned commit; we should consider either backporting this fix to branch-2.4, or reverting the documentation modification on branch-2.4 to be consistent with the previous syntax. --- NEWS | 3 +++ lib/ofp-actions.c | 57 +++++++++++++++++++++++++++++++++++++-------------- tests/ofproto-dpif.at | 6 +++--- 3 files changed, 48 insertions(+), 18 deletions(-)