| Message ID | 20240610125421.23461-1-naveen.yerramneni@nutanix.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v1] controller: Fix issue with ct_commit encode. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
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 <naveen.yerramneni@nutanix.com> > --- > 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.
On 6/10/24 18:05, martin.kalcok@canonical.com 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 <naveen.yerramneni@nutanix.com> >> --- 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 6/11/24 00:07, Dumitru Ceara wrote: > On 6/10/24 18:05, martin.kalcok@canonical.com 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 <naveen.yerramneni@nutanix.com> >>> --- > > 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. Tests passed. I applied this to main and all supported stable branches. Best regards, Dumitru
> On 11 Jun 2024, at 00:07, Dumitru Ceara <dceara@redhat.com> wrote: > > On 6/10/24 18:05, martin.kalcok@canonical.com <mailto:martin.kalcok@canonical.com> 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 <naveen.yerramneni@nutanix.com> >>> --- > > 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. Sure, I’ll find some time to make the proposal this week. > > 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 >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev Martin.
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); } 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();
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 <naveen.yerramneni@nutanix.com> --- v1: - Addressed review comments from Ales. --- lib/actions.c | 3 +++ tests/ovn.at | 3 +++ 2 files changed, 6 insertions(+)