On Thu, Dec 4, 2014 at 2:33 PM, Ben Pfaff <b...@nicira.com> wrote:
> Commit 7eb4b1f1d70345f ("ofp-actions: Support OF1.5 (draft) masked
> Set-Field, merge with reg_load.") introduced a bug in that a set_field
> action that set an entire field would be translated incorrectly to
> reg_load, if the field being set only occupied a portion of the bytes that
> it contains. For example, an MPLS label is 20 bits but has a 4-byte field,
> which meant that a set_field would get translated into a reg_load that
> wrote all 32 bits; in turn, the receiver of that reg_load would reject it
> because it was attempting to set invalid bits (the top 12 bits).
>
> This commit fixes the problem by omitting invalid bits when encoding a
> reg_load action.
>
> Reported-by: Pravin Shelar <pshe...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
This patch fixed the bug.
Thanks,
Pravin.
> ---
> lib/ofp-actions.c | 15 ++++++++-------
> tests/ofp-actions.at | 15 +++++++++++++++
> 2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 33b419d..23ca55b 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -2183,16 +2183,17 @@ static bool
> next_load_segment(const struct ofpact_set_field *sf,
> struct mf_subfield *dst, uint64_t *value)
> {
> - int w = sf->field->n_bytes;
> + int n_bits = sf->field->n_bits;
> + int n_bytes = sf->field->n_bytes;
> int start = dst->ofs + dst->n_bits;
>
> - if (start < 8 * w) {
> + if (start < n_bits) {
> dst->field = sf->field;
> - dst->ofs = bitwise_scan(&sf->mask, w, 1, start, 8 * w);
> - if (dst->ofs < 8 * w) {
> - dst->n_bits = bitwise_scan(&sf->mask, w, 0, dst->ofs + 1,
> - MIN(dst->ofs + 64, 8 * w)) - dst->ofs;
> - *value = bitwise_get(&sf->value, w, dst->ofs, dst->n_bits);
> + dst->ofs = bitwise_scan(&sf->mask, n_bytes, 1, start, n_bits);
> + if (dst->ofs < n_bits) {
> + dst->n_bits = bitwise_scan(&sf->mask, n_bytes, 0, dst->ofs + 1,
> + MIN(dst->ofs + 64, n_bits)) -
> dst->ofs;
> + *value = bitwise_get(&sf->value, n_bytes, dst->ofs, dst->n_bits);
> return true;
> }
> }
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index af5dd19..876be67 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -581,3 +581,18 @@ ovs-ofctl: none of the usable flow formats
> (OpenFlow10,NXM) is among the allowed
> ])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([reg_load <-> set_field translation corner case])
> +AT_KEYWORDS([ofp-actions])
> +OVS_VSWITCHD_START
> +dnl In OpenFlow 1.3, set_field always sets all the bits in the field,
> +dnl but when we translate to NXAST_LOAD we need to only set the bits that
> +dnl actually exist (e.g. mpls_label only has 20 bits) otherwise OVS rejects
> +dnl the "load" action as invalid. Check that we do this correctly.
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0
> mpls,actions=set_field:10-\>mpls_label])
> +AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
> +NXST_FLOW reply:
> + mpls actions=load:0xa->OXM_OF_MPLS_LABEL[[]]
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> --
> 1.7.10.4
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
