Re: [PATCH net-next 1/5] net: record XDP programs propagated to lower devices

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Critical] A new -EBUSY refusal in netif_xdp_propagate()
  (net/core/dev.c:10415) causes a double bpf_prog_put through netvsc's…
- [Critical] netif_xdp_propagate() only refuses a device's own program
  when bpf->prog is non-NULL (net/core/dev.c:10415).
- [Medium] Only two things clear xdp_from_upper: a later
  netif_xdp_propagate() call and dev_xdp_uninstall() at unregister.
- [Medium] dev_xdp_install() treats any non-HW program in xdp_state[mode]
  as a registered XDP dispatcher user: `if (mode != XDP_MODE_HW)…
- [Medium] On netvsc's ndo_bpf path, netvsc_bpf() → netvsc_vf_setxdp() →
  netif_xdp_propagate(vf_netdev) runs with only RTNL held…
- [Low] The commit fixes user-visible bugs but has no Fixes: tag.
- [Low] The patch adds the bitfield `unsigned long xdp_from_upper:1` to
  struct net_device but does not add it to…
- [Low] netvsc_register_vf() ignores the return value of
  netvsc_vf_setxdp(vf_netdev, prog) and returns NOTIFY_OK.

--

net: record XDP programs propagated to lower devices

When an upper device pushes its XDP program down with
netif_xdp_propagate(), this commit records the program in the lower's
xdp_state[] and tags it with a new xdp_from_upper bit. Bonding and
dev_xdp_attach() use the bit to tell a lower's own program apart from the
upper's. netif_xdp_propagate() now also refuses to propagate onto a device
that has a program of its own.

> is refused by dev_xdp_sb_prog_count() for a device running a
> single-buffer XDP program, but not for a bond slave running the bond's,
> even though netif_xdp_propagate() would have refused to install that
> same program had header-data split been on already. Binding a memory
> provider has the same asymmetry.

[Severity: Low]

Should this have a Fixes: tag?

The one-directional HDS check on the propagate side came from commit
2d46e481a9af ("net: disallow setup single buffer XDP when tcp-data-split
is enabled."). The memory provider side dates to the devmem work, for
example commit 170aafe35cb9 ("netdev: support binding dma-buf to
netdevice").

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d037faff7c44b..512e3a21d0bda 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2517,6 +2517,12 @@ struct net_device {
>  	unsigned long		change_proto_down:1;
>  	unsigned long		netns_immutable:1;
>  	unsigned long		fcoe_mtu:1;
> +	/**
> +	 * @xdp_from_upper: the program in @xdp_state was installed by an
> +	 *	upper device with netif_xdp_propagate(); it belongs to the
> +	 *	upper and can't be replaced or removed through this device.
> +	 */
> +	unsigned long		xdp_from_upper:1;

[Severity: Low]

This isn't a bug, but should xdp_from_upper also be added to
Documentation/networking/net_cachelines/net_device.rst?

The comment at the top of struct net_device asks for that document to be
updated when fields are added. It already lists the neighbouring
change_proto_down, netns_immutable and fcoe_mtu bitfields.

[ ... ]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index a8eb382f40caf..5c67e555f82d6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -10377,13 +10386,39 @@ u8 dev_xdp_sb_prog_count(struct net_device *dev)

[ ... ]

>  int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
>  {
> +	struct bpf_prog *old_prog;
> +	int err;
> +
>  	if (!dev->netdev_ops->ndo_bpf)
>  		return -EOPNOTSUPP;
>  
> +	/* we have more work to do for setup, bypass for other commands */
> +	if (bpf->command != XDP_SETUP_PROG)
> +		return dev->netdev_ops->ndo_bpf(dev, bpf);
> +
> +	if (bpf->prog && dev_xdp_has_own_prog(dev)) {
> +		NL_SET_ERR_MSG(bpf->extack,
> +			       "unable to propagate XDP to device with an XDP program of its own");
> +		return -EBUSY;
> +	}

[Severity: Critical]

Can this new -EBUSY cause a double bpf_prog_put() through netvsc's error
path?

The VF can easily have a program of its own. dev_xdp_attach() on the VF
only refuses when the upper already has one, and netvsc has none yet:

  ip link set dev $VF xdp obj q.o
  ip link set dev $NETVSC xdp obj p.o

The second command then goes roughly like this:

dev_change_xdp_fd(netvsc)            bpf_prog_get()
  dev_xdp_install()                  bpf_prog_inc()
    netvsc_bpf()
      netvsc_xdp_set(dev, prog)      bpf_prog_add(num_chn - 1)
      netvsc_vf_setxdp()             inc, then put on error
        netif_xdp_propagate()        returns -EBUSY
      netvsc_xdp_set(dev, NULL)      bpf_prog_put() num_chn times
  dev_xdp_install()                  bpf_prog_put() on error
dev_change_xdp_fd()                  bpf_prog_put() on error

The rollback in netvsc_bpf() drops all num_chn chan_table references. That
includes the one dev_xdp_install() moved into the driver:

drivers/net/hyperv/netvsc_bpf.c:netvsc_xdp_set() {
    ...
	if (old_prog)
		for (i = 0; i < nvdev->num_chn; i++)
			bpf_prog_put(old_prog);
    ...
}

Then dev_xdp_install() puts that same reference again:

	err = bpf_op(dev, &xdp);
	if (err) {
		if (prog)
			bpf_prog_put(prog);
		return err;
	}

Wouldn't this free the program while the user's fd still points at it?

The imbalance in netvsc's error path is older. Before this patch, though,
only the HDS, memory provider or driver failure refusals could reach it.
This refusal makes it reachable with two ordinary commands.

[Severity: Low]

Related to the above, netvsc_register_vf() ignores the return value of
netvsc_vf_setxdp():

drivers/net/hyperv/netvsc_drv.c:netvsc_register_vf() {
    ...
	prog = netvsc_xdp_get(netvsc_dev);
	netvsc_vf_setxdp(vf_netdev, prog);

	return NOTIFY_OK;
}

Suppose a VF with its own program joins netvsc while netvsc has a
different one. One way this happens is moving the VF into netvsc's netns,
which keeps XDP state. The new -EBUSY is then dropped without any message.
The VF keeps running its own program, and netvsc's program never applies
to VF traffic.

Should this failure at least be logged?

>  	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> -	    bpf->command == XDP_SETUP_PROG &&
>  	    bpf->prog && !bpf->prog->aux->xdp_has_frags) {

[ ... ]

> @@ -10395,7 +10430,23 @@ int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
>  		return -EBUSY;
>  	}
>  
> -	return dev->netdev_ops->ndo_bpf(dev, bpf);
> +	err = dev->netdev_ops->ndo_bpf(dev, bpf);
> +	if (err)
> +		return err;
> +
> +	/* Record it like a program of our own, so that everything which asks
> +	 * whether XDP is running here gets the right answer. @xdp_from_upper
> +	 * keeps the two apart where it matters.
> +	 */
> +	old_prog = dev_xdp_prog(dev, XDP_MODE_DRV);
> +	if (bpf->prog)
> +		bpf_prog_inc(bpf->prog);
> +	dev_xdp_set_prog(dev, XDP_MODE_DRV, bpf->prog);

[Severity: Medium]

Does recording the upper's program here unbalance the XDP dispatcher
accounting?

dev_xdp_install() assumes every non-HW program in xdp_state[] was
registered with the dispatcher by this device:

	if (mode != XDP_MODE_HW)
		bpf_prog_change_xdp(dev_xdp_prog(dev, mode), prog);

Here the program goes into the lower's xdp_state[XDP_MODE_DRV] without a
matching bpf_prog_change_xdp(). Only the upper's own dev_xdp_install()
registered it.

In unregister_netdevice_many_notify(), dev_xdp_uninstall() runs before the
NETDEV_UNREGISTER notifier that makes bonding or netvsc release the lower.
So deleting an enslaved lower, for example with ip link del on a veth bond
slave, goes:

dev_xdp_uninstall(lower)
  dev_xdp_install(lower, XDP_MODE_DRV, ..., NULL)
    bpf_prog_change_xdp(upper_prog, NULL)
      bpf_dispatcher_remove_prog()

Won't this drop the dispatcher user that the bond added? The program would
lose its direct-call entry while the bond is still running it.

> +	dev->xdp_from_upper = !!bpf->prog;
> +	if (old_prog)
> +		bpf_prog_put(old_prog);

[Severity: Critical]

What happens here when bpf->prog is NULL and the lower has its own program
attached through a bpf_link?

The own-program check above is skipped for NULL requests:

	if (bpf->prog && dev_xdp_has_own_prog(dev)) {

In that case dev_xdp_prog() returns link->link.prog, and this block puts
it. That reference belongs to the link. dev_xdp_uninstall() follows this
rule and never puts a link's prog:

	link = dev_xdp_link(dev, mode);
	if (link)
		link->dev = NULL;
	else
		bpf_prog_put(prog);

dev_xdp_set_prog() also clears xdp_state[XDP_MODE_DRV].link here without
setting link->dev = NULL.

netvsc_register_vf() calls netvsc_vf_setxdp() unconditionally, with a NULL
program whenever netvsc has none. It runs from netvsc_probe() for existing
VFs (hv_netvsc reload) and from NETDEV_REGISTER after a netns move.
__dev_change_net_namespace() keeps XDP state. So a VF with a user-created
XDP link can get here. ndo_bpf() drops the driver's reference and this
bpf_prog_put() drops the link's.

Closing the link fd afterwards runs bpf_xdp_link_release().
dev_xdp_detach_link() returns -EINVAL and trips the WARN_ON, and
bpf_link_free() puts the already freed prog.

If the VF is unregistered first, dev_xdp_uninstall() never sees the link.
link->dev then points at a freed net_device, which later
bpf_xdp_link_release(), fill_link_info, show_fdinfo and
bpf_xdp_link_update() calls would use. bpf_xdp_link_update() would also
bypass the new xdp_from_upper check.

For a plain non-link program the refcounts balance, but the user's program
is still removed without any error. Doesn't that go against refusing to
override a device's own program?

[Severity: Medium]

Is anything serializing these new xdp_state[] and xdp_from_upper writes
against the memory provider bind path on netvsc's VF?

netvsc_bpf()->netvsc_vf_setxdp()->netif_xdp_propagate(vf_netdev) runs with
only RTNL held. netvsc_vf_setxdp() only does ASSERT_RTNL() and never takes
the VF's instance lock.

netif_xdp_propagate() otherwise expects the caller to hold that lock.
dev_xdp_propagate() takes netdev_lock_ops(), and
dev_get_min_mp_channel_count() asserts it.

The bind side, netdev_nl_bind_rx_doit()->__netif_mp_open_rxq(), holds only
the VF instance lock and reads the state this patch writes:

	if (dev_xdp_prog_count(dev)) {
		NL_SET_ERR_MSG(extack, "unable to custom memory provider to device with XDP program attached");
		return -EEXIST;
	}

With no common lock, a concurrent bind and netvsc XDP set could each pass
the other's check. Could that leave an ops-locked VF with both an XDP
program and a memory provider? The commit message says this patch now
prevents that combination.

The unlocked VF ndo_bpf() call from netvsc predates this patch.

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(netif_xdp_propagate);

[ ... ]

> @@ -10567,6 +10605,11 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
>  		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified");
>  		return -EINVAL;
>  	}
> +	/* the program belongs to an upper device */
> +	if (dev->xdp_from_upper) {
> +		NL_SET_ERR_MSG(extack, "Can't replace an XDP program installed by an upper device");
> +		return -EBUSY;
> +	}

[Severity: Medium]

Can xdp_from_upper stay set after the upper is gone? If so, this check
would refuse every attach, replace and detach on the lower.

Only a later netif_xdp_propagate() or dev_xdp_uninstall() clears the bit.
netvsc_remove() clears XDP only on the synthetic device:

	netvsc_xdp_set(net, NULL, NULL, nvdev);

Then netvsc_unregister_vf() unlinks the VF without propagating NULL:

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

After rmmod hv_netvsc, a hot remove or an unbind, the VF still has
xdp_state[XDP_MODE_DRV] set with xdp_from_upper=1. It has no upper and its
driver keeps running the program.

This check then returns -EBUSY for every request, including prog=NULL,
until the VF is unregistered. Before this patch the user could attach a new
program and then detach it to clear the stale one.

Bonding can end up in the same state. In __bond_release_one(), a failed
dev_xdp_propagate(slave_dev, NULL) only warns, and the unlink still
happens:

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

dev_xdp_has_own_prog() also returns false for such a device, so a future
upper would propagate over the orphaned program.

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