Re: [PATCH net-next 1/5] net: don't safety-check device features for XDP uninstall

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "net: don't safety-check device features
  for XDP uninstall" calls the change a "refactoring / consistency…

--

net: don't safety-check device features for XDP uninstall

This moves the tcp-data-split and memory provider checks in
dev_xdp_install() and netif_xdp_propagate() under "if (prog)", so they
only run when a program is being installed. netif_xdp_propagate() also
now hands commands other than XDP_SETUP_PROG straight to ndo_bpf.

> This way we don't have to worry about triggering WARN()s
> on cleanup paths mistakenly trying to un-install a program
> when there is none, and failing because device has a memory
> provider.
>
> This is a refactoring / consistency cleanup, later patches
> don't depend on this change.

[Severity: Low]
Is this really only a cleanup? It looks like it also fixes an XDP
uninstall failure that can be hit in the current tree.

Before this patch, netif_xdp_propagate() ran the memory provider check
unconditionally, so it also refused to propagate prog == NULL:

net/core/dev.c:netif_xdp_propagate() {
    ...
	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 lower device can run a program propagated from an upper device and
also have a memory provider bound. At this point in the tree,
netif_xdp_propagate() does not record the propagated program in the
lower device's xdp_state, so the reverse checks pass.
__netif_mp_open_rxq() only checks dev_xdp_prog_count(dev), and
ethnl_set_rings() only checks dev_xdp_sb_prog_count().

When a bond slave is in that state, removing XDP from the bond fails:

  bond_xdp_set(bond_dev, NULL)
    dev_xdp_propagate(slave_dev, &xdp)    /* xdp.prog == NULL */
      netif_xdp_propagate()
        dev_get_min_mp_channel_count(dev) != 0 -> -EBUSY

bond_xdp_set() then rolls back, so XDP cannot be removed from the bond.

In __bond_release_one() the same failure only prints a warning:

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

The released slave keeps running the bond's program, and nothing about
it is in the slave's xdp_state. The program then cannot be seen or
removed through that device. netvsc_vf_setxdp(vf, NULL) fails the same
way.

The check being relaxed here came from commit 170aafe35cb9 ("netdev:
support binding dma-buf to netdevice"), which is in current LTS trees.

Should this patch have a Fixes: tag for that commit? Could the commit
message also describe the uninstall failure, rather than calling the
change a refactoring?

A later patch in this series, "net: record XDP programs propagated to
lower devices", fixes the root cause (a memory provider coexisting with
a propagated program). That patch has no Fixes: tag either.

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