bridge: Clean leaking netdevs when route is added.

When adding a route to a bridge, by executing "$appctl ovs/route/add
$IP/$MASK $BR", a reference to the existing netdev is taken and stored
in an instantiated ip_dev struct which is then stored in an addr_list
list in tnl-ports.c. When OvS is signaled to exit, as a result of a
"$appctl $OVS_PID exit --cleanup", for example, the bridge takes care of
destroying its allocated port and iface structs. While destroying and
freeing an iface, the netdev associated with it is also destroyed.
However, for this to happen its ref_cnt must be 0.  Otherwise the
destructor of the netdev (specific to each datapath) won't be called. On
the userspace datapath this means a system interface, such as "br0",
wouldn't get deleted upon exit of OvS (when a route happens to be
assocaited).

This was first observed in the "ptap - triangle bridge setup with L2 and
L3 GRE tunnels" test, which runs as part of the system userspace
testsuite and uses the netdev datapath (as opoosed to several tests
which use the dummy datapath, where this issue isn't seen). The test
would pass every other time and fail the rest of the times because the
needed system interfaces (br-p1, br-p2 and br-p3) were already present
(from the previous successfull run which didn't clean up properly),
leading to a failure.

To fix the leak and clean up the interfaces upon exit, on its final
stage before destroying a netdev, in iface_destroy__(), the bridge calls
tnl_port_map_delete_ipdev() which takes care of freeing the instatiated
ip_dev structs that refer to a specific netdev.

An extra test is also introduced which verifies that the resources used
by OvS netdev datapath have been correctly cleaned up between
OVS_TRAFFIC_VSWITCHD_STOP and AT_CLEANUP.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

diff --git a/tests/system-interface.at b/tests/system-interface.at
index db790d59e2b3bce975a92410886ee4811948c1e0..784bada12cbb38cfba47a465bc95c88fe9cc04e4 100644 (file)
--- a/tests/system-interface.at
+++ b/tests/system-interface.at
@@ -25,3 +25,41 @@ OVS_TRAFFIC_VSWITCHD_STOP(["dnl
 
 AT_CLEANUP
 
+dnl add a p1-0 interface to br-p1, then add a route to br-p1 and stop the OvS
+dnl instance. Confirm br-p1 interface has been deleted from the system.
+AT_SETUP([interface - add route to br and verify clean-up])
+
+OVS_TRAFFIC_VSWITCHD_START()
+
+HWADDR_BRP1=aa:55:00:00:00:01
+
+dnl Create tap port to later add to br-p1
+AT_CHECK([ip tuntap add name p1-0 mode tap])
+AT_CHECK([ip link set p1-0 up])
+on_exit 'ip link del p1-0'
+
+AT_CHECK([
+    ovs-vsctl add-br br-p1 -- \
+        set bridge br-p1 datapath_type=netdev fail-mode=standalone other-config:hwaddr=$HWADDR_BRP1
+
+    ovs-vsctl add-port br-p1 p1-0
+
+    ovs-ofctl del-flows br-p1
+], [0])
+
+AT_CHECK([
+    ip addr add 10.0.0.1/24 dev br-p1
+    ip link set br-p1 up
+], [0], [stdout])
+
+AT_CHECK([
+    ovs-appctl ovs/route/add 10.0.0.0/24 br-p1
+    ovs-appctl tnl/arp/set br-p1 10.0.0.1 $HWADDR_BRP1
+], [0], [stdout])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CHECK([
+    ip link show br-p1], [1],
+    [stdout], [Device "br-p1" does not exist.]
+)
+AT_CLEANUP

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index d383b287e734643daff677bd4b0604e01a0d950d..706a07cc6fb9eef98c16f78c1d57befb13ed54c7 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -62,6 +62,7 @@
 #include "sset.h"
 #include "system-stats.h"
 #include "timeval.h"
+#include "tnl-ports.h"
 #include "util.h"
 #include "unixctl.h"
 #include "lib/vswitch-idl.h"
@@ -4348,6 +4349,8 @@ iface_destroy__(struct iface *iface)
         ovs_list_remove(&iface->port_elem);
         hmap_remove(&br->iface_by_name, &iface->name_node);
 
+        tnl_port_map_delete_ipdev(netdev_get_name(iface->netdev));
+
         /* The user is changing configuration here, so netdev_remove needs to be
          * used as opposed to netdev_close */
         netdev_remove(iface->netdev);