On Thu, Aug 25, 2022 at 10:05 AM Han Zhou <zhou...@gmail.com> wrote:
>
>
>
> On Thu, Aug 25, 2022 at 7:47 AM Xavier Simonart <xsimo...@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 <xsimo...@redhat.com>
> >
> > ---
> > v2: handled Han's comments (avoid wasting CPU cycles searching for
peer_ld)
> > ---
> > controller/local_data.c | 36 ++++++++++++++----------------------
> > tests/ovn.at | 30 ++++++++++++++++++++++++++++++
> > 2 files changed, 44 insertions(+), 22 deletions(-)
> >
> > diff --git a/controller/local_data.c b/controller/local_data.c
> > index 7f874fc19..669e686ab 100644
> > --- a/controller/local_data.c
> > +++ b/controller/local_data.c
> > @@ -34,7 +34,7 @@
> >
> > VLOG_DEFINE_THIS_MODULE(ldata);
> >
> > -static void add_local_datapath__(
> > +static struct local_datapath *add_local_datapath__(
> > struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> > struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > @@ -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);
> > }
> >
> > @@ -541,7 +525,7 @@ chassis_tunnel_find(const struct hmap
*chassis_tunnels, const char *chassis_id,
> > }
> >
> > /* static functions. */
> > -static void
> > +static struct local_datapath *
> > add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
> > struct ovsdb_idl_index
*sbrec_port_binding_by_datapath,
> > struct ovsdb_idl_index
*sbrec_port_binding_by_name,
> > @@ -553,7 +537,7 @@ add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
> > uint32_t dp_key = dp->tunnel_key;
> > struct local_datapath *ld = get_local_datapath(local_datapaths,
dp_key);
> > if (ld) {
> > - return;
> > + return ld;
> > }
> >
> > ld = local_datapath_alloc(dp);
> > @@ -568,7 +552,7 @@ add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
> > if (depth >= 100) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > VLOG_WARN_RL(&rl, "datapaths nested too deep");
> > - return;
> > + return ld;
> > }
> >
> > struct sbrec_port_binding *target =
> > @@ -589,12 +573,14 @@ add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
> > if (peer && peer->datapath) {
> > if (need_add_patch_peer_to_local(
> > sbrec_port_binding_by_name, pb, chassis)) {
> > -
add_local_datapath__(sbrec_datapath_binding_by_key,
> > + struct local_datapath *peer_ld =
> > +
add_local_datapath__(sbrec_datapath_binding_by_key,
> >
sbrec_port_binding_by_datapath,
> >
sbrec_port_binding_by_name,
> > depth + 1, peer->datapath,
> > chassis, local_datapaths,
> > tracked_datapaths);
> > + local_datapath_peer_port_add(peer_ld, peer,
pb);
>
> Thanks Xavier for the refactor. Now that it is cleaner, it reminds me of
a potential problem that when the peer DP doesn't need to be added to
local_datapaths, we may end up with the same crash because the peer is
added as peer of the pb but the pb is not added as peer of the peer (I am
sorry that this reads confusing). So when the peer is deleted, it won't
remove itself from the pb's peer, and pb's peer would be a dangling
pointer. I would be safe only if we make sure they are always added as
peers from both sides, or not added at all. However, if we move the below
local_datapath_peer_port_add(ld, pb, peer); under this if condition, it is
possible that when we do need the peer port information it is unavailable
from the local_datapath. I am going through all use cases of the peer port
structure before concluding.
>
Hi Xavier,
After going through the use cases of the ld->peer_ports, for what I can
tell, the data is used by pinctrl.c and physical.c for DGPs when both the
LR and the LS are local, so I think we should move the below
"local_datapath_peer_port_add(ld, pb, peer);" into the above "if"
condition. Would you like to update with v3?
Thanks,
Han
> Han
>
> > }
> > local_datapath_peer_port_add(ld, pb, peer);
> > }
> > @@ -602,6 +588,7 @@ add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
> > }
> > }
> > sbrec_port_binding_index_destroy_row(target);
> > + return ld;
> > }
> >
> > static struct tracked_datapath *
> > @@ -622,6 +609,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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
