netvsc_register_vf() pushes netvsc's XDP state down to every VF it takes over - its program, or NULL if it has none - once the VF has joined, and ignores the result. A VF which refuses the program leaves netvsc running XDP on its channels while traffic through the VF bypasses it, with nothing in the logs to say so. The core won't install a single-buffer program on a device with tcp-data-split enabled, nor any program on one with a memory provider, and the VF's driver has conditions of its own. A VF which can't run XDP at all isn't even asked, and netvsc_bpf() reports success for it too. A VF which runs a program of its own gets it replaced or removed behind the back of whoever attached it, when it joins as well as when netvsc gets a program later. Nothing takes netvsc's program back when the VF stops being netvsc's either. netvsc_unregister_vf() runs when the VF is unregistered, when either device moves to another netns (netvsc drags the VF along when it moves itself), when netvsc is unbound or hot-removed, and on rmmod hv_netvsc, where unregistering the notifier replays NETDEV_UNREGISTER before netvsc_remove() ever runs. In all but the first case the VF stays, running netvsc's program with no upper left to own it. We will soon track in the core if the program is installed from upper; a leftover would then become the VF's own, and netvsc would refuse to take the VF back until someone removed it. netvsc_unregister_vf() used to propagate NULL to the VF, until commit 3ec523304976 ("hv_netvsc: fix potential deadlock in netvsc_vf_setxdp()") dropped the call on the grounds that the core cleans up through dev_xdp_uninstall(). That only runs when the VF itself is unregistered, and doesn't know about propagated programs at all, so even then the VF's driver keeps its reference to netvsc's program; mana, for one, leaks it on every VF removal while netvsc runs XDP. Do what bonding does with its slaves instead. With no program on netvsc there is nothing to push down, so leave the VF's program alone. With a program on netvsc, don't take over a VF which runs one of its own, in any mode, or which can't or won't run netvsc's, and say why in the log. netvsc then keeps its traffic on the synthetic path, where the program runs, and doesn't use the VF. Nothing retries taking the VF over until it registers again, just as bonding doesn't retry an enslave it refused. For a VF already in use fail attaching the program to netvsc instead, as netvsc_bpf() already did when the VF refused it, but don't fail a removal over the VF's own program, leave that program be. Install the program before joining the VF, so that a refusal needs no unwinding and nothing reaches netvsc through the VF without having been through the program, and sync the VF's features ahead of it, as LRO may keep the VF from taking XDP; a VF which refuses the program all the same is left with netvsc's features. Take the program back when the VF leaves, under the VF's lock, which none of the paths into netvsc_unregister_vf() hold, once netvsc's rx handler is gone, and warn if the VF won't let go of it, as bonding does, but leave a program of the VF's own be, as on removal. Key that on the program on netvsc's channels, the way bonding keys its release on bond->xdp_prog; a generic program on netvsc never reached the VF. Let the VF go in netvsc_remove() before the channels are cleared. The two halves only work together. The NULL pushed at registration was all that cleared a program an earlier netvsc instance left on the VF, and taking the program back without the refusal would strip a VF of its own program after it was joined despite refusing netvsc's. A VF netvsc uses now always runs netvsc's program when netvsc has one. Update netvsc's documentation, which still said that a program on the VF may get overwritten, and describe the VFs netvsc doesn't use. Reported by Sashiko during core rework. Unverified and untested. Cc: stable+noautosel@kernel.org # LLM report + LLM fix, untested Fixes: 351e1581395f ("hv_netvsc: Add XDP support") Fixes: 3ec523304976 ("hv_netvsc: fix potential deadlock in netvsc_vf_setxdp()") Signed-off-by: Jakub Kicinski --- v2: - refuse a VF which can't run XDP while netvsc has a program, and fail attaching a program to netvsc while such a VF is in use (Sashiko) - fail attaching a program to netvsc while the VF in use runs one of its own, rather than leave that to the core series, but leave the VF's program be on removal, and when the VF leaves (Sashiko) - warn when the VF won't give netvsc's program back (Sashiko) - move the documentation update here from the core series, and don't claim that the refused VF's traffic stays on the synthetic path (Vennela) - commit msg: say that nothing retries a refused VF, and that it keeps netvsc's features (Sashiko) - Tested-by tags from v1 not carried over, the behavior changed v1: https://lore.kernel.org/20261001014131.310771-4-kuba@kernel.org --- .../ethernet/microsoft/netvsc.rst | 11 +++- drivers/net/hyperv/netvsc_bpf.c | 15 ++++- drivers/net/hyperv/netvsc_drv.c | 65 +++++++++++++++---- 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst b/Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst index 0d464706a59a..7f1731a819f9 100644 --- a/Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst +++ b/Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst @@ -109,8 +109,15 @@ XDP support Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to VF NIC automatically. Setting / unsetting XDP program on VF NIC directly - is not recommended, also not propagated to synthetic NIC, and may be - overwritten by setting of synthetic NIC. + is not recommended, and not propagated to synthetic NIC. If the + synthetic NIC has a program when a VF NIC shows up, a VF NIC which runs + a program of its own, or which can't or won't run the synthetic NIC's, + is not used until it is registered again. The synthetic NIC then keeps + its traffic on the synthetic data path; what the VF NIC receives if it + is brought up on its own is outside of the synthetic NIC's control. + For a VF NIC already in use which runs a program of its own, or can't + or won't run the synthetic NIC's, setting the program on the synthetic + NIC fails instead. XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO before running XDP:: diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c index ec8edfaebf0b..143fc0c12841 100644 --- a/drivers/net/hyperv/netvsc_bpf.c +++ b/drivers/net/hyperv/netvsc_bpf.c @@ -176,8 +176,9 @@ int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog) netdev_assert_locked_ops_compat(vf_netdev); + /* as with bonding, a VF which can't run XDP can't carry ours */ if (!vf_netdev->netdev_ops->ndo_bpf) - return 0; + return prog ? -EOPNOTSUPP : 0; memset(&xdp, 0, sizeof(xdp)); @@ -210,6 +211,18 @@ int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf) switch (bpf->command) { case XDP_SETUP_PROG: + /* The VF's own program is not ours: refuse to install over + * it, as bonding does, and leave it be on removal. + */ + if (vf_netdev && dev_xdp_prog_count(vf_netdev)) { + if (bpf->prog) { + NL_SET_ERR_MSG_MOD(extack, + "VF has an XDP program of its own"); + return -EOPNOTSUPP; + } + vf_netdev = NULL; + } + /* Hold on to the old program, to put it back on the channels * if the VF refuses the change. */ diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 1d43c73fd73f..61109ff9927f 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; + } + /* if synthetic interface is a different namespace, * then move the VF to that namespace; join will be * done again in that context. @@ -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) { + ret = prog ? netvsc_vf_setxdp(vf_netdev, NULL) : 0; + if (ret) + netdev_warn(ndev, "failed to remove XDP program from VF %s: %d\n", + vf_netdev->name, ret); return NOTIFY_DONE; + } dev_hold(vf_netdev); rcu_assign_pointer(net_device_ctx->vf_netdev, vf_netdev); @@ -2363,9 +2391,6 @@ static int netvsc_register_vf(struct net_device *vf_netdev, int context) vf_netdev->wanted_features = ndev->features; netdev_update_features(vf_netdev); - prog = netvsc_xdp_get(netvsc_dev); - netvsc_vf_setxdp(vf_netdev, prog); - return NOTIFY_OK; } @@ -2441,6 +2466,8 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev) { struct net_device *ndev; struct net_device_context *net_device_ctx; + struct netvsc_device *nvdev; + int ret; ndev = get_netvsc_byref(vf_netdev); if (!ndev) @@ -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)) { + 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); @@ -2664,21 +2705,21 @@ static void netvsc_remove(struct hv_device *dev) cancel_delayed_work_sync(&ndev_ctx->vfns_work); nvdev = rtnl_dereference(ndev_ctx->nvdev); - if (nvdev) { + if (nvdev) cancel_work_sync(&nvdev->subchan_work); - netvsc_xdp_set(net, NULL, NULL, nvdev); - } + + vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev); + if (vf_netdev) + netvsc_unregister_vf(vf_netdev); /* * Call to the vsc driver to let it know that the device is being * removed. Also blocks mtu and channel changes. */ - vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev); - if (vf_netdev) - netvsc_unregister_vf(vf_netdev); - - if (nvdev) + if (nvdev) { + netvsc_xdp_set(net, NULL, NULL, nvdev); rndis_filter_device_remove(dev, nvdev); + } unregister_netdevice(net); list_del(&ndev_ctx->list); -- 2.56.0