Re: [PATCH net-next v2 3/5] hv_netvsc: treat the VF's XDP program the way bonding treats a slave's

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · 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_unregister_vf()
  (drivers/net/hyperv/netvsc_drv.c:2487-2494) takes netvsc's program…
- [Low] The commit message says that installing netvsc's program before
  joining means 'nothing reaches netvsc through the VF without having…

--

hv_netvsc: treat the VF's XDP program the way bonding treats a slave's

This patch makes netvsc refuse to take over a VF that has its own XDP
program, or that can't or won't run netvsc's program. It installs
netvsc's program on the VF before joining it. It also takes netvsc's
program back from the VF in netvsc_unregister_vf().

> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1d43c73fd73f1..61109ff9927fb 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c

[ ... ]

> @@ -2349,10 +2356,31 @@ static int netvsc_register_vf(struct net_device *vf_netdev, int context)
>  		return NOTIFY_DONE;
>  	}
>  
> +	/* Install netvsc's program before joining, so that a VF which
> +	 * refuses it never gets used. Syncing the features first turns
> +	 * off LRO, which the VF may not run XDP with.
> +	 */
> +	if (prog) {
> +		vf_netdev->wanted_features = ndev->features;
> +		netdev_update_features(vf_netdev);
> +
> +		ret = netvsc_vf_setxdp(vf_netdev, prog);
> +		if (ret) {
> +			netdev_warn(ndev, "not using VF %s, it refused netvsc's XDP program: %d\n",
> +				    vf_netdev->name, ret);
> +			return NOTIFY_DONE;
> +		}
> +	}
> +
>  	netdev_info(ndev, "VF registering: %s\n", vf_netdev->name);
>  
> -	if (netvsc_vf_join(vf_netdev, ndev, context) != 0)
> +	if (netvsc_vf_join(vf_netdev, ndev, context) != 0) {

[Severity: Low]

The commit message says that installing the program before joining means
"nothing reaches netvsc through the VF without having been through the
program". Does this ordering fully guarantee that?

netvsc_vf_setxdp() is followed directly by netvsc_vf_join(), which calls
netdev_rx_handler_register(). That function publishes the handler without
any synchronization:

net/core/dev.c:netdev_rx_handler_register() {
    ...
	rcu_assign_pointer(dev->rx_handler_data, rx_handler_data);
	rcu_assign_pointer(dev->rx_handler, rx_handler);
    ...
}

Some VF drivers also publish the new program without waiting for NAPI.
mana is one example:

drivers/net/ethernet/microsoft/mana/mana_bpf.c:mana_chn_setxdp() {
    ...
	rcu_assign_pointer(apc->rxqs[i]->bpf_prog, prog);
    ...
}

Suppose a VF NAPI poll on another CPU has already read a NULL
rxq->bpf_prog. It could build an skb, hold it in GRO, and deliver it after
rx_handler becomes visible. netvsc_vf_handle_frame() would then pass that
frame to netvsc without it having run the XDP program.

The teardown side does wait, because netdev_rx_handler_unregister() calls
synchronize_net(). The setup side mostly matters for the netvsc_probe()
path (VF_REG_IN_PROBE), where the VF may already be up and polling. The
window is about one NAPI poll, and the ordering is still better than it
was before the patch.

Would a synchronize_net() between netvsc_vf_setxdp() and netvsc_vf_join()
make the claim hold? If not, should the commit message be softened?

[ ... ]

> @@ -2453,6 +2480,20 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
>  
>  	reinit_completion(&net_device_ctx->vf_add);
>  	netdev_rx_handler_unregister(vf_netdev);
> +
> +	/* Take our program back now that frames from the VF no longer
> +	 * reach netvsc, but leave a program of the VF's own be.
> +	 */
> +	nvdev = rtnl_dereference(net_device_ctx->nvdev);
> +	if (nvdev && netvsc_xdp_get(nvdev) && !dev_xdp_prog_count(vf_netdev)) {

[Severity: Medium]

This take-back depends on netvsc's channel program, not on whether a
program was actually pushed to the VF. Can the VF be left running
netvsc's program when the two get out of sync?

There seem to be two reachable states where the condition is false while
the VF stays joined and still runs the program from netvsc_vf_setxdp().

In the first state, nvdev exists but its channels are empty.
netvsc_detach() clears the channel program before steps that can still
fail:

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

	/* If device was up (receiving) then shutdown */
	if (netif_running(ndev)) {
		netvsc_tx_disable(nvdev, ndev);

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

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

If this fails, netvsc_change_mtu() goes to rollback_vf and
netvsc_set_channels() goes to out, and neither one re-attaches. nvdev is
still there, but netvsc_xdp_get(nvdev) returns NULL and the VF was never
touched.

In the second state, nvdev is NULL. If netvsc_attach() fails, either on
an rndis_filter_device_add() error or through the err1/err2 paths,
ndev_ctx->nvdev stays NULL. That happens when both attach attempts fail
in change_mtu or set_channels. It also happens when the single attempt
fails in netvsc_resume(), which drops the saved bprog as well.

In either state, any later path into netvsc_unregister_vf() skips the
take-back: a VF hot-remove, a netns move, netvsc_remove(), unbind, or
rmmod. The VF is then unlinked still running netvsc's program, and its
driver keeps the reference. Isn't that the same orphaned program and
mana leak that the commit message says this patch fixes?

When nvdev is NULL, netvsc_bpf() also returns -ENODEV for a removal while
the VF is joined. So the program can't be cleared through netvsc either.

The patch also removes this push after join in netvsc_register_vf():

	prog = netvsc_xdp_get(netvsc_dev);
	netvsc_vf_setxdp(vf_netdev, prog);

That unconditional push used to clear such a leftover when the VF
registered again with a netvsc that has no program. Now the stale program
survives re-registration. dev_xdp_prog_count() can't see it, because it
was propagated rather than installed on the VF through dev_xdp_install().

The commit message compares this to bonding keying its release on
bond->xdp_prog. The two behave differently: bond->xdp_prog lasts as long
as the bond has a program attached. netvsc's channel pointer is torn down
on every detach/attach and on a failed reconfiguration.

The later patches in the series ("move a new VF to netvsc's netns from a
work item" and "let the core take XDP off a netvsc device that is going
away") leave this condition unchanged.

Could the release be keyed on what was actually pushed? One way is a
pointer or flag in net_device_ctx that is set when
netvsc_vf_setxdp(vf, prog) succeeds. Another is to have the detach and
attach failure paths also pull the program off the VF.

> +		netdev_lock_ops(vf_netdev);
> +		ret = netvsc_vf_setxdp(vf_netdev, NULL);
> +		netdev_unlock_ops(vf_netdev);
> +		if (ret)
> +			netdev_warn(ndev, "failed to remove XDP program from VF %s: %d\n",
> +				    vf_netdev->name, ret);
> +	}
> +
>  	netdev_upper_dev_unlink(vf_netdev, ndev);
>  	RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);
>  	dev_put(vf_netdev);

[ ... ]

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