| Message ID | 1698355354-12869-1-git-send-email-haiyangz@microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] hv_netvsc: fix race of netvsc and VF register_netdevice | expand |
On 26.10.2023 23:22, Haiyang Zhang wrote: > The rtnl lock also needs to be held before rndis_filter_device_add() > which advertises nvsp_2_vsc_capability / sriov bit, and triggers > VF NIC offering and registering. If VF NIC finished register_netdev() > earlier it may cause name based config failure. > > To fix this issue, move the call to rtnl_lock() before > rndis_filter_device_add(), so VF will be registered later than netvsc > / synthetic NIC, and gets a name numbered (ethX) after netvsc. > > And, move register_netdevice_notifier() earlier, so the call back > function is set before probing. > > Cc: stable@vger.kernel.org > Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in netvsc_probe()") > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > --- > v2: > Fix rtnl_unlock() in error handling as found by Wojciech Drewek. > --- > drivers/net/hyperv/netvsc_drv.c | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 3ba3c8fb28a5..1d1491da303b 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -2531,15 +2531,6 @@ static int netvsc_probe(struct hv_device *dev, > goto devinfo_failed; > } > > - nvdev = rndis_filter_device_add(dev, device_info); > - if (IS_ERR(nvdev)) { > - ret = PTR_ERR(nvdev); > - netdev_err(net, "unable to add netvsc device (ret %d)\n", ret); > - goto rndis_failed; > - } > - > - eth_hw_addr_set(net, device_info->mac_adr); > - > /* We must get rtnl lock before scheduling nvdev->subchan_work, > * otherwise netvsc_subchan_work() can get rtnl lock first and wait > * all subchannels to show up, but that may not happen because > @@ -2547,9 +2538,23 @@ static int netvsc_probe(struct hv_device *dev, > * -> ... -> device_add() -> ... -> __device_attach() can't get > * the device lock, so all the subchannels can't be processed -- > * finally netvsc_subchan_work() hangs forever. > + * > + * The rtnl lock also needs to be held before rndis_filter_device_add() > + * which advertises nvsp_2_vsc_capability / sriov bit, and triggers > + * VF NIC offering and registering. If VF NIC finished register_netdev() > + * earlier it may cause name based config failure. > */ > rtnl_lock(); > > + nvdev = rndis_filter_device_add(dev, device_info); > + if (IS_ERR(nvdev)) { > + ret = PTR_ERR(nvdev); > + netdev_err(net, "unable to add netvsc device (ret %d)\n", ret); > + goto rndis_failed; > + } > + > + eth_hw_addr_set(net, device_info->mac_adr); > + > if (nvdev->num_chn > 1) > schedule_work(&nvdev->subchan_work); > > @@ -2586,9 +2591,9 @@ static int netvsc_probe(struct hv_device *dev, > return 0; > > register_failed: > - rtnl_unlock(); > rndis_filter_device_remove(dev, nvdev); > rndis_failed: > + rtnl_unlock(); > netvsc_devinfo_put(device_info); > devinfo_failed: > free_percpu(net_device_ctx->vf_stats); > @@ -2788,11 +2793,14 @@ static int __init netvsc_drv_init(void) > } > netvsc_ring_bytes = ring_size * PAGE_SIZE; > > + register_netdevice_notifier(&netvsc_netdev_notifier); > + > ret = vmbus_driver_register(&netvsc_drv); > - if (ret) > + if (ret) { > + unregister_netdevice_notifier(&netvsc_netdev_notifier); > return ret; > + } > > - register_netdevice_notifier(&netvsc_netdev_notifier); > return 0; > } >
On Thu, 26 Oct 2023 14:22:34 -0700 Haiyang Zhang wrote: > And, move register_netdevice_notifier() earlier, so the call back > function is set before probing. Are you sure you need this? I thought the netdev notifier "replays" registration events (i.e. sends "fake" events for already present netdevs). If I'm wrong this should still be a separate patch from the rtnl reorder.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, November 2, 2023 1:08 AM > To: Haiyang Zhang <haiyangz@microsoft.com> > Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; KY Srinivasan > <kys@microsoft.com>; wei.liu@kernel.org; Dexuan Cui > <decui@microsoft.com>; edumazet@google.com; pabeni@redhat.com; > davem@davemloft.net; linux-kernel@vger.kernel.org; stable@vger.kernel.org > Subject: Re: [PATCH net,v2] hv_netvsc: fix race of netvsc and VF > register_netdevice > > On Thu, 26 Oct 2023 14:22:34 -0700 Haiyang Zhang wrote: > > And, move register_netdevice_notifier() earlier, so the call back > > function is set before probing. > > Are you sure you need this? I thought the netdev notifier "replays" > registration events (i.e. sends "fake" events for already present > netdevs). > > If I'm wrong this should still be a separate patch from the rtnl > reorder. I tested, NETDEV_REGISTER is indeed replayed, but NETDEV_POST_INIT is not. And we will use NETDEV_POST_INIT soon. Also, we want to get notified by NETDEV_POST_INIT immediately from VF, before VF NIC shows up to upper layers. So, even if we make NETDEV_POST_INIT to be replayed, that may be too late. I will put the register_netdevice_notifier() change to a separate patch. Thanks, - Haiyang
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 3ba3c8fb28a5..1d1491da303b 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -2531,15 +2531,6 @@ static int netvsc_probe(struct hv_device *dev, goto devinfo_failed; } - nvdev = rndis_filter_device_add(dev, device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); - netdev_err(net, "unable to add netvsc device (ret %d)\n", ret); - goto rndis_failed; - } - - eth_hw_addr_set(net, device_info->mac_adr); - /* We must get rtnl lock before scheduling nvdev->subchan_work, * otherwise netvsc_subchan_work() can get rtnl lock first and wait * all subchannels to show up, but that may not happen because @@ -2547,9 +2538,23 @@ static int netvsc_probe(struct hv_device *dev, * -> ... -> device_add() -> ... -> __device_attach() can't get * the device lock, so all the subchannels can't be processed -- * finally netvsc_subchan_work() hangs forever. + * + * The rtnl lock also needs to be held before rndis_filter_device_add() + * which advertises nvsp_2_vsc_capability / sriov bit, and triggers + * VF NIC offering and registering. If VF NIC finished register_netdev() + * earlier it may cause name based config failure. */ rtnl_lock(); + nvdev = rndis_filter_device_add(dev, device_info); + if (IS_ERR(nvdev)) { + ret = PTR_ERR(nvdev); + netdev_err(net, "unable to add netvsc device (ret %d)\n", ret); + goto rndis_failed; + } + + eth_hw_addr_set(net, device_info->mac_adr); + if (nvdev->num_chn > 1) schedule_work(&nvdev->subchan_work); @@ -2586,9 +2591,9 @@ static int netvsc_probe(struct hv_device *dev, return 0; register_failed: - rtnl_unlock(); rndis_filter_device_remove(dev, nvdev); rndis_failed: + rtnl_unlock(); netvsc_devinfo_put(device_info); devinfo_failed: free_percpu(net_device_ctx->vf_stats); @@ -2788,11 +2793,14 @@ static int __init netvsc_drv_init(void) } netvsc_ring_bytes = ring_size * PAGE_SIZE; + register_netdevice_notifier(&netvsc_netdev_notifier); + ret = vmbus_driver_register(&netvsc_drv); - if (ret) + if (ret) { + unregister_netdevice_notifier(&netvsc_netdev_notifier); return ret; + } - register_netdevice_notifier(&netvsc_netdev_notifier); return 0; }
The rtnl lock also needs to be held before rndis_filter_device_add() which advertises nvsp_2_vsc_capability / sriov bit, and triggers VF NIC offering and registering. If VF NIC finished register_netdev() earlier it may cause name based config failure. To fix this issue, move the call to rtnl_lock() before rndis_filter_device_add(), so VF will be registered later than netvsc / synthetic NIC, and gets a name numbered (ethX) after netvsc. And, move register_netdevice_notifier() earlier, so the call back function is set before probing. Cc: stable@vger.kernel.org Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in netvsc_probe()") Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> --- v2: Fix rtnl_unlock() in error handling as found by Wojciech Drewek. --- drivers/net/hyperv/netvsc_drv.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)