| Message ID | 1706609772-5783-1-git-send-email-schakrabarti@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] hv_netvsc: Fix race condition between netvsc_probe and netvsc_remove | expand |
> -----Original Message----- > From: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> > Sent: Tuesday, January 30, 2024 5:16 AM > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui > <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; Long Li <longli@microsoft.com>; > yury.norov@gmail.com; leon@kernel.org; cai.huoqing@linux.dev; > ssengar@linux.microsoft.com; vkuznets@redhat.com; tglx@linutronix.de; > linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-rdma@vger.kernel.org > Cc: Souradeep Chakrabarti <schakrabarti@microsoft.com>; Souradeep > Chakrabarti <schakrabarti@linux.microsoft.com> > Subject: [PATCH net] hv_netvsc: Fix race condition between netvsc_probe > and netvsc_remove > > In commit ac5047671758 ("hv_netvsc: Disable NAPI before closing the > VMBus channel"), napi_disable was getting called for all channels, > including all subchannels without confirming if they are enabled or not. > > Which caused hv_netvsc getting hung at napi_disable, when netvsc_probe() > and netvsc_remove() are happening simultaneously and netvsc_remove() > calls cancel_work_sync(&nvdev->subchan_work) before netvsc_sc_open() > calls napi_enable for the sub channels. Which causes NAPIF_STATE_SCHED > bit not getting cleared for the subchannels. > > Now during netvsc_device_remove(), when napi_disable is called for those > subchannels, napi_disable gets stuck on infinite msleep. > > Call trace: > [ 654.559417] task:modprobe state:D stack: 0 pid: 2321 ppid: > 1091 flags:0x00004002 > [ 654.568030] Call Trace: > [ 654.571221] <TASK> > [ 654.573790] __schedule+0x2d6/0x960 > [ 654.577733] schedule+0x69/0xf0 > [ 654.581214] schedule_timeout+0x87/0x140 > [ 654.585463] ? __bpf_trace_tick_stop+0x20/0x20 > [ 654.590291] msleep+0x2d/0x40 > [ 654.593625] napi_disable+0x2b/0x80 > [ 654.597437] netvsc_device_remove+0x8a/0x1f0 [hv_netvsc] > [ 654.603935] rndis_filter_device_remove+0x194/0x1c0 [hv_netvsc] > [ 654.611101] ? do_wait_intr+0xb0/0xb0 > [ 654.615753] netvsc_remove+0x7c/0x120 [hv_netvsc] > [ 654.621675] vmbus_remove+0x27/0x40 [hv_vmbus] > > Fixes: ac5047671758 ("hv_netvsc: Disable NAPI before closing the VMBus > channel") > Signed-off-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> Please add: Cc: stable@vger.kernel.org Otherwise, all look good! Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > --- > drivers/net/hyperv/netvsc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 1dafa44155d0..a6fcbda64ecc 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -708,7 +708,10 @@ void netvsc_device_remove(struct hv_device *device) > /* Disable NAPI and disassociate its context from the device. */ > for (i = 0; i < net_device->num_chn; i++) { > /* See also vmbus_reset_channel_cb(). */ > - napi_disable(&net_device->chan_table[i].napi); > + /* only disable enabled NAPI channel */ > + if (i < ndev->real_num_rx_queues) > + napi_disable(&net_device->chan_table[i].napi); > + > netif_napi_del(&net_device->chan_table[i].napi); > } > > -- > 2.34.1
> From: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> > Sent: Tuesday, January 30, 2024 2:16 AM > [...] > In commit ac5047671758 ("hv_netvsc: Disable NAPI before closing the > VMBus channel"), napi_disable was getting called for all channels, > including all subchannels without confirming if they are enabled or not. s/enabled/created/ > Which caused hv_netvsc getting hung at napi_disable, when > netvsc_probe() > and netvsc_remove() are happening simultaneously and netvsc_remove() Technically, they are not happening simultaneously: netvsc_probe() itself has finished, but the work item scheduled by it has not started yet. > calls cancel_work_sync(&nvdev->subchan_work) before netvsc_sc_open() > calls napi_enable for the sub channels. Which causes NAPIF_STATE_SCHED Technically, nvdev->subchan_work has not started to run yet, i.e. netvsc_subchan_work() -> rndis_set_subchannel() has not created the sub-channels yet, so netvsc_sc_open() can't run. It would be great if you could briefly explain how the NAPIF_STATE_SCHED bit is set and cleared, e.g. it's pre-set in rndis_filter_device_add() -> netif_napi_add() so if the sub-channels are not created, netvsc_sc_open() -> napi_enable() won't clear the flag and the flag remains set for ever for the sub-channels. > bit not getting cleared for the subchannels. > > Now during netvsc_device_remove(), when napi_disable is called for those > subchannels, napi_disable gets stuck on infinite msleep. The patch body looks good to me. Please post v2 with an updated changelog. Reviewed-by: Dexuan Cui <decui@microsoft.com>
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 1dafa44155d0..a6fcbda64ecc 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -708,7 +708,10 @@ void netvsc_device_remove(struct hv_device *device) /* Disable NAPI and disassociate its context from the device. */ for (i = 0; i < net_device->num_chn; i++) { /* See also vmbus_reset_channel_cb(). */ - napi_disable(&net_device->chan_table[i].napi); + /* only disable enabled NAPI channel */ + if (i < ndev->real_num_rx_queues) + napi_disable(&net_device->chan_table[i].napi); + netif_napi_del(&net_device->chan_table[i].napi); }
In commit ac5047671758 ("hv_netvsc: Disable NAPI before closing the VMBus channel"), napi_disable was getting called for all channels, including all subchannels without confirming if they are enabled or not. Which caused hv_netvsc getting hung at napi_disable, when netvsc_probe() and netvsc_remove() are happening simultaneously and netvsc_remove() calls cancel_work_sync(&nvdev->subchan_work) before netvsc_sc_open() calls napi_enable for the sub channels. Which causes NAPIF_STATE_SCHED bit not getting cleared for the subchannels. Now during netvsc_device_remove(), when napi_disable is called for those subchannels, napi_disable gets stuck on infinite msleep. Call trace: [ 654.559417] task:modprobe state:D stack: 0 pid: 2321 ppid: 1091 flags:0x00004002 [ 654.568030] Call Trace: [ 654.571221] <TASK> [ 654.573790] __schedule+0x2d6/0x960 [ 654.577733] schedule+0x69/0xf0 [ 654.581214] schedule_timeout+0x87/0x140 [ 654.585463] ? __bpf_trace_tick_stop+0x20/0x20 [ 654.590291] msleep+0x2d/0x40 [ 654.593625] napi_disable+0x2b/0x80 [ 654.597437] netvsc_device_remove+0x8a/0x1f0 [hv_netvsc] [ 654.603935] rndis_filter_device_remove+0x194/0x1c0 [hv_netvsc] [ 654.611101] ? do_wait_intr+0xb0/0xb0 [ 654.615753] netvsc_remove+0x7c/0x120 [hv_netvsc] [ 654.621675] vmbus_remove+0x27/0x40 [hv_vmbus] Fixes: ac5047671758 ("hv_netvsc: Disable NAPI before closing the VMBus channel") Signed-off-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> --- drivers/net/hyperv/netvsc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)