| Message ID | 1386926487-30822-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Friday, December 13, 2013 4:21 AM > To: KY Srinivasan; Haiyang Zhang; devel@linuxdriverproject.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Jason Wang > Subject: [PATCH net] netvsc: don't flush peers notifying work during setting > mtu > > There's a possible deadlock if we flush the peers notifying work during > setting > mtu: > > [ 22.991149] > ====================================================== > [ 22.991173] [ INFO: possible circular locking dependency detected ] > [ 22.991198] 3.10.0-54.0.1.el7.x86_64.debug #1 Not tainted > [ 22.991219] ------------------------------------------------------- > [ 22.991243] ip/974 is trying to acquire lock: > [ 22.991261] ((&(&net_device_ctx->dwork)->work)){+.+.+.}, at: > [<ffffffff8108af95>] flush_work+0x5/0x2e0 > [ 22.991307] > but task is already holding lock: > [ 22.991330] (rtnl_mutex){+.+.+.}, at: [<ffffffff81539deb>] > rtnetlink_rcv+0x1b/0x40 > [ 22.991367] > which lock already depends on the new lock. > > [ 22.991398] > the existing dependency chain (in reverse order) is: > [ 22.991426] > -> #1 (rtnl_mutex){+.+.+.}: > [ 22.991449] [<ffffffff810dfdd9>] __lock_acquire+0xb19/0x1260 > [ 22.991477] [<ffffffff810e0d12>] lock_acquire+0xa2/0x1f0 > [ 22.991501] [<ffffffff81673659>] mutex_lock_nested+0x89/0x4f0 > [ 22.991529] [<ffffffff815392b7>] rtnl_lock+0x17/0x20 > [ 22.991552] [<ffffffff815230b2>] netdev_notify_peers+0x12/0x30 > [ 22.991579] [<ffffffffa0340212>] netvsc_send_garp+0x22/0x30 > [hv_netvsc] > [ 22.991610] [<ffffffff8108d251>] process_one_work+0x211/0x6e0 > [ 22.991637] [<ffffffff8108d83b>] worker_thread+0x11b/0x3a0 > [ 22.991663] [<ffffffff81095e5d>] kthread+0xed/0x100 > [ 22.991686] [<ffffffff81681c6c>] ret_from_fork+0x7c/0xb0 > [ 22.991715] > -> #0 ((&(&net_device_ctx->dwork)->work)){+.+.+.}: > [ 22.991715] [<ffffffff810de817>] check_prevs_add+0x967/0x970 > [ 22.991715] [<ffffffff810dfdd9>] __lock_acquire+0xb19/0x1260 > [ 22.991715] [<ffffffff810e0d12>] lock_acquire+0xa2/0x1f0 > [ 22.991715] [<ffffffff8108afde>] flush_work+0x4e/0x2e0 > [ 22.991715] [<ffffffff8108e1b5>] __cancel_work_timer+0x95/0x130 > [ 22.991715] [<ffffffff8108e303>] cancel_delayed_work_sync+0x13/0x20 > [ 22.991715] [<ffffffffa03404e4>] netvsc_change_mtu+0x84/0x200 > [hv_netvsc] > [ 22.991715] [<ffffffff815233d4>] dev_set_mtu+0x34/0x80 > [ 22.991715] [<ffffffff8153bc2a>] do_setlink+0x23a/0xa00 > [ 22.991715] [<ffffffff8153d054>] rtnl_newlink+0x394/0x5e0 > [ 22.991715] [<ffffffff81539eac>] rtnetlink_rcv_msg+0x9c/0x260 > [ 22.991715] [<ffffffff8155cdd9>] netlink_rcv_skb+0xa9/0xc0 > [ 22.991715] [<ffffffff81539dfa>] rtnetlink_rcv+0x2a/0x40 > [ 22.991715] [<ffffffff8155c41d>] netlink_unicast+0xdd/0x190 > [ 22.991715] [<ffffffff8155c807>] netlink_sendmsg+0x337/0x750 > [ 22.991715] [<ffffffff8150d219>] sock_sendmsg+0x99/0xd0 > [ 22.991715] [<ffffffff8150d63e>] ___sys_sendmsg+0x39e/0x3b0 > [ 22.991715] [<ffffffff8150eba2>] __sys_sendmsg+0x42/0x80 > [ 22.991715] [<ffffffff8150ebf2>] SyS_sendmsg+0x12/0x20 > [ 22.991715] [<ffffffff81681d19>] system_call_fastpath+0x16/0x1b > > This is because we hold the rtnl_lock() before ndo_change_mtu() and try to > flush the work in netvsc_change_mtu(), in the mean time, > netdev_notify_peers() may be called from worker and also trying to hold the > rtnl_lock. This will lead the flush won't succeed forever. Solve this by not > canceling and flushing the work, this is safe because the transmission done > by NETDEV_NOTIFY_PEERS was synchronized with the netif_tx_disable() > called by netvsc_change_mtu(). > > Reported-by: Yaju Cao <yacao@redhat.com> > Tested-by: Yaju Cao <yacao@redhat.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> Acked-by: Haiyang Zhang <haiyangz@microsoft.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jason Wang <jasowang@redhat.com> Date: Fri, 13 Dec 2013 17:21:27 +0800 > There's a possible deadlock if we flush the peers notifying work during setting > mtu: ... > Reported-by: Yaju Cao <yacao@redhat.com> > Tested-by: Yaju Cao <yacao@redhat.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > The patch is needed for stable. Applied and queued up for -stable, thanks Jason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 524f713..f813572 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -327,7 +327,6 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) return -EINVAL; nvdev->start_remove = true; - cancel_delayed_work_sync(&ndevctx->dwork); cancel_work_sync(&ndevctx->work); netif_tx_disable(ndev); rndis_filter_device_remove(hdev);
There's a possible deadlock if we flush the peers notifying work during setting mtu: [ 22.991149] ====================================================== [ 22.991173] [ INFO: possible circular locking dependency detected ] [ 22.991198] 3.10.0-54.0.1.el7.x86_64.debug #1 Not tainted [ 22.991219] ------------------------------------------------------- [ 22.991243] ip/974 is trying to acquire lock: [ 22.991261] ((&(&net_device_ctx->dwork)->work)){+.+.+.}, at: [<ffffffff8108af95>] flush_work+0x5/0x2e0 [ 22.991307] but task is already holding lock: [ 22.991330] (rtnl_mutex){+.+.+.}, at: [<ffffffff81539deb>] rtnetlink_rcv+0x1b/0x40 [ 22.991367] which lock already depends on the new lock. [ 22.991398] the existing dependency chain (in reverse order) is: [ 22.991426] -> #1 (rtnl_mutex){+.+.+.}: [ 22.991449] [<ffffffff810dfdd9>] __lock_acquire+0xb19/0x1260 [ 22.991477] [<ffffffff810e0d12>] lock_acquire+0xa2/0x1f0 [ 22.991501] [<ffffffff81673659>] mutex_lock_nested+0x89/0x4f0 [ 22.991529] [<ffffffff815392b7>] rtnl_lock+0x17/0x20 [ 22.991552] [<ffffffff815230b2>] netdev_notify_peers+0x12/0x30 [ 22.991579] [<ffffffffa0340212>] netvsc_send_garp+0x22/0x30 [hv_netvsc] [ 22.991610] [<ffffffff8108d251>] process_one_work+0x211/0x6e0 [ 22.991637] [<ffffffff8108d83b>] worker_thread+0x11b/0x3a0 [ 22.991663] [<ffffffff81095e5d>] kthread+0xed/0x100 [ 22.991686] [<ffffffff81681c6c>] ret_from_fork+0x7c/0xb0 [ 22.991715] -> #0 ((&(&net_device_ctx->dwork)->work)){+.+.+.}: [ 22.991715] [<ffffffff810de817>] check_prevs_add+0x967/0x970 [ 22.991715] [<ffffffff810dfdd9>] __lock_acquire+0xb19/0x1260 [ 22.991715] [<ffffffff810e0d12>] lock_acquire+0xa2/0x1f0 [ 22.991715] [<ffffffff8108afde>] flush_work+0x4e/0x2e0 [ 22.991715] [<ffffffff8108e1b5>] __cancel_work_timer+0x95/0x130 [ 22.991715] [<ffffffff8108e303>] cancel_delayed_work_sync+0x13/0x20 [ 22.991715] [<ffffffffa03404e4>] netvsc_change_mtu+0x84/0x200 [hv_netvsc] [ 22.991715] [<ffffffff815233d4>] dev_set_mtu+0x34/0x80 [ 22.991715] [<ffffffff8153bc2a>] do_setlink+0x23a/0xa00 [ 22.991715] [<ffffffff8153d054>] rtnl_newlink+0x394/0x5e0 [ 22.991715] [<ffffffff81539eac>] rtnetlink_rcv_msg+0x9c/0x260 [ 22.991715] [<ffffffff8155cdd9>] netlink_rcv_skb+0xa9/0xc0 [ 22.991715] [<ffffffff81539dfa>] rtnetlink_rcv+0x2a/0x40 [ 22.991715] [<ffffffff8155c41d>] netlink_unicast+0xdd/0x190 [ 22.991715] [<ffffffff8155c807>] netlink_sendmsg+0x337/0x750 [ 22.991715] [<ffffffff8150d219>] sock_sendmsg+0x99/0xd0 [ 22.991715] [<ffffffff8150d63e>] ___sys_sendmsg+0x39e/0x3b0 [ 22.991715] [<ffffffff8150eba2>] __sys_sendmsg+0x42/0x80 [ 22.991715] [<ffffffff8150ebf2>] SyS_sendmsg+0x12/0x20 [ 22.991715] [<ffffffff81681d19>] system_call_fastpath+0x16/0x1b This is because we hold the rtnl_lock() before ndo_change_mtu() and try to flush the work in netvsc_change_mtu(), in the mean time, netdev_notify_peers() may be called from worker and also trying to hold the rtnl_lock. This will lead the flush won't succeed forever. Solve this by not canceling and flushing the work, this is safe because the transmission done by NETDEV_NOTIFY_PEERS was synchronized with the netif_tx_disable() called by netvsc_change_mtu(). Reported-by: Yaju Cao <yacao@redhat.com> Tested-by: Yaju Cao <yacao@redhat.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- The patch is needed for stable. --- drivers/net/hyperv/netvsc_drv.c | 1 - 1 file changed, 1 deletion(-)