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

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 13 Nov 2026 23:53:16 +0000 netdev-bot+sashiko@kernel.org wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 69ee4342f43ee..3a520c798d4a8 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9326,6 +9326,9 @@ static void __netdev_upper_dev_unlink(struct net_device *dev,
> >  
> >  	__netdev_adjacent_dev_unlink_neighbour(dev, upper_dev);
> >  
> > +	if (changeupper_info.master)
> > +		dev->xdp_from_upper = 0;
> > +
> >  	call_netdevice_notifiers_info(NETDEV_CHANGEUPPER,
> >  				      &changeupper_info.info);
> >    
> 
> [Severity: Medium]
> The commit message says that clearing the flag here lets the device own a
> leftover program "rather than leave it with nobody allowed to remove it".
> Can a lower end up with xdp_from_upper set when no master link was ever
> created, so that this code never runs?
> 
> netvsc_register_vf() propagates netvsc's program before it joins:
> 
>     ret = netvsc_vf_setxdp(vf_netdev, prog);
>     ...
>     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;
>     }
> 
> The first call stores the program in vf_netdev->xdp_state[XDP_MODE_DRV]
> and sets xdp_from_upper. netvsc_vf_join() can fail in
> netdev_rx_handler_register() or in netdev_master_upper_dev_link(). Both
> failure paths return without a master link.
> 
> If the driver's ndo_bpf() then refuses the NULL program,
> netif_xdp_propagate() returns before it touches xdp_state[] or the flag:
> 
>     err = dev->netdev_ops->ndo_bpf(dev, bpf);
>     if (err)
>         return err;
> 
> netvsc returns NOTIFY_DONE without storing the VF in
> net_device_ctx->vf_netdev, so it never comes back to this VF. No master
> was linked, so __netdev_upper_dev_unlink() never clears the flag. Only
> dev_xdp_uninstall() at unregister does.
> 
> From then on, every IFLA_XDP set or remove and every BPF link attach on  
> the VF hits the new -EBUSY check in dev_xdp_attach(). netvsc's program
> keeps running on the VF. Before this patch xdp_state[] was empty in this
> case, so the user could attach a program and then detach it to clear
> things up.
> 
> The only way out seems to be linking the VF to some master and then
> unlinking it. Should the failed-join path, or netif_xdp_propagate(),
> handle this case?
> 
> bond_xdp_set() has a smaller version of the same problem. If the unwind
> call fails:
> 
>     err_unwind = dev_xdp_propagate(slave_dev, &xdp);
> 
> the slave keeps the new program with xdp_from_upper set, while
> bond->xdp_prog has been reset to old_prog. That slave can't be fixed
> directly until it is released from the bond.

I guess AI semi-randomly anchored this comment here, it's basically
saying that since netvsc is buggy the state may get out of sync.
Yes, as mentioned in the netvsc series I didn't manage to cover all
the error paths there. And also that's the reason why I added the
clearing of "from upper" on unlink, to make the bad state recoverable.

IOW patch is working as intended. We're going from "no safety checks"
to "some already buggy driver error path may fail to clean up".

> > @@ -10561,6 +10564,15 @@ u8 dev_xdp_prog_count(struct net_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_xdp_prog_count);
> >  
> > +/* Does the device have an XDP program, and was it installed directly on the
> > + * device rather than propagated down by an upper with netif_xdp_propagate()?
> > + */
> > +bool dev_xdp_has_own_prog(struct net_device *dev)
> > +{
> > +	return dev_xdp_prog_count(dev) && !dev->xdp_from_upper;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_xdp_has_own_prog);  
> 
> [Severity: Low]
> This isn't a bug, but does this comment still match what the function
> returns? It describes where the program came from. The new kernel-doc in
> include/linux/netdevice.h does too:
> 
>     @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.
> 
> __netdev_upper_dev_unlink() clears xdp_from_upper on any master unlink.
> That includes the case where the upper's NULL propagate failed and
> bonding or netvsc only warned. After that, dev_xdp_has_own_prog() returns
> true for a program that an upper propagated.
> 
> Could both comments describe the flag as current ownership, including the
> hand-over on master unlink?

No, it's a failure mode / de-sync case. We shouldn't document it.

> > @@ -10773,6 +10816,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
> >  	}
> >  
> >  	mode = dev_xdp_mode(dev, flags);
> > +	if (dev->xdp_from_upper) {
> > +		NL_SET_ERR_MSG(extack, "Can't replace an XDP program installed by an upper device");
> > +		return -EBUSY;
> > +	}  
> 
> [Severity: Low]
> This check now runs before the existing loop that refuses an attach when
> an upper has a program:
> 
>     netdev_for_each_upper_dev_rcu(dev, upper, iter) {
>         if (dev_xdp_prog_count(upper) > 0) {
>             NL_SET_ERR_MSG(extack, "Cannot attach when an upper device already has a program");
>             return -EEXIST;
>         }
>     }
> 
> For a direct bond slave or a joined netvsc VF, the upper keeps its
> program in its own xdp_state[]. Attach, replace and detach on the lower
> were therefore already refused, with -EEXIST. They now fail with -EBUSY
> and a different extack message.
> 
> The commit message says dev_xdp_attach() reads the flag "to keep the
> program from being replaced or removed behind the upper's back". However,
> that protection is only new for nested lowers and for the netvsc window
> before the join.
> 
> Is the errno and message change for the common direct-lower case
> intended? If so, could the commit message mention it?

Intended / doesn't matter.