Re: [PATCH net-next v2 4/5] hv_netvsc: move a new VF to netvsc's netns from a work item

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] netvsc_resume() now always requeues vfns_work, and it does so
  from the vmbus noirq resume phase (vmbus_pm:…
- [Low] netvsc_register_vf() takes a tracked reference on a VF registered
  in another netns (netdev_hold(vf_netdev, &vfns_dev_tracker) and stores…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but netvsc_probe()
  can free the net_device while its embedded vfns_work is still queued.

--

hv_netvsc: move a new VF to netvsc's netns from a work item

The patch stops calling dev_change_net_namespace() directly from the VF's
NETDEV_REGISTER notifier, which deadlocks for ops-locked VFs. Instead it
takes a reference on the VF in vfns_dev and has vfns_work move it, or take
it over if netvsc reached the VF's netns first. Suspend now keeps vfns_dev,
and resume queues the work again.

[ ... ]

[Severity: Low]
What drops this reference if the VF is unregistered while it is parked
in vfns_dev?

The NETDEV_UNREGISTER handler finds netvsc only through the master upper
device:

drivers/net/hyperv/netvsc_drv.c:netvsc_unregister_vf() {
    ...
	ndev = get_netvsc_byref(vf_netdev);
	if (!ndev)
		return NOTIFY_DONE;
    ...
}

A VF that is held only in vfns_dev has not joined netvsc, so this lookup
returns NULL. Only vfns_work, through netvsc_event_set_vf_ns() and
netvsc_vfns_dev_put(), or netvsc_remove() drops the reference.

Usually the work runs soon and drops it when it sees reg_state !=
NETREG_REGISTERED. But netvsc_suspend() cancels vfns_work and keeps
vfns_dev until netvsc_resume() queues the work again.

If the VF is unregistered in that window, will unregister_netdevice()
wait in netdev_wait_allrefs_any() until thaw_noirq/restore_noirq, with
"waiting for ethX to become free" messages? netvsc ignores the repeated
NETDEV_UNREGISTER that the wait loop sends.

The window looks narrow, since hv_pci and the VF driver have already
suspended by then. Should the unregister path still check vfns_dev and
drop the reference there?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but netvsc_remove() cancels
vfns_work and the netvsc_probe() error path does not. Can netvsc_probe()
free the net_device while vfns_work is still queued?

register_netdevice(net) sends NETDEV_REGISTER for the netvsc device, and
netvsc_netdev_event() queues the work:

	if (event_dev->netdev_ops == &device_ops && event == NETDEV_REGISTER) {
		ndev_ctx = netdev_priv(event_dev);
		schedule_delayed_work(&ndev_ctx->vfns_work, 0);
		return NOTIFY_DONE;
	}

If a later NETDEV_REGISTER notifier rejects the device, register_netdevice()
calls unregister_netdevice_queue() and returns an error. For example,
mpls_dev_notify() returns an error when mpls_add_dev() fails with -ENOMEM.

netvsc_probe() then goes through register_failed -> rndis_failed -> ... ->
free_netdev(net), and it never calls
cancel_delayed_work_sync(&net_device_ctx->vfns_work).

Probe holds RTNL at this point, so rtnl_trylock() fails in
netvsc_vfns_work() and the work requeues itself with a 1 jiffy delay.

If the work is still pending when free_netdev() frees the netdev_priv
memory, wouldn't the timer and workqueue linkage, and the container_of()
in netvsc_vfns_work(), touch freed memory?

[ ... ]

[Severity: Medium]
Can this run the VF move or takeover before the VF's own driver has
resumed?

netvsc_resume() is called from the vmbus noirq phase:

drivers/hv/vmbus_drv.c:vmbus_pm {
	.freeze_noirq	= vmbus_suspend,
	.thaw_noirq	= vmbus_resume,
	.poweroff_noirq	= vmbus_suspend,
	.restore_noirq	= vmbus_resume,
}

vfns_work runs on system_percpu_wq, which isn't freezable, and it only
needs rtnl_trylock(). It can therefore run before the VF's PCI driver gets
its normal-phase .thaw/.restore callback. This happens when vfns_dev, or a
netvsc netns move, is still pending when netvsc_suspend() cancels the
work.

netvsc_event_set_vf_ns() then calls one of two paths:

- dev_change_net_namespace(), which closes the VF via ndo_stop
- netif_close() followed by netvsc_register_vf()

The second path calls ndo_bpf, ndo_set_features and netvsc_vf_join(), and
netvsc_vf_join() schedules vf_takeover and dev_open().

Take mana as an example. mana_gd_suspend() -> mana_remove(..., true) ->
mana_detach() saves port_st_save and clears port_is_up. The early
netif_close() then hits:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_close() {
    ...
	if (!apc->port_is_up)
		return 0;
    ...
}

The core then clears IFF_UP. On resume, mana_attach() brings the port
back up from port_st_save. The hardware port is then up while the netdev
is marked down.

A later dev_open() from vf_takeover would then either fail with -ENODEV
while the device is still detached, or call mana_open() ->
mana_alloc_queues() a second time on a port that is already up.

Before this patch, netvsc_resume() didn't touch the VF. Should this
requeue wait until the VF driver has resumed?

 	rtnl_unlock();
 
 	return ret;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109194843.1353592-1-kuba%40kernel.org