hv_netvsc: Fix race condition between netvsc_probe and netvsc_remove
Subsystems (6)
PATCH v2 1/1 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.
This caused hv_netvsc getting hung at napi_disable, when netvsc_probe() has finished running but nvdev->subchan_work has not started yet. netvsc_subchan_work() -> rndis_set_subchannel() has not created the sub-channels and because of that netvsc_sc_open() is not running. netvsc_remove() calls cancel_work_sync(&nvdev->subchan_work), for which netvsc_subchan_work did not run.
netif_napi_add() sets the bit NAPI_STATE_SCHED because it ensures NAPI cannot be scheduled. Then netvsc_sc_open() -> napi_enable will clear the NAPIF_STATE_SCHED bit, so it can be scheduled. napi_disable() does the opposite.
Now during netvsc_device_remove(), when napi_disable is called for those subchannels, napi_disable gets stuck on infinite msleep.
This fix addresses this problem by ensuring that napi_disable() is not getting called for non-enabled NAPI struct.
But netif_napi_del() is still necessary for these non-enabled NAPI struct for cleanup purpose.
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]
---
V1 -> V2:
Changed commit message, added some more details on napi NAPIF_STATE_SCHED bit set and reset.
--- drivers/net/hyperv/netvsc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/hyperv/netvsc.c +4 -1
@@ -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); }
Signature · 1 line
2.34.1
Quoted 40 lines · Souradeep Chakrabarti
In commit
ac5047671758("hv_netvsc: Disable NAPI before closing the VMBus channel"),napi_disablewas getting called for all channels, including all subchannels without confirming if they are enabled or not.This caused
hv_netvscgetting hung atnapi_disable, whennetvsc_probe()has finished running butnvdev->subchan_workhas not started yet.netvsc_subchan_work()->rndis_set_subchannel()has not created the sub-channels and because of thatnetvsc_sc_open()is not running.netvsc_remove()callscancel_work_sync(&nvdev->subchan_work), for whichnetvsc_subchan_workdid not run.
netif_napi_add()sets the bitNAPI_STATE_SCHEDbecause it ensures NAPI cannot be scheduled. Thennetvsc_sc_open()->napi_enablewill clear theNAPIF_STATE_SCHEDbit, so it can be scheduled.napi_disable()does the opposite.Now during
netvsc_device_remove(), whennapi_disableis called for those subchannels,napi_disablegets stuck on infinite msleep.This fix addresses this problem by ensuring that
napi_disable()is not getting called for non-enabled NAPI struct.
Butnetif_napi_del()is still necessary for these non-enabled NAPI struct for cleanup purpose.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]
Quoted 5 lines
Fixes ac5047671758 ("hv_netvsc: Disable NAPI before closing the VMBus channel")Signed-off-by Souradeep Chakrabarti <[email protected]>Reviewed-by Dexuan Cui <[email protected]>Reviewed-by Haiyang Zhang <[email protected]>
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 4 lines · On Tue, 30 Jan 2024 23:35:51 -0800 you wrote:
In commit
ac5047671758("hv_netvsc: Disable NAPI before closing the VMBus channel"),napi_disablewas getting called for all channels, including all subchannels without confirming if they are enabled or not.
Quoted 8 lines
This caused
hv_netvscgetting hung atnapi_disable, whennetvsc_probe()has finished running butnvdev->subchan_workhas not started yet.netvsc_subchan_work()->rndis_set_subchannel()has not created the sub-channels and because of thatnetvsc_sc_open()is not running.netvsc_remove()callscancel_work_sync(&nvdev->subchan_work), for whichnetvsc_subchan_workdid not run.[...]
Here is the summary with links:
- [V2,net] hv_netvsc: Fix race condition between netvsc_probe and netvsc_remove
https://git.kernel.org/netdev/net/c/e0526ec5360a
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html