| Message ID | 20220818094110.160733-1-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] controller: Fix wrong controller action definition | 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 | success | github build: passed |
Thanks for the fix, Ales. Acked-by: Mark Michelson <mmichels@redhat.com> On 8/18/22 05:41, Ales Musil wrote: > The controller action had a wrong wrong userdata_len due to > missing substraction of previous data len. The pointer to oc > was not moved correctly so it could in theory point to wrong > memory. In order to fix that expose encode_start_controller_op > and encode_finish_controller_op which are helper methods > to define the controller action. Use those instead of > manually creating the action. At the same time use stack > buffer for the ofpbuf which should be large enough to hold > related data without any heap allocations. > > Fixes: e52c2451 ("physical.c: Fix bug of wrong use in vm migration") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > controller/physical.c | 19 ++++++------------- > include/ovn/actions.h | 4 ++++ > lib/actions.c | 4 ++-- > 3 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index 552837bcd..bc8c304fe 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -992,8 +992,8 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > struct ovn_desired_flow_table *flow_table) > { > struct match match = MATCH_CATCHALL_INITIALIZER; > - struct ofpbuf ofpacts; > - ofpbuf_init(&ofpacts, 0); > + uint64_t stub[1024 / 8]; > + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); > > /* Unblock the port on ingress RARP. */ > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > @@ -1001,18 +1001,11 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > load_logical_ingress_metadata(binding, zone_ids, &ofpacts); > > - struct ofpact_controller *oc = ofpact_put_CONTROLLER(&ofpacts); > - oc->max_len = UINT16_MAX; > - oc->reason = OFPR_ACTION; > - > - struct action_header ah = { > - .opcode = htonl(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP) > - }; > - ofpbuf_put(&ofpacts, &ah, sizeof ah); > + uint32_t ofs = > + encode_start_controller_op(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP, > + false, NX_CTLR_NO_METER, &ofpacts); > + encode_finish_controller_op(ofs, &ofpacts); > > - ofpacts.header = oc; > - oc->userdata_len = ofpacts.size - (sizeof *oc); > - ofpact_finish_CONTROLLER(&ofpacts, &oc); > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1010, > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 33c319f1c..cbcf4130c 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -829,4 +829,8 @@ void ovnacts_free(struct ovnact[], size_t ovnacts_len); > char *ovnact_op_to_string(uint32_t); > int encode_ra_dnssl_opt(char *data, char *buf, int buf_len); > > +size_t encode_start_controller_op(enum action_opcode opcode, bool pause, > + uint32_t meter_id, struct ofpbuf *ofpacts); > +void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts); > + > #endif /* ovn/actions.h */ > diff --git a/lib/actions.c b/lib/actions.c > index aab044306..5c70ef411 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -78,7 +78,7 @@ ovnact_init(struct ovnact *ovnact, enum ovnact_type type, size_t len) > ovnact->len = len; > } > > -static size_t > +size_t > encode_start_controller_op(enum action_opcode opcode, bool pause, > uint32_t meter_id, struct ofpbuf *ofpacts) > { > @@ -99,7 +99,7 @@ encode_start_controller_op(enum action_opcode opcode, bool pause, > return ofs; > } > > -static void > +void > encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts) > { > struct ofpact_controller *oc = ofpbuf_at_assert(ofpacts, ofs, sizeof *oc);
On Thu, Aug 18, 2022 at 7:40 AM Mark Michelson <mmichels@redhat.com> wrote: > > Thanks for the fix, Ales. > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 8/18/22 05:41, Ales Musil wrote: > > The controller action had a wrong wrong userdata_len due to > > missing substraction of previous data len. The pointer to oc > > was not moved correctly so it could in theory point to wrong > > memory. In order to fix that expose encode_start_controller_op > > and encode_finish_controller_op which are helper methods > > to define the controller action. Use those instead of > > manually creating the action. At the same time use stack > > buffer for the ofpbuf which should be large enough to hold > > related data without any heap allocations. > > Thanks Ales! I am wondering why not expose and call encode_controller_op() directly? Thanks, Han > > Fixes: e52c2451 ("physical.c: Fix bug of wrong use in vm migration") > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > controller/physical.c | 19 ++++++------------- > > include/ovn/actions.h | 4 ++++ > > lib/actions.c | 4 ++-- > > 3 files changed, 12 insertions(+), 15 deletions(-) > > > > diff --git a/controller/physical.c b/controller/physical.c > > index 552837bcd..bc8c304fe 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -992,8 +992,8 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > struct ovn_desired_flow_table *flow_table) > > { > > struct match match = MATCH_CATCHALL_INITIALIZER; > > - struct ofpbuf ofpacts; > > - ofpbuf_init(&ofpacts, 0); > > + uint64_t stub[1024 / 8]; > > + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); > > > > /* Unblock the port on ingress RARP. */ > > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > > @@ -1001,18 +1001,11 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > > > load_logical_ingress_metadata(binding, zone_ids, &ofpacts); > > > > - struct ofpact_controller *oc = ofpact_put_CONTROLLER(&ofpacts); > > - oc->max_len = UINT16_MAX; > > - oc->reason = OFPR_ACTION; > > - > > - struct action_header ah = { > > - .opcode = htonl(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP) > > - }; > > - ofpbuf_put(&ofpacts, &ah, sizeof ah); > > + uint32_t ofs = > > + encode_start_controller_op(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP, > > + false, NX_CTLR_NO_METER, &ofpacts); > > + encode_finish_controller_op(ofs, &ofpacts); > > > > - ofpacts.header = oc; > > - oc->userdata_len = ofpacts.size - (sizeof *oc); > > - ofpact_finish_CONTROLLER(&ofpacts, &oc); > > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); > > > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1010, > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > > index 33c319f1c..cbcf4130c 100644 > > --- a/include/ovn/actions.h > > +++ b/include/ovn/actions.h > > @@ -829,4 +829,8 @@ void ovnacts_free(struct ovnact[], size_t ovnacts_len); > > char *ovnact_op_to_string(uint32_t); > > int encode_ra_dnssl_opt(char *data, char *buf, int buf_len); > > > > +size_t encode_start_controller_op(enum action_opcode opcode, bool pause, > > + uint32_t meter_id, struct ofpbuf *ofpacts); > > +void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts); > > + > > #endif /* ovn/actions.h */ > > diff --git a/lib/actions.c b/lib/actions.c > > index aab044306..5c70ef411 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -78,7 +78,7 @@ ovnact_init(struct ovnact *ovnact, enum ovnact_type type, size_t len) > > ovnact->len = len; > > } > > > > -static size_t > > +size_t > > encode_start_controller_op(enum action_opcode opcode, bool pause, > > uint32_t meter_id, struct ofpbuf *ofpacts) > > { > > @@ -99,7 +99,7 @@ encode_start_controller_op(enum action_opcode opcode, bool pause, > > return ofs; > > } > > > > -static void > > +void > > encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts) > > { > > struct ofpact_controller *oc = ofpbuf_at_assert(ofpacts, ofs, sizeof *oc); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Aug 19, 2022 atAs the 7:53 AM Han Zhou <zhouhan@gmail.com> wrote: > > > On Thu, Aug 18, 2022 at 7:40 AM Mark Michelson <mmichels@redhat.com> > wrote: > > > > Thanks for the fix, Ales. > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > > On 8/18/22 05:41, Ales Musil wrote: > > > The controller action had a wrong wrong userdata_len due to > > > missing substraction of previous data len. The pointer to oc > > > was not moved correctly so it could in theory point to wrong > > > memory. In order to fix that expose encode_start_controller_op > > > and encode_finish_controller_op which are helper methods > > > to define the controller action. Use those instead of > > > manually creating the action. At the same time use stack > > > buffer for the ofpbuf which should be large enough to hold > > > related data without any heap allocations. > > > > > Thanks Ales! I am wondering why not expose and call encode_controller_op() > directly? > You are right, that is actually better, posted v3. Thanks, Ales > > Thanks, > Han > > > > Fixes: e52c2451 ("physical.c: Fix bug of wrong use in vm migration") > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > > --- > > > controller/physical.c | 19 ++++++------------- > > > include/ovn/actions.h | 4 ++++ > > > lib/actions.c | 4 ++-- > > > 3 files changed, 12 insertions(+), 15 deletions(-) > > > > > > diff --git a/controller/physical.c b/controller/physical.c > > > index 552837bcd..bc8c304fe 100644 > > > --- a/controller/physical.c > > > +++ b/controller/physical.c > > > @@ -992,8 +992,8 @@ setup_rarp_activation_strategy(const struct > sbrec_port_binding *binding, > > > struct ovn_desired_flow_table > *flow_table) > > > { > > > struct match match = MATCH_CATCHALL_INITIALIZER; > > > - struct ofpbuf ofpacts; > > > - ofpbuf_init(&ofpacts, 0); > > > + uint64_t stub[1024 / 8]; > > > + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); > > > > > > /* Unblock the port on ingress RARP. */ > > > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > > > @@ -1001,18 +1001,11 @@ setup_rarp_activation_strategy(const struct > sbrec_port_binding *binding, > > > > > > load_logical_ingress_metadata(binding, zone_ids, &ofpacts); > > > > > > - struct ofpact_controller *oc = ofpact_put_CONTROLLER(&ofpacts); > > > - oc->max_len = UINT16_MAX; > > > - oc->reason = OFPR_ACTION; > > > - > > > - struct action_header ah = { > > > - .opcode = htonl(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP) > > > - }; > > > - ofpbuf_put(&ofpacts, &ah, sizeof ah); > > > + uint32_t ofs = > > > + > encode_start_controller_op(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP, > > > + false, NX_CTLR_NO_METER, &ofpacts); > > > + encode_finish_controller_op(ofs, &ofpacts); > > > > > > - ofpacts.header = oc; > > > - oc->userdata_len = ofpacts.size - (sizeof *oc); > > > - ofpact_finish_CONTROLLER(&ofpacts, &oc); > > > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); > > > > > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1010, > > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > > > index 33c319f1c..cbcf4130c 100644 > > > --- a/include/ovn/actions.h > > > +++ b/include/ovn/actions.h > > > @@ -829,4 +829,8 @@ void ovnacts_free(struct ovnact[], size_t > ovnacts_len); > > > char *ovnact_op_to_string(uint32_t); > > > int encode_ra_dnssl_opt(char *data, char *buf, int buf_len); > > > > > > +size_t encode_start_controller_op(enum action_opcode opcode, bool > pause, > > > + uint32_t meter_id, struct ofpbuf > *ofpacts); > > > +void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts); > > > + > > > #endif /* ovn/actions.h */ > > > diff --git a/lib/actions.c b/lib/actions.c > > > index aab044306..5c70ef411 100644 > > > --- a/lib/actions.c > > > +++ b/lib/actions.c > > > @@ -78,7 +78,7 @@ ovnact_init(struct ovnact *ovnact, enum ovnact_type > type, size_t len) > > > ovnact->len = len; > > > } > > > > > > -static size_t > > > +size_t > > > encode_start_controller_op(enum action_opcode opcode, bool pause, > > > uint32_t meter_id, struct ofpbuf *ofpacts) > > > { > > > @@ -99,7 +99,7 @@ encode_start_controller_op(enum action_opcode > opcode, bool pause, > > > return ofs; > > > } > > > > > > -static void > > > +void > > > encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts) > > > { > > > struct ofpact_controller *oc = ofpbuf_at_assert(ofpacts, ofs, > sizeof *oc); > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/physical.c b/controller/physical.c index 552837bcd..bc8c304fe 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -992,8 +992,8 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, struct ovn_desired_flow_table *flow_table) { struct match match = MATCH_CATCHALL_INITIALIZER; - struct ofpbuf ofpacts; - ofpbuf_init(&ofpacts, 0); + uint64_t stub[1024 / 8]; + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); /* Unblock the port on ingress RARP. */ match_set_dl_type(&match, htons(ETH_TYPE_RARP)); @@ -1001,18 +1001,11 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, load_logical_ingress_metadata(binding, zone_ids, &ofpacts); - struct ofpact_controller *oc = ofpact_put_CONTROLLER(&ofpacts); - oc->max_len = UINT16_MAX; - oc->reason = OFPR_ACTION; - - struct action_header ah = { - .opcode = htonl(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP) - }; - ofpbuf_put(&ofpacts, &ah, sizeof ah); + uint32_t ofs = + encode_start_controller_op(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP, + false, NX_CTLR_NO_METER, &ofpacts); + encode_finish_controller_op(ofs, &ofpacts); - ofpacts.header = oc; - oc->userdata_len = ofpacts.size - (sizeof *oc); - ofpact_finish_CONTROLLER(&ofpacts, &oc); put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1010, diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 33c319f1c..cbcf4130c 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -829,4 +829,8 @@ void ovnacts_free(struct ovnact[], size_t ovnacts_len); char *ovnact_op_to_string(uint32_t); int encode_ra_dnssl_opt(char *data, char *buf, int buf_len); +size_t encode_start_controller_op(enum action_opcode opcode, bool pause, + uint32_t meter_id, struct ofpbuf *ofpacts); +void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts); + #endif /* ovn/actions.h */ diff --git a/lib/actions.c b/lib/actions.c index aab044306..5c70ef411 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -78,7 +78,7 @@ ovnact_init(struct ovnact *ovnact, enum ovnact_type type, size_t len) ovnact->len = len; } -static size_t +size_t encode_start_controller_op(enum action_opcode opcode, bool pause, uint32_t meter_id, struct ofpbuf *ofpacts) { @@ -99,7 +99,7 @@ encode_start_controller_op(enum action_opcode opcode, bool pause, return ofs; } -static void +void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts) { struct ofpact_controller *oc = ofpbuf_at_assert(ofpacts, ofs, sizeof *oc);
The controller action had a wrong wrong userdata_len due to missing substraction of previous data len. The pointer to oc was not moved correctly so it could in theory point to wrong memory. In order to fix that expose encode_start_controller_op and encode_finish_controller_op which are helper methods to define the controller action. Use those instead of manually creating the action. At the same time use stack buffer for the ofpbuf which should be large enough to hold related data without any heap allocations. Fixes: e52c2451 ("physical.c: Fix bug of wrong use in vm migration") Signed-off-by: Ales Musil <amusil@redhat.com> --- controller/physical.c | 19 ++++++------------- include/ovn/actions.h | 4 ++++ lib/actions.c | 4 ++-- 3 files changed, 12 insertions(+), 15 deletions(-)