Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] The patch introduces an order-dependent state violation where attaching a memory provider and a hardware-offloaded XDP program succeeds or fails depending purely on the order of operations. Pre-existing issues: - [High] Blocking XDP uninstalls when a memory provider is active leads to a resource leak of the BPF program and a permanently active 'zombie' XDP program on slave devices. -- --- Patch [3]: [RFC PATCH net-next 03/13] net: core: add XDP_MODE_HW offload hook for knod --- commit 2719ee9dd236296c86e09fa87d5113250098ff78 Author: Taehee Yoo <[email protected]> net: core: add XDP_MODE_HW offload hook for knod Route XDP program install/uninstall to the knod accelerator when a device is attached, so BPF programs run on the accelerator instead of the host. > diff --git a/net/core/dev.c b/net/core/dev.c > index 7c21bc0a1e349..5ba1c969029bc 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -10334,7 +10334,7 @@ static int dev_xdp_install(struct net_device *dev, > enum bpf_xdp_mode mode, > return -EBUSY; > } > > - if (dev_get_min_mp_channel_count(dev)) { > + if (dev_get_min_mp_channel_count(dev) && mode != XDP_MODE_HW) { [Severity: High] This is a pre-existing issue, but blocking XDP uninstalls when a memory provider is active appears to lead to a resource leak and an inconsistent state on slave devices. If we attach an XDP program to a bonding device, it propagates to slave devices without updating their individual dev->xdp_state. If we then bind a memory provider to the slave device (which succeeds since its local prog count is 0), what happens when we try to destroy the bond device? netif_xdp_propagate() will fail because of the memory provider: 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..."); return -EBUSY; } ... } In dev_xdp_uninstall(), this failure is ignored: net/core/dev.c:dev_xdp_uninstall() { ... WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL)); /* auto-detach link from net device */ link = dev_xdp_link(dev, mode); if (link) link->dev = NULL; else bpf_prog_put(prog); ... } Doesn't this result in the bond dropping its reference and clearing its state, while the slave driver never unloads the program because ndo_bpf is not called? Will this leave a zombie XDP program permanently executing on the slave device and leak the BPF program memory? [Severity: Medium] Does modifying this check create an order-dependent state regression? If a memory provider is attached first, this modified check allows an XDP_MODE_HW program to be installed successfully. However, if an XDP_MODE_HW program is installed first, a subsequent attempt to bind a memory provider fails in __netif_mp_open_rxq() because it unconditionally checks the program count: net/core/netdev_rx_queue.c:__netif_mp_open_rxq() { ... if (dev_xdp_prog_count(dev)) { NL_SET_ERR_MSG(extack, "unable to custom memory provider..."); return -EEXIST; } ... } Should __netif_mp_open_rxq() also check if the existing programs are hardware offloaded to ensure the API behaves consistently regardless of the operation order? > NL_SET_ERR_MSG(extack, "unable to install XDP to device using > memory provider"); > return -EBUSY; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
