Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460401 > unrolled thread

[PATCH net 0/4] hv_netvsc: fixes for VF removal path

Back to article view | Back to linux.kernel

Contents

  [PATCH net 0/4] hv_netvsc: fixes for VF removal path Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-08-11 13:00 +0200
    [PATCH net 4/4] hv_netvsc: avoid deadlocks between rtnl lock and netvsc_inject_disable() Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-08-11 13:10 +0200
    [PATCH net 3/4] hv_netvsc: protect module refcount by checking net_device_ctx->vf_netdev Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-08-11 13:10 +0200
      RE: [PATCH net 3/4] hv_netvsc: protect module refcount by checking  net_device_ctx->vf_netdev Haiyang Zhang <haiyangz@microsoft.com> - 2016-08-11 18:20 +0200

#1460401 — [PATCH net 0/4] hv_netvsc: fixes for VF removal path

Kernel crash is reported after VF is removed and detached from netvsc
device. My investigation led me to PATCH2 of this series but PATCH1 is
required to support the change. I also noticed a couple of other issues
while debugging and I fix them with PATCH3 and PATCH4.

Please review.

Vitaly Kuznetsov (4):
  hv_netvsc: don't lose VF information
  hv_netvsc: reset vf_inject on VF removal
  hv_netvsc: protect module refcount by checking
    net_device_ctx->vf_netdev
  hv_netvsc: avoid deadlocks between rtnl lock and
    netvsc_inject_disable()

 drivers/net/hyperv/hyperv_net.h | 24 ++++-------
 drivers/net/hyperv/netvsc.c     | 19 ++++----
 drivers/net/hyperv/netvsc_drv.c | 96 ++++++++++++++++++-----------------------
 3 files changed, 59 insertions(+), 80 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1460405 — [PATCH net 4/4] hv_netvsc: avoid deadlocks between rtnl lock and netvsc_inject_disable()

Here is a deadlock scenario:
- netvsc_vf_up() schedules netvsc_notify_peers() work and quits.
- netvsc_vf_down() runs before netvsc_notify_peers() gets executed. As it
  is being executed from netdev notifier chain we hold rtnl lock when we
  get here.
- we enter netvsc_inject_disable() and loop and wait till
  netvsc_notify_peers() drops vf_use_cnt.
- netvsc_notify_peers() starts on some other CPU but netdev_notify_peers()
  will hang on rtnl_lock().
- deadlock!

Similar deadlocks are possible between netvsc_vf_{up,down}() and
netvsc_unregister_vf() as it also waits till vf_use_cnt drops to zero.
Instead of introducing additional synchronization I suggest we drop
gwrk.dwrk completely and call NETDEV_NOTIFY_PEERS directly. As we're
acting under rtnl lock this is legitimate.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/hyperv_net.h |  7 -------
 drivers/net/hyperv/netvsc_drv.c | 33 +++++----------------------------
 2 files changed, 5 insertions(+), 35 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 3b3ecf2..591af71 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -644,12 +644,6 @@ struct netvsc_reconfig {
 	u32 event;
 };
 
-struct garp_wrk {
-	struct work_struct dwrk;
-	struct net_device *netdev;
-	struct net_device_context *net_device_ctx;
-};
-
 /* The context of the netvsc device  */
 struct net_device_context {
 	/* point back to our device context */
@@ -667,7 +661,6 @@ struct net_device_context {
 
 	struct work_struct work;
 	u32 msg_enable; /* debug level */
-	struct garp_wrk gwrk;
 
 	struct netvsc_stats __percpu *tx_stats;
 	struct netvsc_stats __percpu *rx_stats;
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 874829a..62a4e6e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1151,17 +1151,6 @@ static void netvsc_free_netdev(struct net_device *netdev)
 	free_netdev(netdev);
 }
 
-static void netvsc_notify_peers(struct work_struct *wrk)
-{
-	struct garp_wrk *gwrk;
-
-	gwrk = container_of(wrk, struct garp_wrk, dwrk);
-
-	netdev_notify_peers(gwrk->netdev);
-
-	atomic_dec(&gwrk->net_device_ctx->vf_use_cnt);
-}
-
 static struct net_device *get_netvsc_net_device(char *mac)
 {
 	struct net_device *dev, *found = NULL;
@@ -1266,15 +1255,8 @@ static int netvsc_vf_up(struct net_device *vf_netdev)
 
 	netif_carrier_off(ndev);
 
-	/*
-	 * Now notify peers. We are scheduling work to
-	 * notify peers; take a reference to prevent
-	 * the VF interface from vanishing.
-	 */
-	atomic_inc(&net_device_ctx->vf_use_cnt);
-	net_device_ctx->gwrk.netdev = vf_netdev;
-	net_device_ctx->gwrk.net_device_ctx = net_device_ctx;
-	schedule_work(&net_device_ctx->gwrk.dwrk);
+	/* Now notify peers through VF device. */
+	call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, vf_netdev);
 
 	return NOTIFY_OK;
 }
