[PATCH net v2 1/2] openvswitch: vport: fix self-deadlock on release of tunnel ports

[PATCH net v2 1/2] openvswitch: vport: fix self-deadlock on release of tunnel ports

[Ilya Maximets]

vports are used concurrently and protected by RCU, so netdev_put()
must happen after the RCU grace period.  So, either in an RCU call or
after the synchronize_net().  The rtnl_delete_link() must happen under
RTNL and so can't be executed in RCU context.  Calling synchronize_net()
while holding RTNL is not a good idea for performance and system
stability under load in general, so calling netdev_put() in RCU call
is the right solution here.

However,
when the device is deleted, rtnl_unlock() will call netdev_run_todo()
and block until all the references are gone.  In the current code this
means that we never reach the call_rcu() and the vport is never freed
and the reference is never released, causing a self-deadlock on device
removal.

Fix that by moving the rcu_call() before the rtnl_unlock(), so the
scheduled RCU callback will be executed when synchronize_net() is
called from the rtnl_unlock()->netdev_run_todo() while the RTNL itself
is already released.

Fixes: 6931d21f87bc ("openvswitch: defer tunnel netdev_put to RCU release")
Cc: stable@vger.kernel.org
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/vport-netdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index 12055af832dc0..a1df551e915bc 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -196,9 +196,13 @@ void ovs_netdev_tunnel_destroy(struct vport *vport)
 	 */
 	if (vport->dev->reg_state == NETREG_REGISTERED)
 		rtnl_delete_link(vport->dev, 0, NULL);
-	rtnl_unlock();
 
+	/* We can't put the device reference yet, since it can still be in
+	 * use, but rtnl_unlock()->netdev_run_todo() will block until all
+	 * the references are released, so the RCU call must be before it.
+	 */
 	call_rcu(&vport->rcu, vport_netdev_free);
+	rtnl_unlock();
 }
 EXPORT_SYMBOL_GPL(ovs_netdev_tunnel_destroy);
 
-- 
2.53.0

Re: [PATCH net v2 1/2] openvswitch: vport: fix self-deadlock on release of tunnel ports

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> vports are used concurrently and protected by RCU, so netdev_put()
> must happen after the RCU grace period.  So, either in an RCU call or
> after the synchronize_net().  The rtnl_delete_link() must happen under
> RTNL and so can't be executed in RCU context.  Calling synchronize_net()
> while holding RTNL is not a good idea for performance and system
> stability under load in general, so calling netdev_put() in RCU call
> is the right solution here.
>
> However,
> when the device is deleted, rtnl_unlock() will call netdev_run_todo()
> and block until all the references are gone.  In the current code this
> means that we never reach the call_rcu() and the vport is never freed
> and the reference is never released, causing a self-deadlock on device
> removal.
>
> Fix that by moving the rcu_call() before the rtnl_unlock(), so the
> scheduled RCU callback will be executed when synchronize_net() is
> called from the rtnl_unlock()->netdev_run_todo() while the RTNL itself
> is already released.
>
> Fixes: 6931d21f87bc ("openvswitch: defer tunnel netdev_put to RCU release")
> Cc: stable@vger.kernel.org
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>