tunnel: Fix deletion of datapath tunnel ports in case of reconfiguration

There is an issue in OVS with tunnel deletion during the
reconfiguration of OF tunnels. If the dst_port value is changed, the
old tunnel map entry will not be deleted, because the tp_port
argument of tnl_port_map_delete() has the new dst_port setting, hence
the tunnel cannot be found in the list of tnl_port structures.

The patch corrects this mechanism by adding a new argument,
'old_odp_port' to tnl_port_reconfigure(). This value is used to
identify the datapath tunnel port which is being reconfigured. In
connection with this fix, to unify the tunnel port map handling,
odp_port value is used to search the proper port to insert and delete
tunnel map entries as well. This variable can be used instead of
tp_port, as it is unique for all datapath tunnel ports, and there is
no need to reach dst_port from netdev_tunnel_config structure.

This patch also adds a printout to check the reference counter of
a tnl_port structure in tnl-port.c. Extending OVS unit test cases to
have ref_cnt values in the expected dump. Adding new test cases to
check if packet receiving is still working in the case of OF tunnel
port deletion. Adding new test cases to check the reference counter
in case of OF tunnel deletion or reconfiguration.

Signed-off-by: Balazs Nemeth <balazs.nemeth@ericsson.com>
Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
Co-authored-by: Jan Scheurich <jan.scheurich@ericsson.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

