[ovs-dev] [PATCH] ofp-actions: Only set defined bits when encoding "load" actions.

[Pravin Shelar]

On Thu, Dec 4, 2014 at 2:33 PM, Ben Pfaff <blp at 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 <pshelar at nicira.com>
> Signed-off-by: Ben Pfaff <blp at 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
>