Hi Mohammad, I have some questions and comments, see inline.
On Wed, Aug 3, 2022 at 4:35 PM Mohammad Heib <mh...@redhat.com> 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 <xsimo...@redhat.com> > Signed-off-by: Mohammad Heib <mh...@redhat.com> > Signed-off-by: Xavier Simonart <xsimo...@redhat.com> > --- > controller/binding.c | 33 +++++++++++++++++++++++++++++++++ > tests/ovn.at | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 19b28369f..3bd8f2ada 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -505,6 +505,38 @@ 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); > I think the correct formatting is: 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) > @@ -2612,6 +2644,7 @@ delete_done: > continue; > } > > + update_ld_peers(pb, b_ctx_out->local_datapaths); > If I understand that right this happens only during I-P right? If that is the case we could actually avoid running the update_ld_peers for every port that is not LP_PATCH. By using "sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)" we could check only the port bindings that changed the type between idl loop, WDYT? > 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 3ba6ced4e..54673e9ec 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32349,3 +32349,36 @@ 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 > Could you please break this line? Probably after every option will be fine. > +check ovn-nbctl lsp-set-type lsp localnet > +check ovn-nbctl --wait=hv sync > +check ovn-nbctl lsp-del lsp > +check ovn-nbctl lrp-del lrp > +check ovn-nbctl --wait=hv sync > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.34.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
