| Message ID | 1529602756-239046-1-git-send-email-tiago.lam@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev] bridge: Clean leaking netdevs when route is added. | expand |
On Thu, Jun 21, 2018 at 06:39:16PM +0100, Tiago Lam wrote: > 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> I don't fully understand this one, but the commit message is so detailed and the patch is so short, so I applied it to master. If it needs backporting, please let me know.
diff --git a/tests/system-interface.at b/tests/system-interface.at index db790d5..784bada 100644 --- 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 d383b28..706a07c 100644 --- 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);
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> --- tests/system-interface.at | 38 ++++++++++++++++++++++++++++++++++++++ vswitchd/bridge.c | 3 +++ 2 files changed, 41 insertions(+)