Hi Mark, Thank you for reviewing the patch. actually, the test here is less interested in the type update but more in the LSP deletion from the ld->peer_ports list i don't have a way to see if the ld->n_peer_ports updated properly from the tests units when we change the port type, but i know that if something wrong happens in the ovn-controller and ld->peer_ports keeps pointing to the deleted LSP( which is what this patch comes to fix ) the command* "check ovn-nbctl lrp-del lrp"* will cause some invalid memory access and that will lead to a core dump in the ovn-controller and then the test case will fail with dump in the ovn-controller logs. i know it is a bad way to test it like that but as i mentioned i don't have a way to see the content of ld->peer_ports from the test :(.
see comments in the tests below. On Thu, Aug 18, 2022 at 9:18 PM Mark Michelson <[email protected]> wrote: > Hi Mohammad. The code changes look good, but I'm a bit confused about > what the test case is intending to prove. It updates the type of the > LSP, but there is no check to ensure that the change was successful, as > far as I can tell. > > > On 8/16/22 06:56, Mohammad Heib wrote: > > The local_datapath->peer_ports list contains peers pointers > > to lsp<-->lrp ports that are supposed to be router end ports, > > those pointers are added and deleted to the local_datapath->peer_ports > > when logical switch port of type router are added or deleted from the > database. > > > > The deletion and creation of those ports are handled very well when the > LSP type > > is a router, but if in any case, the user has changed the LSP type from > router > > port to any other LSP type the ld->peer_ports will keep pointing to this > port > > and if it was deleted it will keep pointing to invalid memory regions > and that > > could lead to invalid memory access in the ovn-controller. > > > > To solve the above issue this patch will update the > local_dataoath->peer_ports > > whenever a lport is updated. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2077078 > > Co-authored-by: Xavier Simonart <[email protected]> > > Signed-off-by: Mohammad Heib <[email protected]> > > Signed-off-by: Xavier Simonart <[email protected]> > > Signed-off-by: Mohammad Heib <[email protected]> > > --- > > controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++ > > tests/ovn.at | 37 +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 74 insertions(+) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 9f5393a92..1221419a9 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -566,6 +566,39 @@ remove_related_lport(const struct > sbrec_port_binding *pb, > > } > > } > > > > +/* > > + * Update local_datapath peers when port type changed > > + * and remove irrelevant ports from this list. > > + */ > > +static void > > +update_ld_peers(const struct sbrec_port_binding *pb, > > + struct hmap *local_datapaths) > > +{ > > + struct local_datapath *ld = > > + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); > > + > > + if (!ld) { > > + return; > > + } > > + > > + /* > > + * This will handle cases where the pb type was explicitly > > + * changed from router type to any other port type and will > > + * remove it from the ld peers list. > > + */ > > + enum en_lport_type type = get_lport_type(pb); > > + int num_peers = ld->n_peer_ports; > > + if (type != LP_PATCH) { > > + remove_local_datapath_peer_port(pb, ld, local_datapaths); > > + if (num_peers != ld->n_peer_ports) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > > + VLOG_DBG_RL(&rl, > > + "removing lport %s from the ld peers list", > > + pb->logical_port); > > + } > > + } > > +} > > + > > static void > > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > > struct shash *ras_pd_map) > > @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in > *b_ctx_in, > > return true; > > } > > > > + if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) > { > > + update_ld_peers(pb, b_ctx_out->local_datapaths); > > + } > > + > > update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > > "ipv6_prefix_delegation"); > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index c8cc8cde4..79eda21d3 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb | grep -c > "00:00:00:00:10:30") = 0]) > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([router port type update and then remove]) > > +ovn_start > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lr-add ro0 > > +check ovn-nbctl lsp-add sw0 sw0-p1 > > +check ovn-nbctl lsp-add sw0 lsp > > +check ovn-nbctl lsp-set-type lsp router > > +check ovn-nbctl lsp-set-options lsp router-port=lrp > > +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1 > > +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 > > +check ovn-nbctl set Logical_Router_Port lrp > ipv6_ra_configs:send_periodic=true \ > > + -- set Logical_Router_Port lrp > ipv6_ra_configs:address_mode=slaac \ > > + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \ > > + -- set Logical_Router_Port lrp ipv6_ra_configs:max_interval=2 \ > > + -- set Logical_Router_Port lrp ipv6_ra_configs:min_interval=1 > > > +check ovn-nbctl lsp-set-type lsp localnet <-------------- > this will delete the lsp from the ld->peer_ports and will delete its peer > lrp from the same list > > +check ovn-nbctl --wait=hv sync > > +check ovn-nbctl lsp-del lsp <--------- > normall LSP deletion ( without this patch the ld->peer_ports will keeps > pointing to deleted LSP invalid memory area) > > +check ovn-nbctl lrp-del lrp <--------- > noremal LRP deletions > > +check ovn-nbctl --wait=hv sync > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > thanks _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
