| Message ID | 1492133466-38862-1-git-send-email-jarno@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Apr 13, 2017 at 06:31:06PM -0700, Jarno Rajahalme wrote: > Commit 21b2fa617126 ("ofp-parse: Allow match field names in actions > and brackets in matches.") added support for matching a consecutive > set of bits with the [x..y]=z format, but the copying of the parsed > value ('z') to the match was done from a wrong offset, so that the > actual value matched would be incorrect. > > Fix this and add a test case preventing regression in future. > > Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.") > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> Oops, thanks for the fix! Should the test try a multibit match too, for completeness? Acked-by: Ben Pfaff <blp@ovn.org>
> On Apr 14, 2017, at 8:58 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Apr 13, 2017 at 06:31:06PM -0700, Jarno Rajahalme wrote: >> Commit 21b2fa617126 ("ofp-parse: Allow match field names in actions >> and brackets in matches.") added support for matching a consecutive >> set of bits with the [x..y]=z format, but the copying of the parsed >> value ('z') to the match was done from a wrong offset, so that the >> actual value matched would be incorrect. >> >> Fix this and add a test case preventing regression in future. >> >> Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.") >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > > Oops, thanks for the fix! > > Should the test try a multibit match too, for completeness? > Pushed to master and branch-2.7 with an additional multibit match. Thanks for the review! Jarno > Acked-by: Ben Pfaff <blp@ovn.org>
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 7826bc5..c8cac5b 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -290,11 +290,11 @@ parse_subfield(const char *name, const char *str_value, struct match *match, const struct mf_field *field = sf.field; union mf_value value, mask; - unsigned int size = DIV_ROUND_UP(sf.n_bits, 8); + unsigned int size = field->n_bytes; mf_get(field, match, &value, &mask); - bitwise_copy(&val, size, 0, &value, field->n_bytes, sf.ofs, sf.n_bits); - bitwise_one ( &mask, field->n_bytes, sf.ofs, sf.n_bits); + bitwise_copy(&val, size, 0, &value, size, sf.ofs, sf.n_bits); + bitwise_one ( &mask, size, sf.ofs, sf.n_bits); *usable_protocols &= mf_set(field, &value, &mask, match, &error); } return error; diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index 737f609..18ab788 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -285,6 +285,7 @@ AT_CLEANUP AT_SETUP([ovs-ofctl parse-flows (OpenFlow 1.2)]) AT_DATA([flows.txt], [[ # comment +tcp,tp_src[5]=1,actions=flood tcp,tp_src=123,actions=flood in_port=LOCAL dl_vlan=9 dl_src=00:0A:E4:25:6B:B0 actions=mod_vlan_vid:7,mod_vlan_pcp:2 udp dl_vlan_pcp=7 idle_timeout=5 actions=strip_vlan output:0 @@ -309,6 +310,7 @@ AT_CHECK([ovs-ofctl --protocols OpenFlow12 parse-flows flows.txt AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout]], [0], [[usable protocols: NXM,OXM chosen protocol: OXM-OpenFlow12 +OFPT_FLOW_MOD (OF1.2): ADD tcp,tp_src=0x20/0x20 actions=FLOOD OFPT_FLOW_MOD (OF1.2): ADD tcp,tp_src=123 actions=FLOOD OFPT_FLOW_MOD (OF1.2): ADD in_port=LOCAL,dl_vlan=9,dl_src=00:0a:e4:25:6b:b0 actions=set_field:4103->vlan_vid,set_field:2->vlan_pcp OFPT_FLOW_MOD (OF1.2): ADD udp,dl_vlan_pcp=7 idle:5 actions=pop_vlan,output:0
Commit 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.") added support for matching a consecutive set of bits with the [x..y]=z format, but the copying of the parsed value ('z') to the match was done from a wrong offset, so that the actual value matched would be incorrect. Fix this and add a test case preventing regression in future. Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.") Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- lib/ofp-parse.c | 6 +++--- tests/ovs-ofctl.at | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-)