@@ -1306,13 +1288,9 @@ static int netvsc_vf_down(struct net_device *vf_netdev)
 	netdev_info(ndev, "Data path switched from VF: %s\n", vf_netdev->name);
 	rndis_filter_close(netvsc_dev);
 	netif_carrier_on(ndev);
-	/*
-	 * Notify peers.
-	 */
-	atomic_inc(&net_device_ctx->vf_use_cnt);
-	net_device_ctx->gwrk.netdev = ndev;
-	net_device_ctx->gwrk.net_device_ctx = net_device_ctx;
-	schedule_work(&net_device_ctx->gwrk.dwrk);
+
+	/* Now notify peers through netvsc device. */
+	call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, ndev);
 
 	return NOTIFY_OK;
 }
@@ -1384,7 +1362,6 @@ static int netvsc_probe(struct hv_device *dev,
 
 	INIT_DELAYED_WORK(&net_device_ctx->dwork, netvsc_link_change);
 	INIT_WORK(&net_device_ctx->work, do_set_multicast);
-	INIT_WORK(&net_device_ctx->gwrk.dwrk, netvsc_notify_peers);
 
 	spin_lock_init(&net_device_ctx->lock);
 	INIT_LIST_HEAD(&net_device_ctx->reconfig_events);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1460406 — [PATCH net 3/4] hv_netvsc: protect module refcount by checking net_device_ctx->vf_netdev

We're not guaranteed to see NETDEV_REGISTER/NETDEV_UNREGISTER notifications
only once per VF but we increase/decrease module refcount unconditionally.
Check vf_netdev to make sure we don't take/release it twice. We presume
that only one VF per netvsc device may exist.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/netvsc_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index b3c31e3..874829a 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1204,7 +1204,7 @@ static int netvsc_register_vf(struct net_device *vf_netdev)
 
 	net_device_ctx = netdev_priv(ndev);
 	netvsc_dev = net_device_ctx->nvdev;
-	if (netvsc_dev == NULL)
+	if (!netvsc_dev || net_device_ctx->vf_netdev)
 		return NOTIFY_DONE;
 
 	netdev_info(ndev, "VF registering: %s\n", vf_netdev->name);
@@ -1334,7 +1334,7 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
 
 	net_device_ctx = netdev_priv(ndev);
 	netvsc_dev = net_device_ctx->nvdev;
-	if (netvsc_dev == NULL)
+	if (!netvsc_dev || !net_device_ctx->vf_netdev)
 		return NOTIFY_DONE;
 	netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name);
 	netvsc_inject_disable(net_device_ctx);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1460648 — RE: [PATCH net 3/4] hv_netvsc: protect module refcount by checking net_device_ctx->vf_netdev

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Thursday, August 11, 2016 6:59 AM
> To: netdev@vger.kernel.org
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Haiyang
> Zhang <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>
> Subject: [PATCH net 3/4] hv_netvsc: protect module refcount by checking
> net_device_ctx->vf_netdev
> 
> We're not guaranteed to see NETDEV_REGISTER/NETDEV_UNREGISTER
> notifications
> only once per VF but we increase/decrease module refcount unconditionally.
> Check vf_netdev to make sure we don't take/release it twice. We presume
> that only one VF per netvsc device may exist.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Haiyang Zhang <haiyangz@microsoft.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web