[ovs-dev] ofp-actions: Store raw type for NXAST_LEARN2.

Previously, if a controller wrote a flow with action NXAST_LEARN2, then
OVS would internally store an ofpact_learn structure with the raw type
set to NXAST_LEARN. When re-encoding, if the learn action happened to
have a limit or dst_ofs specified (which can only be specified for
NXAST_LEARN2), then it would re-encode using NXAST_LEARN2. However, if
these fields were both zero then OVS relies on the ofpact 'raw' type to
re-encode the action, so would end up encoding it as NXAST_LEARN in
subsequent serialization.

Fix this issue by storing the raw type when decoding learn actions.

VMWare-BZ: #1897275
Fixes: 4c71600d2256 ("ofp-actions: Add limit to learn action.")
Reported-by: Harold Lim <haroldl@vmware.com>
Signed-off-by: Joe Stringer <joe@ovn.org>
---
 lib/ofp-actions.c    | 6 ++++--
 tests/ofp-actions.at | 7 +++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

Thanks for the patch. I think it does address the aforementioned issue.
The testcase may not verify the action re-encoding.  We can always
figure out a new testcase and an easier way to verify action re-encoding
later on.

Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>

On Tue, Jun 20, 2017 at 3:17 PM, Joe Stringer <joe@ovn.org> wrote:
> Previously, if a controller wrote a flow with action NXAST_LEARN2, then
> OVS would internally store an ofpact_learn structure with the raw type
> set to NXAST_LEARN. When re-encoding, if the learn action happened to
> have a limit or dst_ofs specified (which can only be specified for
> NXAST_LEARN2), then it would re-encode using NXAST_LEARN2. However, if
> these fields were both zero then OVS relies on the ofpact 'raw' type to
> re-encode the action, so would end up encoding it as NXAST_LEARN in
> subsequent serialization.
>
> Fix this issue by storing the raw type when decoding learn actions.
>
> VMWare-BZ: #1897275
> Fixes: 4c71600d2256 ("ofp-actions: Add limit to learn action.")
> Reported-by: Harold Lim <haroldl@vmware.com>
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
>  lib/ofp-actions.c    | 6 ++++--
>  tests/ofp-actions.at | 7 +++++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index a66cadcb50e1..ae27d9d88080 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -4541,12 +4541,14 @@ learn_min_len(uint16_t header)
>
>  static enum ofperr
>  decode_LEARN_common(const struct nx_action_learn *nal,
> +                    enum ofp_raw_action_type raw,
>                      struct ofpact_learn *learn)
>  {
>      if (nal->pad) {
>          return OFPERR_OFPBAC_BAD_ARGUMENT;
>      }
>
> +    learn->ofpact.raw = raw;
>      learn->idle_timeout = ntohs(nal->idle_timeout);
>      learn->hard_timeout = ntohs(nal->hard_timeout);
>      learn->priority = ntohs(nal->priority);
> @@ -4658,7 +4660,7 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
>
>      learn = ofpact_put_LEARN(ofpacts);
>
> -    error = decode_LEARN_common(nal, learn);
> +    error = decode_LEARN_common(nal, NXAST_RAW_LEARN, learn);
>      if (error) {
>          return error;
>      }
> @@ -4689,7 +4691,7 @@ decode_NXAST_RAW_LEARN2(const struct nx_action_learn2 *nal,
>      }
>
>      learn = ofpact_put_LEARN(ofpacts);
> -    error = decode_LEARN_common(&nal->up, learn);
> +    error = decode_LEARN_common(&nal->up, NXAST_RAW_LEARN2, learn);
>      if (error) {
>          return error;
>      }
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index 11b36537b1c8..e320a92a8f6f 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -276,6 +276,13 @@ ffff 0020 00002320 002a 000000000000 dnl
>  0001 0008 0005 0000 dnl
>  0000 0008 000a 0000
>
> +# actions=learn(table=2,priority=0,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[])
> +ffff 0050 00002320 002d 0000 0000 0000 0000000000000000 0000 02 00 0000 0000 00000000 0000 0000 dnl
> +000c 00000802 0000 00000802 0000 dnl
> +0030 00000406 0000 00000206 0000 dnl
> +1010 00000002 0000 dnl
> +00000000
> +
>  # actions=learn(table=2,idle_timeout=10,hard_timeout=20,fin_idle_timeout=2,fin_hard_timeout=4,priority=80,cookie=0x123456789abcdef0,limit=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[])
>  ffff 0050 00002320 002d 000a 0014 0050 123456789abcdef0 0000 02 00 0002 0004 00000001 0000 0000 dnl
>  000c 00000802 0000 00000802 0000 dnl
> --
> 2.11.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 21 June 2017 at 15:30, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> Thanks for the patch. I think it does address the aforementioned issue.
> The testcase may not verify the action re-encoding.  We can always
> figure out a new testcase and an easier way to verify action re-encoding
> later on.
>
> Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>

I thought so too about the verification of action re-encoding, but
then I reverted the core change and kept the test, and I found that
the test actually fails otherwise.

See here:
https://github.com/openvswitch/ovs/blob/v2.7.0/utilities/ovs-ofctl.c#L3930

/* "parse-actions VERSION": reads a series of action specifications for the
* given OpenFlow VERSION as hex bytes from stdin, converts them to ofpacts,
* prints them as strings on stdout, and then converts them back to hex bytes
* and prints any differences from the input. */

Thanks for the review, applied to master.