| Message ID | 20220823104415.2197632-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] controller: fix potential segmentation violation when removing ports | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Aug 23, 2022 at 3:44 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > If a logical switch port is added and connected to a logical router > port (through options: router-port) before the router port is > created, then this might cause further issues such as segmentation > violation when the switch and router ports are deleted. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> Thanks Xavier. Great catch! Please see minor comments below. > --- > controller/local_data.c | 30 +++++++++++++----------------- > tests/ovn.at | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 17 deletions(-) > > diff --git a/controller/local_data.c b/controller/local_data.c > index 7f874fc19..c5a948702 100644 > --- a/controller/local_data.c > +++ b/controller/local_data.c > @@ -194,17 +194,7 @@ add_local_datapath_peer_port( > return; > } > > - bool present = false; > - for (size_t i = 0; i < ld->n_peer_ports; i++) { > - if (ld->peer_ports[i].local == pb) { > - present = true; > - break; > - } > - } > - > - if (!present) { > - local_datapath_peer_port_add(ld, pb, peer); > - } > + local_datapath_peer_port_add(ld, pb, peer); > > struct local_datapath *peer_ld = > get_local_datapath(local_datapaths, > @@ -218,12 +208,6 @@ add_local_datapath_peer_port( > return; > } > > - for (size_t i = 0; i < peer_ld->n_peer_ports; i++) { > - if (peer_ld->peer_ports[i].local == peer) { > - return; > - } > - } > - > local_datapath_peer_port_add(peer_ld, peer, pb); > } > > @@ -597,6 +581,13 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > tracked_datapaths); > } > local_datapath_peer_port_add(ld, pb, peer); > + struct local_datapath *peer_ld = > + get_local_datapath(local_datapaths, > + peer->datapath->tunnel_key); > + Since we just added the peer datapath to local_datapaths several lines above, it seems really a waste to look it up again. It would be nice to return the *ld* newly created (or existed one) from this function add_local_datapath__() directly. > + if (peer_ld != NULL) { nit: the style of this project is usually "if (peer_ld) ...". With the above: Acked-by: Han Zhou <hzhou@ovn.org> > + local_datapath_peer_port_add(peer_ld, peer, pb); > + } > } > } > } > @@ -622,6 +613,11 @@ local_datapath_peer_port_add(struct local_datapath *ld, > const struct sbrec_port_binding *local, > const struct sbrec_port_binding *remote) > { > + for (size_t i = 0; i < ld->n_peer_ports; i++) { > + if (ld->peer_ports[i].local == local) { > + return; > + } > + } > ld->n_peer_ports++; > if (ld->n_peer_ports > ld->n_allocated_peer_ports) { > size_t old_n_ports = ld->n_allocated_peer_ports; > diff --git a/tests/ovn.at b/tests/ovn.at > index bba2c9c1d..ae0918d55 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32580,3 +32580,33 @@ check ovn-nbctl --wait=hv sync > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([router port add then remove - lsp first]) > +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 --wait=hv lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 > +check ovn-nbctl --wait=hv lsp-del lsp > +check ovn-nbctl lrp-del lrp > +check ovn-nbctl --wait=hv sync > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller/local_data.c b/controller/local_data.c index 7f874fc19..c5a948702 100644 --- a/controller/local_data.c +++ b/controller/local_data.c @@ -194,17 +194,7 @@ add_local_datapath_peer_port( return; } - bool present = false; - for (size_t i = 0; i < ld->n_peer_ports; i++) { - if (ld->peer_ports[i].local == pb) { - present = true; - break; - } - } - - if (!present) { - local_datapath_peer_port_add(ld, pb, peer); - } + local_datapath_peer_port_add(ld, pb, peer); struct local_datapath *peer_ld = get_local_datapath(local_datapaths, @@ -218,12 +208,6 @@ add_local_datapath_peer_port( return; } - for (size_t i = 0; i < peer_ld->n_peer_ports; i++) { - if (peer_ld->peer_ports[i].local == peer) { - return; - } - } - local_datapath_peer_port_add(peer_ld, peer, pb); } @@ -597,6 +581,13 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, tracked_datapaths); } local_datapath_peer_port_add(ld, pb, peer); + struct local_datapath *peer_ld = + get_local_datapath(local_datapaths, + peer->datapath->tunnel_key); + + if (peer_ld != NULL) { + local_datapath_peer_port_add(peer_ld, peer, pb); + } } } } @@ -622,6 +613,11 @@ local_datapath_peer_port_add(struct local_datapath *ld, const struct sbrec_port_binding *local, const struct sbrec_port_binding *remote) { + for (size_t i = 0; i < ld->n_peer_ports; i++) { + if (ld->peer_ports[i].local == local) { + return; + } + } ld->n_peer_ports++; if (ld->n_peer_ports > ld->n_allocated_peer_ports) { size_t old_n_ports = ld->n_allocated_peer_ports; diff --git a/tests/ovn.at b/tests/ovn.at index bba2c9c1d..ae0918d55 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32580,3 +32580,33 @@ check ovn-nbctl --wait=hv sync OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([router port add then remove - lsp first]) +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 --wait=hv lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 +check ovn-nbctl --wait=hv lsp-del lsp +check ovn-nbctl lrp-del lrp +check ovn-nbctl --wait=hv sync +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
If a logical switch port is added and connected to a logical router port (through options: router-port) before the router port is created, then this might cause further issues such as segmentation violation when the switch and router ports are deleted. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/local_data.c | 30 +++++++++++++----------------- tests/ovn.at | 30 ++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 17 deletions(-)