| Message ID | 20230420052408.1318505-1-taoliu828@163.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: fix use-after-free after lrp destroyed | expand |
On Thu, Apr 20, 2023 at 01:24:08PM +0800, Tao Liu wrote: > In vxlan mode with more than 2047 lrp in router, build_ports() > prints "all port tunnel ids exhausted", and frees the lrp port. > > However, lsp of type "router" connected to the lrp still holds > the pointer in port->peer. This leads to northd crash in > build_lflows(). > > CallTrace: > build_lswitch_rport_arp_req_flows > build_lswitch_ip_unicast_lookup > build_lswitch_and_lrouter_iterate_by_op > build_lflows > en_lflow_run > engine_recompute > engine_run > inc_proc_northd_run > main > > Fixes: 3044132261d3 ("northd: Enhance implementation of port tunnel key requests.") > Signed-off-by: Tao Liu <taoliu828@163.com> > --- > northd/northd.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/northd/northd.c b/northd/northd.c > index c10e5c20c..fe36a0362 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4598,6 +4598,9 @@ ovn_port_allocate_key(const struct sbrec_chassis_table *sbrec_chassis_table, > if (op->sb) { > sbrec_port_binding_delete(op->sb); > } > + if (op->peer) { > + op->peer->peer = NULL; > + } Thanks for your patch. I wonder if it is appropriate to clear op->peer->peer in ovn_port_destroy(). > ovs_list_remove(&op->list); > ovn_port_destroy(ports, op); > } > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 4/24/23 15:06, Simon Horman wrote: > On Thu, Apr 20, 2023 at 01:24:08PM +0800, Tao Liu wrote: >> In vxlan mode with more than 2047 lrp in router, build_ports() >> prints "all port tunnel ids exhausted", and frees the lrp port. >> >> However, lsp of type "router" connected to the lrp still holds >> the pointer in port->peer. This leads to northd crash in >> build_lflows(). >> >> CallTrace: >> build_lswitch_rport_arp_req_flows >> build_lswitch_ip_unicast_lookup >> build_lswitch_and_lrouter_iterate_by_op >> build_lflows >> en_lflow_run >> engine_recompute >> engine_run >> inc_proc_northd_run >> main >> >> Fixes: 3044132261d3 ("northd: Enhance implementation of port tunnel key requests.") >> Signed-off-by: Tao Liu <taoliu828@163.com> >> --- >> northd/northd.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index c10e5c20c..fe36a0362 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4598,6 +4598,9 @@ ovn_port_allocate_key(const struct sbrec_chassis_table *sbrec_chassis_table, >> if (op->sb) { >> sbrec_port_binding_delete(op->sb); >> } >> + if (op->peer) { >> + op->peer->peer = NULL; >> + } > > Thanks for your patch. > > I wonder if it is appropriate to clear op->peer->peer in ovn_port_destroy(). > I agree, it seems better indeed. Regards, Dumitru
On 4/24/23 9:06 PM, Simon Horman wrote: > On Thu, Apr 20, 2023 at 01:24:08PM +0800, Tao Liu wrote: >> In vxlan mode with more than 2047 lrp in router, build_ports() >> prints "all port tunnel ids exhausted", and frees the lrp port. >> >> However, lsp of type "router" connected to the lrp still holds >> the pointer in port->peer. This leads to northd crash in >> build_lflows(). >> >> CallTrace: >> build_lswitch_rport_arp_req_flows >> build_lswitch_ip_unicast_lookup >> build_lswitch_and_lrouter_iterate_by_op >> build_lflows >> en_lflow_run >> engine_recompute >> engine_run >> inc_proc_northd_run >> main >> >> Fixes: 3044132261d3 ("northd: Enhance implementation of port tunnel key requests.") >> Signed-off-by: Tao Liu <taoliu828@163.com> >> --- >> northd/northd.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index c10e5c20c..fe36a0362 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4598,6 +4598,9 @@ ovn_port_allocate_key(const struct sbrec_chassis_table *sbrec_chassis_table, >> if (op->sb) { >> sbrec_port_binding_delete(op->sb); >> } >> + if (op->peer) { >> + op->peer->peer = NULL; >> + } > Thanks for your patch. > > I wonder if it is appropriate to clear op->peer->peer in ovn_port_destroy(). Thanks for comment, and sorry for late reply. Agree with it, will send V2 >> ovs_list_remove(&op->list); >> ovn_port_destroy(ports, op); >> } >> -- >> 2.31.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
diff --git a/northd/northd.c b/northd/northd.c index c10e5c20c..fe36a0362 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4598,6 +4598,9 @@ ovn_port_allocate_key(const struct sbrec_chassis_table *sbrec_chassis_table, if (op->sb) { sbrec_port_binding_delete(op->sb); } + if (op->peer) { + op->peer->peer = NULL; + } ovs_list_remove(&op->list); ovn_port_destroy(ports, op); }
In vxlan mode with more than 2047 lrp in router, build_ports() prints "all port tunnel ids exhausted", and frees the lrp port. However, lsp of type "router" connected to the lrp still holds the pointer in port->peer. This leads to northd crash in build_lflows(). CallTrace: build_lswitch_rport_arp_req_flows build_lswitch_ip_unicast_lookup build_lswitch_and_lrouter_iterate_by_op build_lflows en_lflow_run engine_recompute engine_run inc_proc_northd_run main Fixes: 3044132261d3 ("northd: Enhance implementation of port tunnel key requests.") Signed-off-by: Tao Liu <taoliu828@163.com> --- northd/northd.c | 3 +++ 1 file changed, 3 insertions(+)