Re: [PATCH net-next 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 6 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 4 · 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).

- [High] netvsc_vf_setxdp() returns 0 without installing anything when
  the VF has no ndo_bpf (drivers/net/hyperv/netvsc_bpf.c:180-181, `if…
- [Medium] netvsc_unregister_vf() only takes the program back from the VF
  when `nvdev && netvsc_xdp_get(nvdev)`, that is, when netvsc's channels…
- [Medium] Two calls to netvsc_vf_setxdp(vf_netdev, NULL) drop their
  result without logging: the new take-back in netvsc_unregister_vf()…
- [Medium] netvsc_register_vf() now refuses a VF with its own program
  (dev_xdp_prog_count(vf_netdev) != 0), but only at join time and only…
- [Medium] Once netvsc refuses a VF at registration, nothing retries the
  takeover.
- [Low] Before installing the program, netvsc_register_vf() now sets
  `vf_netdev->wanted_features = ndev->features;…

Pre-existing issues:
- [High] At this commit, netvsc_register_vf() still calls
  dev_change_net_namespace(vf_netdev, dev_net(ndev), "eth%d") directly…

--

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

When netvsc has an XDP program, netvsc_register_vf() now refuses a VF
that has its own program or that refuses netvsc's. It also installs
netvsc's program before joining the VF. netvsc_unregister_vf() now takes
netvsc's program back from the VF when the VF leaves, and
netvsc_remove() releases the VF before clearing the channel program.

> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1d43c73fd73f1..17c86e7493577 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -2331,6 +2331,13 @@ static int netvsc_register_vf(struct net_device *vf_netdev, int context)
>  	if (!netvsc_dev || rtnl_dereference(net_device_ctx->vf_netdev))
>  		return NOTIFY_DONE;
>  
> +	prog = netvsc_xdp_get(netvsc_dev);
> +	if (prog && dev_xdp_prog_count(vf_netdev)) {
> +		netdev_warn(ndev, "not using VF %s, it has an XDP program attached\n",
> +			    vf_netdev->name);
> +		return NOTIFY_DONE;
> +	}

[Severity: Medium]
A VF with its own program is only refused here, at join time, and only
when netvsc already has a program. Does netvsc_bpf() need the same
check? It still propagates onto a joined VF without looking at the VF's
own program:

drivers/net/hyperv/netvsc_bpf.c:netvsc_bpf() {
    ...
		if (vf_netdev) {
			netdev_lock_ops(vf_netdev);
			ret = netvsc_vf_setxdp(vf_netdev, bpf->prog);
			netdev_unlock_ops(vf_netdev);
		}
    ...
}

A joined VF can get its own program while netvsc has none.
dev_xdp_attach() on the VF only refuses when an upper has a program,
and nothing checks the lowers when a program is attached to netvsc.
Here is one sequence:

  netvsc has no program, VF joins
  admin attaches P to the VF (allowed, the upper has no program)
  admin attaches Q to netvsc
    netvsc_bpf()->netvsc_vf_setxdp()->netif_xdp_propagate()
      VF driver now runs Q, VF xdp_state still records P
  VF leaves
    netvsc_unregister_vf()->netvsc_vf_setxdp(vf_netdev, NULL)
      VF driver runs nothing, VF xdp_state still reports P

After that, the next netvsc_register_vf() refuses the VF because of a
program that is not running.

Bonding repeats the check for every slave in bond_xdp_set():

		if (dev_xdp_prog_count(slave_dev) > 0) {
			SLAVE_NL_ERR(dev, slave_dev, extack,
				     "Slave has XDP program loaded, please unload before enslaving");

The commit message names this as one of the problems being fixed: "A VF
which runs a program of its own gets it replaced or removed behind the
back of whoever attached it". The core change it mentions ("the core is
about to refuse propagating over a device's own program") is not part
of this series.

> +
>  	/* if synthetic interface is a different namespace,
>  	 * then move the VF to that namespace; join will be
>  	 * done again in that context.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The
netns branch here still calls dev_change_net_namespace() directly from
the VF's NETDEV_REGISTER notifier:

	if (!net_eq(dev_net(ndev), dev_net(vf_netdev))) {
		ret = dev_change_net_namespace(vf_netdev,
					       dev_net(ndev), "eth%d");

Since commit 4c975fd70002, register_netdevice() holds the VF's instance
lock across NETDEV_REGISTER. __dev_change_net_namespace() then calls
netdev_lock_ops() on the same VF again.

Can this self-deadlock with RTNL held for ops-locked VFs such as mana or
mlx5? That would happen when netvsc is in a non-init netns and the VF
is hot-added into init_net.

The later patch in this series, "hv_netvsc: move a new VF to netvsc's
netns from a work item", replaces the direct call with a hand-off to
vfns_work, so this is resolved by the end of the series.

[ ... ]

> @@ -2349,10 +2356,29 @@ 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);

[Severity: Low]
This replaces the VF's entire wanted feature set, not only LRO, before
netvsc knows whether it will use the VF. If the VF then refuses the
program, or netvsc_vf_join() fails below, netvsc returns NOTIFY_DONE
and the original wanted_features are not restored.

Is it intended that offloads the user or driver enabled on the VF, and
that netvsc doesn't carry, stay off on a device netvsc doesn't own?
The commit message only mentions LRO: "a VF which refuses the program
all the same keeps LRO off". Before this patch, the full sync only ran
after a successful join.

Would clearing only NETIF_F_LRO here avoid the side effect?

> +
> +		ret = netvsc_vf_setxdp(vf_netdev, prog);

[Severity: High]
Can netvsc_vf_setxdp() return 0 here without installing anything? It
returns early when the VF has no ndo_bpf, before netif_xdp_propagate()
gets the chance to return -EOPNOTSUPP:

drivers/net/hyperv/netvsc_bpf.c:netvsc_vf_setxdp() {
    ...
	if (!vf_netdev->netdev_ops->ndo_bpf)
		return 0;
    ...
}

Suppose netvsc has a native program and the matched VF has no ndo_bpf
(iavf, for example). The refusal branch is then skipped and the VF is
joined. netvsc_vf_handle_frame() hands the VF's frames to netvsc, and
they never go through netvsc's program, which only runs on the synthetic
channels. Nothing is logged.

netvsc_bpf() has the same early return, so attaching a program to
netvsc while such a VF is joined also appears to succeed.

That seems to contradict two statements in the commit message: "nothing
reaches netvsc through the VF without having been through the program"
and "a VF netvsc uses now always runs netvsc's program when netvsc has
one".

Bonding, which this patch follows, refuses such a slave in
bond_enslave():

	if (!slave_dev->netdev_ops->ndo_bpf ||
	    !slave_dev->netdev_ops->ndo_xdp_xmit) {
		if (bond->xdp_prog) {
			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
				     "Slave does not support XDP");

> +		if (ret) {
> +			netdev_warn(ndev, "not using VF %s, it refused netvsc's XDP program: %d\n",
> +				    vf_netdev->name, ret);
> +			return NOTIFY_DONE;
> +		}
> +	}

[Severity: Medium]
Once a VF is refused here, or by the dev_xdp_prog_count() check above,
what brings it back later?

netvsc_register_vf() is only called from netvsc_probe() and from the
VF's NETDEV_REGISTER notifier. The later patch "hv_netvsc: move a new VF
to netvsc's netns from a work item" adds a netns-move path.

netvsc_vf_changed() finds netvsc through get_netvsc_byref(), which needs
the master upper link that a refused VF never gets:

drivers/net/hyperv/netvsc_drv.c:get_netvsc_byref() {
    ...
	dev = netdev_master_upper_dev_get(vf_netdev);
	if (!dev || dev->netdev_ops != &device_ops)
		return NULL;
    ...
}

netvsc_bpf() only touches vf_netdev when a VF is already joined.

So detaching netvsc's program, or removing the VF's own program, does
not retry the takeover. The accelerated datapath stays off until the VF
registers again, for example on a host remove/re-add or a manual netns
round-trip.

The handling is also asymmetric. If the VF is already joined, attaching
a program it refuses fails the attach and the VF is kept. If the program
was there first, the VF is lost until it re-registers.

The commit message only says "traffic then stays on the synthetic path,
where the program runs". Bonding refuses a user-initiated enslave that
can be retried, but netvsc's takeover is automatic.

Should something retry the takeover when the program goes away, or
should the commit message at least mention this?

> +
>  	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) {
> +		if (prog)
> +			netvsc_vf_setxdp(vf_netdev, NULL);
>  		return NOTIFY_DONE;
> +	}

[ ... ]

> @@ -2453,6 +2477,15 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
>  
>  	reinit_completion(&net_device_ctx->vf_add);
>  	netdev_rx_handler_unregister(vf_netdev);
> +
> +	/* Only once frames from the VF no longer reach netvsc */
> +	nvdev = rtnl_dereference(net_device_ctx->nvdev);
> +	if (nvdev && netvsc_xdp_get(nvdev)) {

[Severity: Medium]
The take-back depends on netvsc's channels carrying a program right
now. Can the channel program be cleared while nvdev stays installed and
the VF still holds netvsc's propagated program, along with the reference
netvsc_vf_setxdp() took for it?

One case is a failed replace in netvsc_bpf(). P1 runs on netvsc and the
VF. Attaching P2 calls netvsc_xdp_set(P2), and then the VF refuses P2.
For example, mana_xdp_set() restores apc->bpf_prog = old_prog when
mana_pre_alloc_rxbufs() fails, and netif_xdp_propagate() returns -EBUSY
with tcp-data-split or a memory provider. The rollback then leaves the
channels empty:

drivers/net/hyperv/netvsc_bpf.c:netvsc_bpf() {
    ...
			if (bpf->prog)
				bpf_prog_inc(bpf->prog);
			netvsc_xdp_set(dev, NULL, extack, nvdev);
    ...
}

The VF keeps P1, and because the install failed, dev_xdp_attach() keeps
P1 in netvsc's xdp_state.

Another case is netvsc_detach(). It calls netvsc_xdp_set(ndev, NULL,
NULL, nvdev) first, and can then return early when rndis_filter_close()
or netvsc_wait_until_empty() fails, with nvdev still installed.
netvsc_set_channels(), for one, just bails out:

	ret = netvsc_detach(net, nvdev);
	if (ret)
		goto out;

In both states, the VF can later leave: VF unregister, netns move,
netvsc unbind or hot-remove, or the rmmod notifier replay. This check is
then false.

Does the VF keep running P1 with no upper, and does its driver's
reference to P1 leak?

The patch also removes the unconditional push at registration. The
commit message says that push "was all that cleared a program an earlier
netvsc instance left on the VF". A VF in this state that rejoins a
netvsc with no program, for example after reloading hv_netvsc, would
keep the stale program. dev_xdp_prog_count() cannot see it.

The nvdev == NULL paths, such as both netvsc_attach() attempts failing
or suspend, don't reach this check. get_netvsc_byref() already returns
NULL when nvdev is NULL, and that behaviour predates this patch.

> +		netdev_lock_ops(vf_netdev);
> +		netvsc_vf_setxdp(vf_netdev, NULL);

[Severity: Medium]
The return value of netvsc_vf_setxdp(vf_netdev, NULL) is dropped here.
The VF is then unlinked and released as though the program had been
removed. Can the removal fail?

netif_xdp_propagate() returns -EBUSY whenever a memory provider is
bound, even for a NULL program:

	if (dev_get_min_mp_channel_count(dev)) {
		NL_SET_ERR_MSG(bpf->extack, "unable to propagate XDP to device using memory provider");
		return -EBUSY;
	}

A memory provider can be bound to a joined VF that runs netvsc's
program. __net_mp_open_rxq() only checks dev_xdp_prog_count(dev), and
that count doesn't include propagated programs.

mana_xdp_set(NULL) on an up port can also fail in mana_pre_alloc_rxbufs(),
mana_detach() or mana_attach(). Each of those paths restores
apc->bpf_prog = old_prog and returns before bpf_prog_put(old_prog).

After such a failure, the VF keeps netvsc's program and the reference,
with no upper left to clean them up, and nothing is logged. The rollback
after a failed netvsc_vf_join() in netvsc_register_vf() also ignores the
result.

Bonding at least warns in __bond_release_one():

		if (dev_xdp_propagate(slave_dev, &xdp))
			slave_warn(bond_dev, slave_dev, "failed to unload XDP program\n");

Should these two calls at least log a failure?

> +		netdev_unlock_ops(vf_netdev);
> +	}
> +
>  	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/20261001014131.310771-1-kuba%40kernel.org