On 6/10/24 18:05, [email protected] wrote:
> 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]>
>> ---
Thanks Naveen, for the catch!
>> v1:
Well, this should actually be v2 but that's a technicality.
>> - 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);
Nit: missing new line (but I can fix that up at apply time).
>> 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]).
>
I agree that we should do that but we should do it in multiple
functions, e.g.:
encode_CT_NEXT()
encode_ct_nat()
encode_ct_lb()
I think it's fine to do that in a follow up patch. Martin, do you have
time to post one? Otherwise I'll put it on my list of things to do in
the future.
Regarding the current bug fix I'm planning to apply and backport this
patch, pending ovsrobot re-run results.
Recheck-request: github-robot
Regards,
Dumitru
> [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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
