| Message ID | 20220815071658.1883055-1-wangchuanlei@inspur.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] physical.c: Fix bug of wrong use in vm migration | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
Thanks for finding and fixing this! Acked-by: Mark Michelson <mmichels@redhat.com> On 8/15/22 03:16, wangchuanlei wrote: > In master branch, pointer ofpacts_p is modified or > cleared in setup_activation_strategy will lead the > following codes to unexpected exceptions. > > Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> > --- > controller/physical.c | 40 +++++++++++++++++++--------------------- > 1 file changed, 19 insertions(+), 21 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index a92534a03..552837bcd 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -989,37 +989,36 @@ enum access_type { > static void > setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > ofp_port_t ofport, struct zone_ids *zone_ids, > - struct ovn_desired_flow_table *flow_table, > - struct ofpbuf *ofpacts_p) > + struct ovn_desired_flow_table *flow_table) > { > struct match match = MATCH_CATCHALL_INITIALIZER; > + struct ofpbuf ofpacts; > + ofpbuf_init(&ofpacts, 0); > > /* Unblock the port on ingress RARP. */ > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > match_set_in_port(&match, ofport); > - ofpbuf_clear(ofpacts_p); > > - load_logical_ingress_metadata(binding, zone_ids, ofpacts_p); > + load_logical_ingress_metadata(binding, zone_ids, &ofpacts); > > - size_t ofs = ofpacts_p->size; > - struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts_p); > + 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_p, &ah, sizeof ah); > + ofpbuf_put(&ofpacts, &ah, sizeof ah); > > - ofpacts_p->header = oc; > - oc->userdata_len = ofpacts_p->size - (ofs + sizeof *oc); > - ofpact_finish_CONTROLLER(ofpacts_p, &oc); > - put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > + 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, > binding->header_.uuid.parts[0], > - &match, ofpacts_p, &binding->header_.uuid); > - ofpbuf_clear(ofpacts_p); > + &match, &ofpacts, &binding->header_.uuid); > + ofpbuf_clear(&ofpacts); > > /* Block all non-RARP traffic for the port, both directions. */ > match_init_catchall(&match); > @@ -1027,7 +1026,7 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1000, > binding->header_.uuid.parts[0], > - &match, ofpacts_p, &binding->header_.uuid); > + &match, &ofpacts, &binding->header_.uuid); > > match_init_catchall(&match); > uint32_t dp_key = binding->datapath->tunnel_key; > @@ -1037,7 +1036,9 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 1000, > binding->header_.uuid.parts[0], > - &match, ofpacts_p, &binding->header_.uuid); > + &match, &ofpacts, &binding->header_.uuid); > + > + ofpbuf_uninit(&ofpacts); > } > > static void > @@ -1045,8 +1046,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, > const struct sbrec_chassis *chassis, > uint32_t dp_key, uint32_t port_key, > ofp_port_t ofport, struct zone_ids *zone_ids, > - struct ovn_desired_flow_table *flow_table, > - struct ofpbuf *ofpacts_p) > + struct ovn_desired_flow_table *flow_table) > { > for (size_t i = 0; i < binding->n_additional_chassis; i++) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > @@ -1059,8 +1059,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, > && !pinctrl_is_port_activated(dp_key, port_key)) { > if (!strcmp(strategy, "rarp")) { > setup_rarp_activation_strategy(binding, ofport, > - zone_ids, flow_table, > - ofpacts_p); > + zone_ids, flow_table); > } else { > VLOG_WARN_RL(&rl, > "Unknown activation strategy defined for " > @@ -1391,8 +1390,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > } > > setup_activation_strategy(binding, chassis, dp_key, port_key, > - ofport, &zone_ids, flow_table, > - ofpacts_p); > + ofport, &zone_ids, flow_table); > > /* Remember the size with just strip vlan added so far, > * as we're going to remove this with ofpbuf_pull() later. */
On Thu, Aug 18, 2022 at 5:55 AM Mark Michelson <mmichels@redhat.com> wrote: > > Thanks for finding and fixing this! > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks. I applied this patch to the main branch, Numan > > On 8/15/22 03:16, wangchuanlei wrote: > > In master branch, pointer ofpacts_p is modified or > > cleared in setup_activation_strategy will lead the > > following codes to unexpected exceptions. > > > > Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> > > --- > > controller/physical.c | 40 +++++++++++++++++++--------------------- > > 1 file changed, 19 insertions(+), 21 deletions(-) > > > > diff --git a/controller/physical.c b/controller/physical.c > > index a92534a03..552837bcd 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -989,37 +989,36 @@ enum access_type { > > static void > > setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > ofp_port_t ofport, struct zone_ids *zone_ids, > > - struct ovn_desired_flow_table *flow_table, > > - struct ofpbuf *ofpacts_p) > > + struct ovn_desired_flow_table *flow_table) > > { > > struct match match = MATCH_CATCHALL_INITIALIZER; > > + struct ofpbuf ofpacts; > > + ofpbuf_init(&ofpacts, 0); > > > > /* Unblock the port on ingress RARP. */ > > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > > match_set_in_port(&match, ofport); > > - ofpbuf_clear(ofpacts_p); > > > > - load_logical_ingress_metadata(binding, zone_ids, ofpacts_p); > > + load_logical_ingress_metadata(binding, zone_ids, &ofpacts); > > > > - size_t ofs = ofpacts_p->size; > > - struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts_p); > > + 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_p, &ah, sizeof ah); > > + ofpbuf_put(&ofpacts, &ah, sizeof ah); > > > > - ofpacts_p->header = oc; > > - oc->userdata_len = ofpacts_p->size - (ofs + sizeof *oc); > > - ofpact_finish_CONTROLLER(ofpacts_p, &oc); > > - put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > > + 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, > > binding->header_.uuid.parts[0], > > - &match, ofpacts_p, &binding->header_.uuid); > > - ofpbuf_clear(ofpacts_p); > > + &match, &ofpacts, &binding->header_.uuid); > > + ofpbuf_clear(&ofpacts); > > > > /* Block all non-RARP traffic for the port, both directions. */ > > match_init_catchall(&match); > > @@ -1027,7 +1026,7 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1000, > > binding->header_.uuid.parts[0], > > - &match, ofpacts_p, &binding->header_.uuid); > > + &match, &ofpacts, &binding->header_.uuid); > > > > match_init_catchall(&match); > > uint32_t dp_key = binding->datapath->tunnel_key; > > @@ -1037,7 +1036,9 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > > > ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 1000, > > binding->header_.uuid.parts[0], > > - &match, ofpacts_p, &binding->header_.uuid); > > + &match, &ofpacts, &binding->header_.uuid); > > + > > + ofpbuf_uninit(&ofpacts); > > } > > > > static void > > @@ -1045,8 +1046,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, > > const struct sbrec_chassis *chassis, > > uint32_t dp_key, uint32_t port_key, > > ofp_port_t ofport, struct zone_ids *zone_ids, > > - struct ovn_desired_flow_table *flow_table, > > - struct ofpbuf *ofpacts_p) > > + struct ovn_desired_flow_table *flow_table) > > { > > for (size_t i = 0; i < binding->n_additional_chassis; i++) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > @@ -1059,8 +1059,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, > > && !pinctrl_is_port_activated(dp_key, port_key)) { > > if (!strcmp(strategy, "rarp")) { > > setup_rarp_activation_strategy(binding, ofport, > > - zone_ids, flow_table, > > - ofpacts_p); > > + zone_ids, flow_table); > > } else { > > VLOG_WARN_RL(&rl, > > "Unknown activation strategy defined for " > > @@ -1391,8 +1390,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > } > > > > setup_activation_strategy(binding, chassis, dp_key, port_key, > > - ofport, &zone_ids, flow_table, > > - ofpacts_p); > > + ofport, &zone_ids, flow_table); > > > > /* Remember the size with just strip vlan added so far, > > * as we're going to remove this with ofpbuf_pull() later. */ > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Aug 17, 2022 at 7:23 PM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Aug 18, 2022 at 5:55 AM Mark Michelson <mmichels@redhat.com> wrote: > > > > Thanks for finding and fixing this! > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks. I applied this patch to the main branch, > > Numan > A heap-buffer-overflow is detected by ASAN for this patch (CI is broken now). I sent a fix here: https://patchwork.ozlabs.org/project/ovn/patch/20220819020747.1563111-1-hzhou@ovn.org/ Thanks, Han > > > > On 8/15/22 03:16, wangchuanlei wrote: > > > In master branch, pointer ofpacts_p is modified or > > > cleared in setup_activation_strategy will lead the > > > following codes to unexpected exceptions. > > > > > > Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> > > > --- > > > controller/physical.c | 40 +++++++++++++++++++--------------------- > > > 1 file changed, 19 insertions(+), 21 deletions(-) > > > > > > diff --git a/controller/physical.c b/controller/physical.c > > > index a92534a03..552837bcd 100644 > > > --- a/controller/physical.c > > > +++ b/controller/physical.c > > > @@ -989,37 +989,36 @@ enum access_type { > > > static void > > > setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > > ofp_port_t ofport, struct zone_ids *zone_ids, > > > - struct ovn_desired_flow_table *flow_table, > > > - struct ofpbuf *ofpacts_p) > > > + struct ovn_desired_flow_table *flow_table) > > > { > > > struct match match = MATCH_CATCHALL_INITIALIZER; > > > + struct ofpbuf ofpacts; > > > + ofpbuf_init(&ofpacts, 0); > > > > > > /* Unblock the port on ingress RARP. */ > > > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > > > match_set_in_port(&match, ofport); > > > - ofpbuf_clear(ofpacts_p); > > > > > > - load_logical_ingress_metadata(binding, zone_ids, ofpacts_p); > > > + load_logical_ingress_metadata(binding, zone_ids, &ofpacts); > > > > > > - size_t ofs = ofpacts_p->size; > > > - struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts_p); > > > + 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_p, &ah, sizeof ah); > > > + ofpbuf_put(&ofpacts, &ah, sizeof ah); > > > > > > - ofpacts_p->header = oc; > > > - oc->userdata_len = ofpacts_p->size - (ofs + sizeof *oc); > > > - ofpact_finish_CONTROLLER(ofpacts_p, &oc); > > > - put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > > > + 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, > > > binding->header_.uuid.parts[0], > > > - &match, ofpacts_p, &binding->header_.uuid); > > > - ofpbuf_clear(ofpacts_p); > > > + &match, &ofpacts, &binding->header_.uuid); > > > + ofpbuf_clear(&ofpacts); > > > > > > /* Block all non-RARP traffic for the port, both directions. */ > > > match_init_catchall(&match); > > > @@ -1027,7 +1026,7 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > > > > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1000, > > > binding->header_.uuid.parts[0], > > > - &match, ofpacts_p, &binding->header_.uuid); > > > + &match, &ofpacts, &binding->header_.uuid); > > > > > > match_init_catchall(&match); > > > uint32_t dp_key = binding->datapath->tunnel_key; > > > @@ -1037,7 +1036,9 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, > > > > > > ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 1000, > > > binding->header_.uuid.parts[0], > > > - &match, ofpacts_p, &binding->header_.uuid); > > > + &match, &ofpacts, &binding->header_.uuid); > > > + > > > + ofpbuf_uninit(&ofpacts); > > > } > > > > > > static void > > > @@ -1045,8 +1046,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, > > > const struct sbrec_chassis *chassis, > > > uint32_t dp_key, uint32_t port_key, > > > ofp_port_t ofport, struct zone_ids *zone_ids, > > > - struct ovn_desired_flow_table *flow_table, > > > - struct ofpbuf *ofpacts_p) > > > + struct ovn_desired_flow_table *flow_table) > > > { > > > for (size_t i = 0; i < binding->n_additional_chassis; i++) { > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > @@ -1059,8 +1059,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, > > > && !pinctrl_is_port_activated(dp_key, port_key)) { > > > if (!strcmp(strategy, "rarp")) { > > > setup_rarp_activation_strategy(binding, ofport, > > > - zone_ids, flow_table, > > > - ofpacts_p); > > > + zone_ids, flow_table); > > > } else { > > > VLOG_WARN_RL(&rl, > > > "Unknown activation strategy defined for " > > > @@ -1391,8 +1390,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > } > > > > > > setup_activation_strategy(binding, chassis, dp_key, port_key, > > > - ofport, &zone_ids, flow_table, > > > - ofpacts_p); > > > + ofport, &zone_ids, flow_table); > > > > > > /* Remember the size with just strip vlan added so far, > > > * as we're going to remove this with ofpbuf_pull() later. */ > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > 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 a92534a03..552837bcd 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -989,37 +989,36 @@ enum access_type { static void setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, ofp_port_t ofport, struct zone_ids *zone_ids, - struct ovn_desired_flow_table *flow_table, - struct ofpbuf *ofpacts_p) + struct ovn_desired_flow_table *flow_table) { struct match match = MATCH_CATCHALL_INITIALIZER; + struct ofpbuf ofpacts; + ofpbuf_init(&ofpacts, 0); /* Unblock the port on ingress RARP. */ match_set_dl_type(&match, htons(ETH_TYPE_RARP)); match_set_in_port(&match, ofport); - ofpbuf_clear(ofpacts_p); - load_logical_ingress_metadata(binding, zone_ids, ofpacts_p); + load_logical_ingress_metadata(binding, zone_ids, &ofpacts); - size_t ofs = ofpacts_p->size; - struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts_p); + 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_p, &ah, sizeof ah); + ofpbuf_put(&ofpacts, &ah, sizeof ah); - ofpacts_p->header = oc; - oc->userdata_len = ofpacts_p->size - (ofs + sizeof *oc); - ofpact_finish_CONTROLLER(ofpacts_p, &oc); - put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); + 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, binding->header_.uuid.parts[0], - &match, ofpacts_p, &binding->header_.uuid); - ofpbuf_clear(ofpacts_p); + &match, &ofpacts, &binding->header_.uuid); + ofpbuf_clear(&ofpacts); /* Block all non-RARP traffic for the port, both directions. */ match_init_catchall(&match); @@ -1027,7 +1026,7 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1000, binding->header_.uuid.parts[0], - &match, ofpacts_p, &binding->header_.uuid); + &match, &ofpacts, &binding->header_.uuid); match_init_catchall(&match); uint32_t dp_key = binding->datapath->tunnel_key; @@ -1037,7 +1036,9 @@ setup_rarp_activation_strategy(const struct sbrec_port_binding *binding, ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 1000, binding->header_.uuid.parts[0], - &match, ofpacts_p, &binding->header_.uuid); + &match, &ofpacts, &binding->header_.uuid); + + ofpbuf_uninit(&ofpacts); } static void @@ -1045,8 +1046,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, const struct sbrec_chassis *chassis, uint32_t dp_key, uint32_t port_key, ofp_port_t ofport, struct zone_ids *zone_ids, - struct ovn_desired_flow_table *flow_table, - struct ofpbuf *ofpacts_p) + struct ovn_desired_flow_table *flow_table) { for (size_t i = 0; i < binding->n_additional_chassis; i++) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); @@ -1059,8 +1059,7 @@ setup_activation_strategy(const struct sbrec_port_binding *binding, && !pinctrl_is_port_activated(dp_key, port_key)) { if (!strcmp(strategy, "rarp")) { setup_rarp_activation_strategy(binding, ofport, - zone_ids, flow_table, - ofpacts_p); + zone_ids, flow_table); } else { VLOG_WARN_RL(&rl, "Unknown activation strategy defined for " @@ -1391,8 +1390,7 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, } setup_activation_strategy(binding, chassis, dp_key, port_key, - ofport, &zone_ids, flow_table, - ofpacts_p); + ofport, &zone_ids, flow_table); /* Remember the size with just strip vlan added so far, * as we're going to remove this with ofpbuf_pull() later. */
In master branch, pointer ofpacts_p is modified or cleared in setup_activation_strategy will lead the following codes to unexpected exceptions. Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> --- controller/physical.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-)