Han, Ales， yes, the commit has a problem, Thanks for finding and fix! wangchuanlei On Fri, Aug 19, 2022 atAs the 7:53 AM Han Zhou <zhouhan at gmail.com <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > wrote: > > > On Thu, Aug 18, 2022 at 7:40 AM Mark Michelson <mmichels at redhat.com <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > wrote: > > > > Thanks for the fix, Ales. > > > > Acked-by: Mark Michelson <mmichels at redhat.com <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > > > 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 at redhat.com <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > > --- > > > 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 at openvswitch.org <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com <https://www.redhat.com/> > amusil at redhat.com <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> IM: amusil <https://red.ht/sig>
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