3 files changed, 29 insertions, 23 deletions

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b14b339a9..b99f04fad 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -378,6 +378,7 @@ type_run(const char *type)
             HMAP_FOR_EACH (iter, up.hmap_node, &ofproto->up.ports) {
                 char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
                 const char *dp_port;
+                odp_port_t old_odp_port;
 
                 if (!iter->is_tunnel) {
                     continue;
@@ -385,6 +386,7 @@ type_run(const char *type)
 
                 dp_port = netdev_vport_get_dpif_port(iter->up.netdev,
                                                      namebuf, sizeof namebuf);
+                old_odp_port = iter->odp_port;
                 node = simap_find(&tmp_backers, dp_port);
                 if (node) {
                     simap_put(&backer->tnl_backers, dp_port, node->data);
@@ -406,7 +408,7 @@ type_run(const char *type)
 
                 iter->odp_port = node ? u32_to_odp(node->data) : ODPP_NONE;
                 if (tnl_port_reconfigure(iter, iter->up.netdev,
-                                         iter->odp_port,
+                                         iter->odp_port, old_odp_port,
                                          ovs_native_tunneling_is_on(ofproto), dp_port)) {
                     backer->need_revalidate = REV_RECONFIGURE;
                 }
@@ -1950,7 +1952,7 @@ port_destruct(struct ofport *port_, bool del)
        dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
     }
 
-    tnl_port_del(port);
+    tnl_port_del(port, port->odp_port);
     sset_find_and_delete(&ofproto->ports, devname);
     sset_find_and_delete(&ofproto->ghost_ports, devname);
     bundle_remove(port_);
@@ -2006,7 +2008,7 @@ port_modified(struct ofport *port_)
     if (port->is_tunnel) {
         struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
 
-        if (tnl_port_reconfigure(port, netdev, port->odp_port,
+        if (tnl_port_reconfigure(port, netdev, port->odp_port, port->odp_port,
                                  ovs_native_tunneling_is_on(ofproto),
                                  dp_port_name)) {
             ofproto->backer->need_revalidate = REV_RECONFIGURE;
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index c6856a09e..1676f4d46 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -126,7 +126,8 @@ static void tnl_port_mod_log(const struct tnl_port *, const char *action)
     OVS_REQ_RDLOCK(rwlock);
 static const char *tnl_port_get_name(const struct tnl_port *)
     OVS_REQ_RDLOCK(rwlock);
-static void tnl_port_del__(const struct ofport_dpif *) OVS_REQ_WRLOCK(rwlock);
+static void tnl_port_del__(const struct ofport_dpif *, odp_port_t)
+    OVS_REQ_WRLOCK(rwlock);
 
 void
 ofproto_tunnel_init(void)
@@ -221,13 +222,15 @@ tnl_port_add(const struct ofport_dpif *ofport, const struct netdev *netdev,
 }
 
 /* Checks if the tunnel represented by 'ofport' reconfiguration due to changes
- * in its netdev_tunnel_config.  If it does, returns true. Otherwise, returns
- * false.  'ofport' and 'odp_port' should be the same as would be passed to
- * tnl_port_add(). */
+ * in its netdev_tunnel_config. If it does, returns true. Otherwise, returns
+ * false. 'new_odp_port' should be the port number coming from 'ofport' that
+ * is passed to tnl_port_add__(). 'old_odp_port' should be the port number
+ * that is passed to tnl_port_del__(). */
 bool
 tnl_port_reconfigure(const struct ofport_dpif *ofport,
-                     const struct netdev *netdev, odp_port_t odp_port,
-                     bool native_tnl, const char name[])
+                     const struct netdev *netdev, odp_port_t new_odp_port,
+                     odp_port_t old_odp_port, bool native_tnl,
+                     const char name[])
     OVS_EXCLUDED(rwlock)
 {
     struct tnl_port *tnl_port;
@@ -236,14 +239,14 @@ tnl_port_reconfigure(const struct ofport_dpif *ofport,
     fat_rwlock_wrlock(&rwlock);
     tnl_port = tnl_find_ofport(ofport);
     if (!tnl_port) {
-        changed = tnl_port_add__(ofport, netdev, odp_port, false, native_tnl,
-                                 name);
+        changed = tnl_port_add__(ofport, netdev, new_odp_port, false,
+                                 native_tnl, name);
     } else if (tnl_port->netdev != netdev
-               || tnl_port->match.odp_port != odp_port
+               || tnl_port->match.odp_port != new_odp_port
                || tnl_port->change_seq != netdev_get_change_seq(tnl_port->netdev)) {
         VLOG_DBG("reconfiguring %s", tnl_port_get_name(tnl_port));
-        tnl_port_del__(ofport);
-        tnl_port_add__(ofport, netdev, odp_port, true, native_tnl, name);
+        tnl_port_del__(ofport, old_odp_port);
+        tnl_port_add__(ofport, netdev, new_odp_port, true, native_tnl, name);
         changed = true;
     }
     fat_rwlock_unlock(&rwlock);
@@ -251,7 +254,8 @@ tnl_port_reconfigure(const struct ofport_dpif *ofport,
 }
 
 static void
-tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
+tnl_port_del__(const struct ofport_dpif *ofport, odp_port_t odp_port)
+    OVS_REQ_WRLOCK(rwlock)
 {
     struct tnl_port *tnl_port;
 
@@ -261,11 +265,9 @@ tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
 
     tnl_port = tnl_find_ofport(ofport);
     if (tnl_port) {
-        const struct netdev_tunnel_config *cfg =
-            netdev_get_tunnel_config(tnl_port->netdev);
         struct hmap **map;
 
-        tnl_port_map_delete(cfg->dst_port, netdev_get_type(tnl_port->netdev));
+        tnl_port_map_delete(odp_port, netdev_get_type(tnl_port->netdev));
         tnl_port_mod_log(tnl_port, "removing");
         map = tnl_match_map(&tnl_port->match);
         hmap_remove(*map, &tnl_port->match_node);
@@ -282,10 +284,11 @@ tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
 
 /* Removes 'ofport' from the module. */
 void
-tnl_port_del(const struct ofport_dpif *ofport) OVS_EXCLUDED(rwlock)
+tnl_port_del(const struct ofport_dpif *ofport, odp_port_t odp_port)
+    OVS_EXCLUDED(rwlock)
 {
     fat_rwlock_wrlock(&rwlock);
-    tnl_port_del__(ofport);
+    tnl_port_del__(ofport, odp_port);
     fat_rwlock_unlock(&rwlock);
 }
 
diff --git a/ofproto/tunnel.h b/ofproto/tunnel.h
index b0ec67c2b..47d3dd56d 100644
--- a/ofproto/tunnel.h
+++ b/ofproto/tunnel.h
@@ -32,11 +32,12 @@ struct netdev_tnl_build_header_params;
 
 void ofproto_tunnel_init(void);
 bool tnl_port_reconfigure(const struct ofport_dpif *, const struct netdev *,
-                          odp_port_t, bool native_tnl, const char name[]);
+                          odp_port_t new_odp_port, odp_port_t old_odp_port,
+                          bool native_tnl, const char name[]);
 
 int tnl_port_add(const struct ofport_dpif *, const struct netdev *,
-                 odp_port_t odp_port, bool native_tnl, const char name[]);
-void tnl_port_del(const struct ofport_dpif *);
+                 odp_port_t, bool native_tnl, const char name[]);
+void tnl_port_del(const struct ofport_dpif *, odp_port_t);
 
 const struct ofport_dpif *tnl_port_receive(const struct flow *);
 void tnl_wc_init(struct flow *, struct flow_wildcards *);