On Mon, 2024-06-10 at 12:54 +0000, Naveen Yerramneni wrote:
> Action length is getting set incorrectly during ct_commit encode
> due to which ct action is getting skipped during phsycial flows
> creation. This issue is noticed only if ct_commit is prefixed
> with other actions.
>
> logical flow: reg8[17] = 1; ct_commit { ct_mark.blocked = 1; };
> without fix: encodes as set_field:0x2000000000000/0x2000000000000-
> >xreg4
> with fix: encodes as set_field:0x2000000000000/0x2000000000000-
> >xreg4,ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1-
> >ct_mark))
>
> Signed-off-by: Naveen Yerramneni <[email protected]>
> ---
> v1:
> - Addressed review comments from Ales.
> ---
> lib/actions.c | 3 +++
> tests/ovn.at | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/lib/actions.c b/lib/actions.c
> index 361d55009..794d2e916 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -760,6 +760,8 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> const struct ovnact_encode_params *ep
> OVS_UNUSED,
> struct ofpbuf *ofpacts)
> {
> + size_t ct_offset = ofpacts->size;
> + ofpbuf_pull(ofpacts, ct_offset);
> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> ct->flags = NX_CT_F_COMMIT;
> ct->recirc_table = NX_CT_RECIRC_NONE;
> @@ -791,6 +793,7 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
> ct = ofpacts->header;
> ofpact_finish(ofpacts, &ct->ofpact);
> + ofpbuf_push_uninit(ofpacts, ct_offset);
Just a thought here. I was just wondering if this wouldn't be a good
opportunity to also replace the "manual" way of finishing the action
encoding (as @amusil referred to it in this review [0]) with the more
explicit approach (example included in [0]).
[0]
https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html
> }
>
> static void
> diff --git a/tests/ovn.at b/tests/ovn.at
> index dc6aafd53..d4f62f487 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1315,6 +1315,9 @@ ct_commit {
> ct_label=0x181716151413121110090807060504030201; };
> 141-bit constant is not compatible with 128-bit field ct_label.
> ct_commit { ip4.dst = 192.168.0.1; };
> Field ip4.dst is not modifiable.
> +reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; };
> + encodes as set_field:0x2000000000000/0x2000000000000-
> >xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1-
> >ct_mark))
> + has prereqs ip
>
> # Legact ct_commit_v1 action.
> ct_commit();
Martin.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
