[ovs-dev,PATCHv3,02/11] ofp-actions: Make parse_reg_load() accept set_field syntax.
diff mbox

Message ID 1443559234-7330-3-git-send-email-joestringer@nicira.com
State Changes Requested
Headers show

Commit Message

Joe Stringer Sept. 29, 2015, 8:40 p.m. UTC
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(-)

Comments

Ben Pfaff Sept. 30, 2015, 6:35 a.m. UTC | #1
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.
Joe Stringer Sept. 30, 2015, 4:47 p.m. UTC | #2
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.
Ben Pfaff Sept. 30, 2015, 5:34 p.m. UTC | #3
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.

Patch
diff mbox

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