Re: [PATCH net-next 5/5] hv_netvsc: let the core take XDP off a netvsc device that is going away

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

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] The new branch in netvsc_bpf()
  (drivers/net/hyperv/netvsc_bpf.c:206-213) returns success for…
- [Medium] The fix treats a non-NULL ndev_ctx->saved_netvsc_dev_info as
  meaning 'suspend parked the program and netvsc_resume() will put it…

--

hv_netvsc: let the core take XDP off a netvsc device that is going away

With this patch, netvsc_bpf() returns success for an XDP program removal
when nvdev is gone, no VF is linked and suspend has not saved its config.
The aim is to stop dev_xdp_uninstall() from hitting its WARN_ON() on
unbind or hot-remove of a netvsc device running native XDP. It also lets
the program be dropped from the XDP dispatcher.

[Severity: Medium]
Does !vf_netdev here guarantee that the program is gone from the VF?

netvsc_unregister_vf() takes netvsc's program back from the VF only while
nvdev exists and the channels still have a program. It clears vf_netdev
in every case:

drivers/net/hyperv/netvsc_drv.c:netvsc_unregister_vf() {
    ...
	nvdev = rtnl_dereference(net_device_ctx->nvdev);
	if (nvdev && netvsc_xdp_get(nvdev)) {
		netdev_lock_ops(vf_netdev);
		netvsc_vf_setxdp(vf_netdev, NULL);
		netdev_unlock_ops(vf_netdev);
	}

	netdev_upper_dev_unlink(vf_netdev, ndev);
	RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);
    ...
}

nvdev can be NULL while the VF is still linked and still runs netvsc's
program. netvsc_detach() clears XDP only on the channels:

drivers/net/hyperv/netvsc_drv.c:netvsc_detach() {
    ...
	netvsc_xdp_set(ndev, NULL, NULL, nvdev);
    ...
}

Both attaches can then fail in netvsc_set_channels().
netvsc_change_mtu() and netvsc_set_ringparam() follow the same pattern:

drivers/net/hyperv/netvsc_drv.c:netvsc_set_channels() {
    ...
	ret = netvsc_attach(net, device_info);
	if (ret) {
		device_info->num_chn = orig;
		if (netvsc_attach(net, device_info))
			netdev_err(net, "restoring channel setting failed\n");
	}
    ...
}

After that, nvdev stays NULL. The VF still holds the program, along with
the reference that netvsc_vf_setxdp() took through
netif_xdp_propagate().

A later unbind, hot-remove, rmmod or netns move would then do this:

netvsc_unregister_vf()
    nvdev == NULL, so netvsc_vf_setxdp(vf_netdev, NULL) is skipped
    vf_netdev = NULL
dev_xdp_uninstall()->dev_xdp_install(NULL)->netvsc_bpf()
    nvdev == NULL, vf_netdev == NULL, saved_netvsc_dev_info == NULL
    return 0

At that point the core treats the program as gone. The former VF keeps
running netvsc's XDP program on its traffic, and nothing records the
program in the VF's xdp_state.

Before this patch, this state at least triggered the WARN in
dev_xdp_uninstall(). The gating in netvsc_unregister_vf() comes from the
earlier commit in the series, "hv_netvsc: treat the VF's XDP program the
way bonding treats a slave's". The new guard and comment assume that
gating covers every case.

Could the take-back in netvsc_unregister_vf() check what netvsc has
recorded, for example dev_xdp_prog_count(ndev), instead of the channel
state? Alternatively, should the guard here stop treating !vf_netdev as
proof that the program was removed?

[Severity: Medium]
Can saved_netvsc_dev_info be non-NULL here even though netvsc_resume()
will never run to put the program back?

netvsc_suspend() saves the config, which takes a bprog reference through
netvsc_devinfo_get(), and only then calls netvsc_detach():

drivers/net/hyperv/netvsc_drv.c:netvsc_suspend() {
    ...
	ndev_ctx->saved_netvsc_dev_info = netvsc_devinfo_get(nvdev);
	...
	ret = netvsc_detach(net, nvdev);
    ...
}

netvsc_detach() clears the channel programs first and can fail after
that:

drivers/net/hyperv/netvsc_drv.c:netvsc_detach() {
    ...
	netvsc_xdp_set(ndev, NULL, NULL, nvdev);
	...
		ret = rndis_filter_close(nvdev);
		if (ret) {
			...
			return ret;
		}

		ret = netvsc_wait_until_empty(nvdev);
		if (ret) {
			...
			return ret;
		}
    ...
}

The error goes back through vmbus_suspend(), the noirq callback. Because
the callback failed, device_suspend_noirq() does not set
is_noirq_suspended. device_resume_noirq() then skips the device, so
netvsc_resume() never runs. netvsc_resume() is the only code that puts
and clears saved_netvsc_dev_info, and netvsc_remove() does not touch it.

A later unbind or hot-remove with native XDP still attached would then
go like this:

netvsc_remove()
    netvsc_xdp_set(net, NULL, NULL, nvdev)
    rndis_filter_device_remove()    /* nvdev becomes NULL */
    unregister_netdevice()
        dev_xdp_uninstall()
            dev_xdp_install(NULL)->netvsc_bpf()
                saved_netvsc_dev_info != NULL
                return -ENODEV
            WARN_ON(...)

On this path the WARN_ON() in dev_xdp_uninstall() that this patch is
meant to remove still fires. bpf_prog_change_xdp() is also skipped, so
the XDP dispatcher keeps its reference to the program.

The commit message gives the reason for the exclusion as:

  "unless suspend parked the program for resume to put back, as the core
  would then lose track of it"

dev_xdp_uninstall() drops xdp_state and the core's reference no matter
what the driver returns. Does that reason still hold on this path?

The leaked devinfo and its bprog reference predate this patch, but the
fix now depends on that stale pointer. Should saved_netvsc_dev_info be
put and cleared on the netvsc_suspend() error path, and in
netvsc_remove() before unregister_netdevice()?

[ ... ]

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