Re: [PATCH net-next v8 5/7] net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

Jakub Kicinski
2026-05-01T01:42:33Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

This patch adds XDP_REDIRECT support in the RX path and implements
ndo_xdp_xmit for receiving redirected frames from other devices, batching
frames into ring 16 under a single spinlock. It also advertises
NETDEV_XDP_ACT_REDIRECT and NETDEV_XDP_ACT_NDO_XMIT in xdp_features.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3c3b0c44ea8a..9dd258567824 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -3996,10 +4017,16 @@ static int bcmgenet_xdp_setup(struct net_device *dev,
> return -EOPNOTSUPP;
> }
>
> + if (!prog)
> + xdp_features_clear_redirect_target(dev);
> +
> old_prog = xchg(&priv->xdp_prog, prog);
> if (old_prog)
> bpf_prog_put(old_prog);
>
> + if (prog)
> + xdp_features_set_redirect_target(dev, false);
> +

Does this leave NETDEV_XDP_ACT_NDO_XMIT permanently cleared after a
single attach+detach cycle?

bcmgenet_probe() unconditionally sets:

dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
NETDEV_XDP_ACT_NDO_XMIT;

but on any call to bcmgenet_xdp_setup() with prog == NULL this code
runs xdp_features_clear_redirect_target(dev), which clears both
NETDEV_XDP_ACT_NDO_XMIT and NETDEV_XDP_ACT_NDO_XMIT_SG in
xdp_features_clear_redirect_target_locked() (net/core/xdp.c).

bcmgenet_xdp_xmit() does not depend on priv->xdp_prog — it only needs
netif_running() and priv->xdp_tx_ring, which is initialized
unconditionally in bcmgenet_init_tx_queues(). So after attach+detach,
is_valid_dst() in kernel/bpf/devmap.c:__xdp_enqueue() will see
NETDEV_XDP_ACT_NDO_XMIT cleared and reject every redirected frame with
-EOPNOTSUPP until another program is attached.

The paired xdp_features_set_redirect_target(dev, false) call only fires
for prog != NULL, so it does not restore the bit after a no-op detach,
and it is redundant with the probe-time advertisement when prog is
non-NULL.

> return 0;
> }
>
> @@ -4013,6 +4040,36 @@ static int bcmgenet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> }
> }
>
> +static int bcmgenet_xdp_xmit(struct net_device *dev, int num_frames,
> + struct xdp_frame **frames, u32 flags)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> + struct bcmgenet_tx_ring *ring = &priv->xdp_tx_ring;
> + int sent = 0;
> + int i;
> +
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> + return -EINVAL;
> +
> + if (unlikely(!netif_running(dev)))
> + return -ENETDOWN;

Is there a use-after-free window between this check and a concurrent
bcmgenet_close()?

netif_running() is an unlocked test_bit on __LINK_STATE_START.
__dev_close_many() clears that bit and then calls ndo_stop, which
reaches bcmgenet_close() -> bcmgenet_netif_stop() -> bcmgenet_fini_dma(),
where kfree(priv->tx_cbs) frees the backing array that ring->cbs points
into.

The synchronize_net() inside dev_deactivate_many() only waits for
outstanding qdisc enqueuers, not for a remote device's NAPI loop that
is inside xdp_do_flush() -> __dev_flush() -> bq_xmit_all() ->
bcmgenet_xdp_xmit() on another CPU.

A caller that passed the netif_running() test just before
__LINK_STATE_START was cleared can then execute bcmgenet_get_txcb() and
dmadesc_set() against freed tx_cbs memory after bcmgenet_fini_dma() has
run.

Would an RCU-protected "xmit enabled" flag cleared on close followed by
synchronize_rcu() before freeing the ring be needed here?

Jakub: I think this is worried that sync_net will be skipped if the device
has no qdiscs on down(). I don't think we care.

> +
> + spin_lock(&ring->lock);
> +
> + for (i = 0; i < num_frames; i++) {
> + if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
> + break;
> + sent++;
> + }
> +
> + if (sent)
> + bcmgenet_xdp_ring_doorbell(priv, ring);
> +
> + spin_unlock(&ring->lock);
> +
> + return sent;
> +}

How are completions reclaimed on priv->xdp_tx_ring (ring 16) when the
device has no local RX traffic?

The existing ring 16 has no TX interrupt and is reclaimed only from
bcmgenet_rx_poll(); the driver comment states:

/* ring 16 has no interrupt */

bcmgenet_xdp_xmit() itself has no completion path. When this device is
used purely as a redirect target (receiving frames via ndo_xdp_xmit
from other NICs) with little or no local RX traffic, bcmgenet_rx_poll()
is not scheduled, completed descriptors are never reclaimed,
ring->free_bds stays at 0 after the first burst, and
bcmgenet_xdp_xmit_frame() returns false for every subsequent frame.
bcmgenet_xdp_xmit() then returns 0 (all frames dropped) until unrelated
RX traffic happens to arrive.

The XDP_TX path does not hit this because it runs from inside
bcmgenet_rx_poll(), which drains the ring on each call.

Would scheduling tx reclaim from ndo_xdp_xmit, or installing a
completion interrupt or timer for ring 16, be appropriate here?

> +
> static const struct net_device_ops bcmgenet_netdev_ops = {
> .ndo_open = bcmgenet_open,
> .ndo_stop = bcmgenet_close,

[ ... ]

> @@ -4327,7 +4385,8 @@ static int bcmgenet_probe(struct platform_device *pdev)
> NETIF_F_RXCSUM;
> dev->hw_features |= dev->features;
> dev->vlan_features |= dev->features;
> - dev->xdp_features = NETDEV_XDP_ACT_BASIC;
> + dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> + NETDEV_XDP_ACT_NDO_XMIT;
>
> netdev_sw_irq_coalesce_default_on(dev);